fix(webv2): stop erased canvas layers from reporting phantom content - #52
Open
joshistoast wants to merge 5 commits into
Open
fix(webv2): stop erased canvas layers from reporting phantom content#52joshistoast wants to merge 5 commits into
joshistoast wants to merge 5 commits into
Conversation
…ontent
A layer's content rect was pure geometry: a paint source reported its persisted
bitmap's dimensions, and the raster cache only ever grew (strokes chunk-pad their
extent, and the eraser grew it on the same path). Nothing ever shrank it, and
persistence encoded that extent verbatim — so a layer whose pixels had all been
erased kept a full-size bitmap of transparent pixels, and `hittableLayerRect` went
on drawing a movable, transformable blue outline around nothing. It survived
reload, since the paint rasterizer re-sizes the cache from the persisted bitmap.
Make the extent truthful at the persistence boundary:
- `render/alphaBounds.ts` — pure `alphaBounds`/`hasVisiblePixels` over an RGBA
buffer. Any non-zero alpha counts as content, matching the selection mask test.
- `layerCache.shrinkToRect` — the mirror of `growToRect`, cropping via
`resizePreserving` with a negative blit offset (one GPU blit, no CPU round
trip). Bumps the version unconditionally, since a shrink destroys pixels and
must invalidate an in-flight rasterization job; mutates rather than deletes the
entry, so undo can still re-grow into a collapsed cache.
- `render/paintCacheTrim.ts` — reads the cache, crops it to its visible pixels, or
reports `emptied`. Defers while pixels are unpublished or stale (the layer
rasterizer sizes the entry before its async decode fills it, so scanning there
would clear a valid bitmap on every load) and while a gesture, transform/text
session, floating selection or rasterization owns the layer.
- `bitmapStore` — runs the trim after the source-type guard and before reading the
surface, so the flush picks up the trimmed extent and offset with no further
work. A layer left with no visible pixels is cleared to `{ bitmap: null }`
instead of uploading a transparent PNG, dropping its self-echo entry so a later
undo re-dispatching the old image name is not mistaken for an echo.
An emptied layer lands on exactly the source a brand-new layer is created with, so
it stays in the panel — selectable, renamable, deletable — with no outline, no
transform frame and no hit-test, which is what a layer with nothing in it should
do. Fit-to-content and the export guards stop counting it too, and the outline now
hugs the visible pixels rather than the 64-512px growth grid.
The test stub invents readbacks, so `readbackAlpha` lets a test declare that its
surfaces hold pixels; the two suites that drive persistence through the engine's
own bitmap store opt in.
The node stub records draw calls and invents a uniform readback, so it can cover the trim's guard matrix but never its verdict or its crop blit. These run in headless Chromium against real Canvas2D: - `destination-out` over a painted mark empties the cache — the reported bug, reproduced through the exact composite the eraser uses. - A mark at alpha 0.02 survives, pinning the strict-zero threshold: a soft eraser leaves `a_dst * (1 - a_src)` behind and antialiased edges leave a couple of units, so anything higher would silently delete content the user painted. - `shrinkToRect`'s negative-offset blit preserves the retained pixels in every crop direction, discards the rest, and re-grows to transparency rather than smearing the discarded pixels back in.
…mits nothing `resolveTarget` dispatches `addCanvasLayer` at pointer-DOWN, before a single pixel exists, and deliberately outside history — the stroke's composed entry owns the create+paint pair. Its rollback was `cancel: () => undefined`, so any gesture that produced no dirty rect stranded a layer the user could not undo: clicking outside the generation frame with clip-to-bbox on, clicking outside an active pixel selection, pointercancel, switching tools mid-drag, or a throwing session construction. `strokeSession.commit()` returns null in all of them. Give it the real transactional cancel the control-layer branch already has: drop the provisional cache entry and remove the layer. Removing the top layer would otherwise leave the reducer's nearest-remaining-neighbour fallback selecting whatever sits at the top, so the prior selection is restored explicitly. Also re-records the architecture browser baseline for the two modules added by the preceding commit (+2.7 KB script), since the build budget compares source-owner sets by equality.
Cut the restated narrative and kept the load-bearing why: the unpublished/stale guard's load-time hazard, shrinkToRect's unconditional version bump and mutate-not- delete contract, the ground-truth redundancy skip, and the self-echo drop.
joshistoast
requested review from
JPPhoto,
Pfannkuchensack,
blessedcoolant and
lstein
as code owners
August 4, 2026 03:06
joshistoast
enabled auto-merge
August 4, 2026 05:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In the webv2 canvas, a layer could end up visually empty while the Move tool still drew a blue bounds outline around the empty region — draggable and transformable. Deleting the layer was the only way to clear it, and it survived reload.
A layer's content rect was derived from geometry only, never checked against pixel alpha:
document/sources.ts— apaintsource reports its persisted bitmap's dimensions; onlybitmap === nullcounts as empty.render/layerCache.ts—growToRectis documented "Grows (never shrinks)", and the eraser grows it too (strokeSession.ts, compositedestination-out).engine.tsgetLayerSurfacegated only onrect.width > 0, sobitmapStoreencoded, uploaded and dispatched a real bitmap ref for a fully transparent surface.render/rasterizers/paintRasterizer.tsre-sizes the cache from that persisted bitmap, so the phantom survived reload.tools/moveHitTest.tshittableLayerRectis the single chokepoint feeding the move outline, the transform frame and transform eligibility — so all three framed a region holding nothing.This is a regression against the legacy
webfrontend, which handled it inCanvasEntityTransformer(empty pixel rect ⇒ reset the entity rather than frame it) with a worker alpha scan behind aneedsPixelBbox()gate. webv2 had no equivalent — there was no alpha scan anywhere in the bbox path.Reproductions
destination-inclips everything away, yet the stroke still commits.Approach
Make the extent truthful at the persistence boundary. The debounced flush already reads the whole surface to encode it, so the alpha scan is proportionally cheap and never touches the paint hot path.
render/alphaBounds.ts(new, pure) —alphaBounds/hasVisiblePixels. Any non-zero alpha counts as content, matchingselectionState.tsrather than the mask outline's solidity threshold: a higher threshold would silently discard faint pixels the user painted.layerCache.shrinkToRect(new) — the mirror ofgrowToRect, cropping viaresizePreservingwith a negative blit offset (one GPU blit, no CPU round trip). Bumps the version unconditionally, since a shrink destroys pixels and an in-flight rasterization job must invalidate itself rather than resize the trimmed surface back up. Mutates rather than deletes the entry, becauseapplyImagePatchgates undo on the entry existing.render/paintCacheTrim.ts(new) — crops the cache to its visible pixels, or reportsemptied. Defers while pixels are unpublished/stale, or while a gesture, transform/text session, floating selection or rasterization owns the layer.bitmapStore— runs the trim after the source-type guard and before reading the surface, so the flush picks up the trimmed extent and offset with no further work. A layer left with no visible pixels is cleared to{ bitmap: null }instead of uploading a transparent PNG, dropping its self-echo entry so a later undo re-dispatching the old image name isn't mistaken for an echo.An emptied layer lands on exactly the source a brand-new layer is created with, so it stays in the panel — selectable, renamable, deletable — with no outline, no transform frame and no hit-test. Fit-to-content (
fitBbox.ts) andhasExportableLayerContentstop counting it, and the outline now hugs the visible pixels rather than the 64–512px growth grid strokes are chunk-padded to.Also fixed
tools/paintTool.tsdispatchedaddCanvasLayeron pointer-down, before any pixel existed and outside history, withcancel: () => undefined. Any stroke that produced no dirty rect stranded a layer the user couldn't undo — clicking outside the generation frame with clip-to-bbox on, clicking outside an active selection,pointercancel, or switching tools mid-drag. It now rolls back like the control-layer branch already did, restoring the prior selection explicitly (the reducer's nearest-neighbour fallback would otherwise select the top layer).Verification
Driven against the real app with Playwright (mock backend, image upload stubbed since the mock implements only video upload):
canvas-paint-1.pngnullRASTER LAYERS (1)"Layer 1"RASTER LAYERS (1)"Layer 1"Confirmed for both raster layers and inpaint masks (masks share the paint path via
maskAsPaintSource).Gates:
lint(oxfmt + oxlint + tsc + architecture), 5208 node tests, 486 browser tests,test:performance:architecture— all pass.New browser tests assert the real-pixel behaviour the node stub can't: the
destination-outerase that produced this bug, a mark at alpha 0.02 surviving (pinning strict-zero), andshrinkToRect's negative-offset blit in every crop direction.Notes for review
performance/browser-baseline.jsonis re-recorded. The build budget compares source-owner sets by equality, so the two new modules (+2.7 KB script) required it. Timing medians moved as re-measurement jitter.raster.testStub.tsgains areadbackAlphaoption (default0, so every existing call site is unchanged). The stub invents readbacks, and a transparent one now means something — the two suites that drive persistence through the engine's own bitmap store opt into255. Without this the trim would correctly conclude every stub-backed layer was empty.destination-outatglobalAlpha < 1leavesa_dst × (1 − a_src), and antialiased edges leave 1–3/255, so a layer "erased to nothing" with a soft eraser can still report content. Strict-zero is deliberate — repeated passes do reach 0, and relaxing it would delete faint content. If it needs relaxing, the emptiness verdict should move separately from the crop bounds.markLayerDirty, which would re-upload every paint layer on every load.destination-outcan only remove alpha. Clamping the region for that composite is pure waste-removal but shiftsbeforeImageDataand the history patch rects, so it wants its own change.