fix(camera): spotlight() paints over its own target - #44
Conversation
spotlight() builds a full-viewport scrim and cuts a hole in it by tracing the viewport rectangle and then the target rectangle inside the same polygon. CSS polygon() expresses a single path, so whether that second ring clears the first is decided entirely by the fill-rule. None is declared, so it defaults to nonzero, which clears an inner ring only when that ring winds opposite to the outer one. Both are written in the same order, top-left, bottom-left, bottom-right, top-right, so nonzero counts the interior as inside and fills it. The result is that spotlight() has never cut a hole. It paints a flat scrim at the full opacity over the entire viewport for its whole duration, target included, with no exception and no console warning. Measured in Chromium against a white target on a mid-grey field at the default opacity of 0.7: every pixel is multiplied by 0.3, so the target reads 76 of 255 rather than 255, and the field around it reads 38. With evenodd the target stays at 255 while the field still reads 38. dimAround() carries the same markup but only on its Locator fallback path, where sibling dimming is not possible. Given a CSS selector it dims siblings by opacity and is unaffected. evenodd also makes the cutout independent of vertex order, so a later refactor that reorders these points cannot silently reintroduce this. The regression test renders the effect and compares screenshot bytes of two clip regions, one inside the target and one away from it. The existing camera tests mock page.evaluate and assert the arguments handed to it, which cannot observe whether the hole appeared, and that is why this survived. Comparing encoded bytes keeps the test free of an image-decoding dependency, and the second region is what stops the first assertion from passing vacuously: an overlay that never rendered would also leave the target untouched.
The eight line note added with the fix was the longest comment in a file whose next longest is five and whose median is one, and most of it retold a failure narrative that belongs in the PR rather than beside the code. What survives is why `evenodd` rather than reversing a ring. The dimAround copy restated it instead of pointing at it, and the test explained in prose what its own assertion messages already say. Comments only. No behaviour change.
Five suites gated themselves behind `describe.skip` when a loopback socket or a browser binary was unavailable. That is right locally — not every contributor has every browser, and a sandbox may refuse to bind — but in CI those prerequisites are installed deliberately, so a missing one means the workflow drifted and the whole suite quietly retires itself with a green build. That is not hypothetical. It is the same shape as the bug #44 fixes: an effect nothing could observe, so nothing caught it. It also nearly bit this repo already — `tests/e2e/record.e2e.test.ts` happened to guard on `canBindLocalhost()` and so hard-failed when browsers were missing, while a browser-guarded suite in the same run would have silently vanished. `describeWithCapability(available, requirement)` keeps the local skip and turns the CI case into a failing test naming the missing prerequisite and pointing at the workflow rather than the assertion. Verified by mutation: with the capability forced false, the suite skips green with CI unset and fails with CI=1; it passes again once restored. Note for #44: tests/e2e/camera.e2e.test.ts introduces another `canLaunchChromium() ? describe : describe.skip` site. It should move to this helper once that PR lands.
|
Thanks @Joilence — this is a great catch, and a satisfying one: a two-word fix for a bug that made Verified rather than taken on faith: The winding analysis holds. Both rings traverse top-left → bottom-left → bottom-right → top-right, so under the default Cross-browser. No keyhole seam. The polygon bridges the outer ring to the inner one with a doubled-back edge, which under The test isn't vacuous. Reverting Your comment choice is the right one, too. Reversing the inner ring's winding would also work, but One follow-up I'll handle rather than ask you to: Merging. Thanks again. |
Two files, one planned and one a correction. tests/e2e/camera.e2e.test.ts arrived with #44 and used the old `canLaunchChromium() ? describe : describe.skip` shape. It is the suite that most needs the harder guard: it exists because `spotlight()` painted over its own target for as long as nothing could observe it, and a silent skip would put it right back in that state. tests/dashboard.test.ts should have been converted in 7d0bded and was not. Restoring the file after a mutation check reverted the wiring before it was staged, so that commit carried four of the five suites it described. No `? describe : describe.skip` sites remain now. Verified the same way: with the capability forced false, each fails under CI=1 and skips green without it. 753 tests pass.
2 participants
Why
spotlight()has never cut its hole. It paints a flat scrim over the whole viewport for its full duration, target included.Mechanism. The overlay traces the viewport rectangle and then the target rectangle inside a single
clip-path: polygon(...). CSSpolygon()accepts one path, so whether the second ring clears the first is decided entirely by the<fill-rule>it is given. None is declared, so it defaults tononzero, which clears an inner ring only when that ring winds opposite to the outer one. Both are written in the same order, so the interior fills instead.Measured. Chromium, white target on a mid-grey field, default
opacity: 0.7:nonzeroevenoddBefore, every pixel is multiplied by 0.3. After, only the field is.
Reach. The effect is public API, documented as "Dark overlay with hole around target element", and called at five sites in three shipped demos:
argo-launch.demo.ts#L35#hero-command, 5000 msshowcase.demo.ts#L54#hero-command, 4800 msshowcase.demo.ts#L113#effect-spotlight, the card advertising the effectmobile.demo.ts#L46mobile.demo.ts#L58#order-btnWhy it survived. Two blind spots, both worth knowing before reviewing:
tests/camera.test.ts#L151mockspage.evaluateand asserts the arguments handed to it, so no existing test can observe whether the hole appeared.pointer-events: none, soelementFromPointreaches the target through the scrim whether or not the hole exists. That produced a false negative for me before I switched to reading pixels.Nothing errors along the way, since both polygons are legal shapes. The only signal is a dark frame.
What
Prepend
evenoddto both cutout polygons, which counts ray crossings and ignores their direction.Reversing the inner ring would also work under
nonzero, but that fix lives entirely in the order of eight coordinates, so any later tidy-up silently re-breaks the effect.evenoddmakes the cutout independent of vertex order.Scope, for the reviewer:
spotlight()uses the cutout unconditionally, so it is always affected.dimAround()shares the markup, but only on its Locator fallback path. Given a CSS selector it dims siblings by opacity and is unaffected. Fixed anyway, since the two share the cutout.focusRing()andzoomTo()have no cutout and are untouched.The second commit only trims comments, to keep the note in
camera.tsin the register of the surrounding file.Test
New
tests/e2e/camera.e2e.test.ts, gated oncanLaunchChromium()the same waypreview.e2e.test.ts#L27is. CI installs chromium beforenpm test, so it runs rather than skips on both matrix nodes.mainspotlight painted over its own target: the cutout did not clear a holeHow it works, since the approach is unusual. It renders the effect and compares screenshot bytes for two clip regions, one inside the target and one away from it. Identical pixels from the same browser encode to an identical PNG, so byte equality answers the question without an image-decoding dependency. The second region is what stops the first assertion from passing vacuously: an overlay that failed to render at all would also leave the target untouched, so the scrim has to be shown to exist somewhere.
Not verified
The five demo call sites above are reach derived from the code path. I have not re-rendered those demos to confirm the effect frame by frame.