fix(core): the picker offers what is drawn under the pointer, not see-through layers over it - #4536
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Review at b739dfb8.
Verdict: APPROVE. The rule is sound. In real Chromium, every common visible element stays pickable: fills at any alpha, opacity 0.3, gradients, text, outline text, background-clip: text, pseudo paint, SVG strokes, img, canvas, video, clip-path and mask. The fallback holds. Two things are worth fixing before or soon after merge. Pick mode now flickers to the whole scene because of a pre-existing self-exclusion. And a handful of paint sources the probe never reads now lose to the scene.
How "drawn" is decided
- Pictures: IMG, VIDEO, IFRAME, EMBED, OBJECT and CANVAS always count (
pickerDrawn.ts:6,:173). - SVG: any SVG element other than
svg,gandforeignObjectcounts, fill or not (:8,:174-175). An outersvgorgcounts only through its own CSS paint or a shape it holds. - Fill: a
background-imageother thannonecounts, gradients included (even a fully transparent one). So does a background colour whose alpha is not 0 (:5,:12-14). Opacity is not read here, so 0.3 counts. Onlyopacity <= 0.01hides, checked up the ancestor chain (picker.ts:139-145). - Border: counts only on its band, measured from the bounding box. Each side needs a style other than
noneand a colour that is not transparent (:24-29,:153-168). - Shadow: an inset
box-shadowcounts across the whole box. An outer one never counts (:154). - Pseudo: a displayed
::beforeor::afterwith non-empty content, a fill or a border counts across the host's whole box (:145-150,:169). - Text: the probe takes the glyph rects of the element's own text nodes and its in-flow descendants. It joins them into line boxes and adds the leading between close lines (
:41-67,:123-143). It skipsdisplay:none, opacity 0.01 or less, absolute and fixed descendants, and boxes far from the pointer's row (:82-99). A text node counts if its parent is notvisibility:hiddenand its fill or stroke is not transparent (:17-22,:100-109). An outersvgnever counts text (:178). - Never read:
outline,text-shadow,border-image,backdrop-filter, form-control values and shadow roots. - List: a candidate stays if it draws, or if it holds a candidate that draws. The first
limitof those are returned. If nothing draws, the old top-limitlist comes back (picker.ts:227-240). Visibility now counts only on the element itself (picker.ts:134). That is correct, becausevisibilityinherits.
Visible elements I checked
I ran these in headless Chrome 152 through puppeteer. I bundled the picker twice, from this head and from 9faade09, and put every case over a painted scene. "Offered" means candidate 0.
| Case | main | this PR | Why |
|---|---|---|---|
rgba(255,0,0,0.3) fill |
offered | offered | :14 |
opacity: 0.3 fill |
offered | offered | paints does not read opacity |
linear-gradient fill |
offered | offered | :13 |
| Border band | offered | offered | :167 |
| Border-only box, interior | box | scene (intended) | :167 |
| Text in a transparent div, on glyphs | offered | offered | :138-143 |
| Same div, off glyphs | div | scene (intended) | :138-143 |
Painted ::before |
offered | offered | :169 |
SVG path, fill="none" with a stroke |
offered | offered | :175 |
| SVG box off the stroke | svg | scene (intended) | :174 |
| canvas, video, img in a see-through wrapper | offered | offered | :173 |
-webkit-text-stroke text, background-clip: text |
offered | offered | :21, :13 |
| clip-path shape, masked shape | offered | offered | the hit test already clips |
Shown child of a visibility:hidden parent |
scene | offered (fixed) | picker.ts:134 |
Glow text: color: transparent plus text-shadow |
offered | scene | glyphsPaint ignores text-shadow (:17-22) |
border: 8px solid transparent plus border-image |
offered | scene | borderWidths needs a colour that is not transparent (:26) |
outline with a negative outline-offset |
offered | scene | outline is never read |
| Custom element drawing a canvas in its shadow root | offered | scene | el.childNodes is light DOM only, and the tag is not in PICTURES (:6, :127) |
Round ring (border-radius: 50%), diagonal band |
offered | scene | the band is measured on the square box (:161-167) |
backdrop-filter panel with no fill |
offered | scene | never read |
Transparent <input>, on its value text |
offered | scene | the value is not a child text node |
Fallback: when nothing draws, the list is exactly the old top-limit list (picker.ts:240). I confirmed that in jsdom and by mutation. The list can be empty only when nothing under the point is pickable, the same as on main.
Should-fix 1: pick mode now flickers to the whole scene, and a click can pick it
The root cause is older than this PR. isPickableElement rejects any element with the __hf-pick-highlight class (picker.ts:155), and pick mode puts that class on the hovered element itself (picker.ts:288). So each mousemove skips the element it just highlighted, and the highlight alternates. On main the other element in that alternation was the see-through wrapper. This PR drops the wrapper, so now it is the painted scene.
Real Chromium, with an <img> in a see-through wrapper over a painted scene. I sent 6 mousemoves, then a click:
- main: highlight
pic,picWrap,pic,picWrap,pic,picWrap, click candidates["#picWrap","#scene"] - this PR: highlight
pic,scene,pic,scene,pic,scene, a full-frame outline flashing on every move, click candidates["#scene"] - this PR with line 155 deleted: highlight
picsix times, click candidates["#pic","#picWrap","#scene"].picker.test.tsstill passes 42/42.
The PR body lists pick mode's hover and click as fixed. The Studio rig in the body calls getCandidatesAtPoint, which sets no highlight, so the rig could not show this. I suggest deleting line 155, since the highlight is an outline on the element and not a separate node, and adding a hover-then-click test.
Should-fix 2: paint the probe does not read
These are the rows marked scene above. Each one is visible. In a real composition each sits on a painted scene, so it loses to the scene before the fallback can apply. Cheap fixes:
- Custom elements: treat an element with a shadow root, or a tag name with a hyphen, as a picture (
pickerDrawn.ts:173).elementsFromPointretargets a hit inside a shadow root to its host, and a closed root can't be seen from outside at all. HF's own lottie guidance uses light DOM. Third-party players such as<dotlottie-player>and<model-viewer>are web components. text-shadow: count glyphs inglyphsPaintwhentextShadowis notnone(:17-22).border-image: count the band inborderWidthswhenborderImageSourceis notnone(:24-29).- Form controls: add INPUT, TEXTAREA, SELECT and BUTTON to
PICTURES. - A negative-offset outline,
backdrop-filterand rounded bands can go in the known limits instead.
Should-fix 3: hover cost grows with every child of a see-through text block
closed() reads getComputedStyle and getBoundingClientRect for every child element before the row check prunes it (pickerDrawn.ts:82-99, :130). There is no layout thrash. These are reads only, and elementsFromPoint has already laid the page out. But the walk is linear in the number of children. Real Chromium, median of 21 calls to getCandidatesAtPoint(x, y, 1) on this box:
- 300-word paragraph, pointer on empty space: main 0.10 ms, this PR 0.10 ms
- 3000 absolutely positioned particles in a see-through layer: main 0.9 ms, this PR 3.7 ms
- 5000 per-word spans in one see-through paragraph, pointer in the leading: main 0.7 ms, this PR 13.1 ms
The last case is in the body's known limits. The particle case is 4x here, where the body reports 0.7 ms to 1.1 ms. So "within about 0.4 ms" holds for some layouts and not others. Pick mode's mousemove handler is not throttled. This is not a blocker. Two options: run pick-mode hover at most once per animation frame, or cap how many children one pick walks.
Nit
- A see-through layer with nothing drawn at the point is now gone from the candidate list, not just moved down it. For example, a captions section between two captions can't be picked on the canvas while a painted scene is under it. You could append the dropped candidates after the drawn ones, up to
limit. Candidate 0 would stay the same, and the layers would stay reachable inelement-pick-candidates. This is a design call, and either way is fine.
Callers
getPickCandidatesFromPointfeedsgetCandidatesAtPoint,pickAtPoint,pickManyAtPointand pick mode's click, all throughgetPickInfosFromPoint(picker.ts:264-272). It also feeds pick mode's hover, with limit 1 (picker.ts:276-281).createDrawnProbeandisEffectivelyHiddenhave no other callers.- In this repo the only consumer of the picker messages is
useElementPicker(studio/src/hooks/useElementPicker.ts:114-121), and it takes candidate 0. The package exports it, but Studio itself does not use it. - Studio's own canvas hover and click go through
getPreviewTargetFromPointerandgetAllPreviewTargetsFromPointer(studio/src/utils/studioPreviewHelpers.ts:205-300), which walkelementsFromPointthemselves. The SDK iframe adapter also has its own walk (sdk/src/adapters/iframe.ts:791-796). Neither calls this picker. So Studio's normal canvas selection does not change with this PR. The change reaches__HF_PICKER_APIembedders and pick mode. - No drop-target, marquee, keyboard or agent path calls it. In hyperframes-internal and pacific the only hit is a bundled runtime copy (
demo-next/public/hyperframe-runtime.js). - The index claim holds. For any
limitbelow 8, the list is a prefix of thepickAtPointlist. The loop breaks only on drawn candidates, and the fallback fires only when nothing draws.
Checked
picker.test.tspasses 42/42.src/runtimepasses 1382/1382 across 61 files, run frompackages/core. Run from another directory,init.test.tsfails because it reads a relative path; that is unrelated to this PR.tscis clean ontsconfig.jsonandtsconfig.runtime.jsononce the runtime artifact is generated. oxlint reports 0 warnings and 0 errors, and oxfmt is clean on the 3 files.- Four mutations, each restored afterwards:
- Containment removed: 2 tests red (picture box, SVG shapes).
- Fallback removed: 2 red (section host,
id="0"). rgba(..., 0)treated as paint: 16 red.- Own-visibility check removed: 1 red.
- 15 scratch jsdom probes, since deleted.
- A puppeteer page with 32 probe points, comparing the head bundle with the
9faade09bundle, plus the pick-mode sequence and the timings above. - Existing reviews: none. The only comment is the fallow sticky, with 2 CRAP-score notes on
closedanddecoratedAtand 4 test clones.
Not verified
- The Studio UI and the HF-heygen-stripe film. I did not rerun the Before and After clips.
- Rotated elements, and browsers other than Chromium.
- Whether any external embedder relies on see-through layers appearing in the candidate list.
— Rames
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 8d73d3d0, following my APPROVE at b739dfb8.
Verdict: the code is approve-worthy, but I'm leaving a comment instead of an approval, because this head now conflicts with main. #4538 merged at 13:46Z (0b24a0ea). This repo requires approval on the last push, so an approval here would not carry over to the push that resolves the conflict. I'll approve the resolved head.
The conflict: git merge-tree --write-tree 8d73d3d0 0b24a0ea conflicts only in picker.test.ts, because both PRs add a describe block at the same spot, just before "escape key handler". picker.ts merges on its own and drops the self-exclusion line (:155), so S1 below is fixed by the merge. The fix is to keep both blocks. The markers split at a shared closing, so #4538's block needs its own restore(); } }); }); before this PR's describe("picks what is drawn under the pointer") starts. With both blocks kept, the picker tests pass 43/43 (this PR's 42 plus #4538's one).
What changed
One commit, 2 files, +70 -73. picker.ts has no diff.
pickerDrawn.ts: the body ofclosedmoves into two module-level rules,closesText(:42-46) andfarFromRow(:50-55). The border part ofdecoratedAtmoves intoonBorderBand(:58-72).closednow setsknownin one line (:118), anddecoratedAtis one||chain (:176-180).picker.test.ts: acandidatesUnderhelper (:46-53) replaces two copies of the hit-test, try and finally block (:223,:286). The two far-row tests become oneit.eachwith two rows (:640-660).
Behaviour check
I compared each rule with the old code, branch by branch.
closesTextreturns the same boolean as the oldknownline. Only the order changed. The old code read opacity and the grading attribute first, and the new code reads them last. Both are plain reads that have no side effects and cannot throw, so the order does not matter.farFromRowkeeps the same font reach, the same cap and the same row test. It runs only whenclosesTextis false andlayoutis true, which matches the oldif (!known && layout). SogetBoundingClientRectruns in exactly the same cases.layoutis a boolean (:108), soknownstays a boolean.- The zero-size guard is now written the other way round (
:51, was(r.width > 0 || r.height > 0) &&). The two forms agree on every real rect. They differ only for a NaN width with a height of 0 or less, andgetBoundingClientRectnever returns that. onBorderBandis the old border block, moved without changes. With no border it returns false, and the chain goes on to the pseudo checks as before.xandyare now passed in, and they are the probe's ownxandy(:178).decoratedAtkeeps the same order: inset shadow, border band,::before,::after. It is now an arrow function, not a hoisted declaration. Only the returned closure calls it (:188), and that runs after it is defined, so this is safe.
I also ran both versions side by side in headless Chrome 152. I bundled createDrawnProbe from b739dfb8 and from this head into one page with 250 elements. The page covers fills, borders, rings, text, text-shadow, text stroke, pseudo paint, SVG, pictures, form controls, a custom element with a shadow root, faded words, a grading-hidden node, absolute and fixed children, scaled and rotated boxes, overflowing text and a zero-size box. Over 2,814,253 calls on grids of points, the two versions gave 0 different answers.
To show the page can catch a difference, I seeded five changes into the new code, one at a time. Four showed up: dropping the opacity rule (fadeLine), ignoring the border scale (scaled), turning off farFromRow (ovWrap, negWrap), and treating a zero-size box as far (zeroHost, zeroWrap). The fifth, removing the reach cap, did not show up, because no box on the page is tall enough for the cap to apply. The jsdom tests cover the cap (below).
Tests
- There are 42 tests before and after. The names differ only where the two far-row tests became the two
it.eachrows. - The assertions are the same. The helper returns the same selectors that
getCandidatesAtPoint(10, 10)gave, and the two converted tests still expect["#code", "#card"]and["#aroll", "#root"]. Bothit.eachrows expect["#bg"], as the two old tests did. The second row's rect is exactly the old tall-box rect (top 400, bottom 700, height 300). - One small fixture change. The first row now puts the far rect on a div around the paragraph. The old test put it on the
<p>itself (old:641). It still goes through the sameclosed()call on the layer's child. The label still says "a paragraph far below", so "a box far below" would be more exact. Not a blocker. - Cross runs: the old test file passes 42/42 against the new source, and the new test file passes 42/42 against the old source.
- Mutations, each run against both test files and then restored.
farFromRowalways false turns 2 old and 2 new tests red. Removing the reach cap turns 1 old and 1 new test red.onBorderBandalways false turns 2 old and 2 new red. Dropping the opacity rule turns 1 old and 1 new red. So the tests are as strong as before.
Fallow
At b739dfb8, Fallow failed (job 108408040807) with 6 findings. Two were CRAP scores over the 30 threshold: closed at 43.1 and decoratedAt at 31.6. Four were test clones in 2 groups, at picker.test.ts:213 and :282, and at :648 and :684. I ran fallow audit --base origin/main --fail-on-issues (fallow 2.75.0) locally at both heads. b739dfb8 gives the same 6 findings. This head gives "No GitHub PR/MR findings". In CI, Fallow audit passes at 8d73d3d0 (job 108411904645), and the sticky comment is gone.
Prior should-fixes
- S1, pick-mode flicker: unchanged at this head.
picker.ts:155still rejects__hf-pick-highlight. #4538, now onmain, removes it, and the merge above keeps that removal. - S2, paint the probe does not read: unchanged.
glyphsPaint(:17-22),borderWidths(:24-29) andPICTURES(:6) are the same as before. In Chrome, both heads say "not drawn" for text-shadow glow,border-image, a negative-offset outline, a canvas in a shadow root, input value text,backdrop-filterand a round ring's corner band. - S3, hover cost: unchanged, as expected for a pure refactor.
closedstill reads the style and rect of every child before the row check (:117-118). I timed the median of 21 probe calls on a 5000-span paragraph, with the pointer in the leading. Old: 12.0 and 11.7 ms. New: 12.2 and 11.0 ms.
#4538
The self-exclusion line is still at picker.ts:155 here. I ran this check before #4538 merged, against its head f486ac46, which is the code that landed. git merge-tree --write-tree 8d73d3d0 f486ac46 merges picker.ts cleanly and drops the line. It conflicts in picker.test.ts, because both PRs add a new describe block at the same spot, just before "escape key handler". The same conflict exists against b739dfb8, so this commit did not cause it. Keeping both blocks works. With the merged picker.ts and both sets of test additions, the picker tests pass 43/43. #4538 landed first, so this PR needs that rebase (see the top).
Checked
git diff b739dfb8 8d73d3d0, read in full, and the fullpickerDrawn.tsat this head.vitest run src/runtime/picker.test.tsfrompackages/core: 42/42.- The cross runs and mutations above, all restored afterwards.
- The Chrome 152 page comparing old and new: 2,814,253 calls, 0 differences, and 4 of the 5 seeded changes caught.
oxlint: 0 warnings and 0 errors.oxfmt --check: clean on the 2 files.- Local Fallow at both heads, as above.
gh pr checks 4536: every check passes at this head, including Typecheck, Lint, Format, Test (runtime) and Fallow audit. CI run 36244782233 is on8d73d3d0.- Reviews: my approval at
b739dfb8is the only one, andreviewDecisionisREVIEW_REQUIREDat this head.
Not verified
- Miguel's own 41-case Chromium rig. I used my own page instead.
- The Studio UI, and browsers other than Chromium.
— Rames
…-through layers over it
8d73d3d to
f5f91f9
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at f5f91f97, following my APPROVE at 8d73d3d0.
Verdict: APPROVE. This is a clean rebase onto main after #4538. The PR's own changes are line-for-line the ones I approved, and the one test-file conflict keeps both blocks.
What I compared
- The head is one commit. Its parent
89fe898cis on main and contains #4538's merge commit0b24a0ea. - The PR's own diff against its parent has the same 631 added and removed lines as the old diff against its old base
9faade09, compared line by line. Both are 3 files, +608/-23. pickerDrawn.tsis byte-identical to8d73d3d0(blob97238994).picker.tsdiffers from8d73d3d0by one line: the__hf-pick-highlightself-exclusion at old:155is gone. That is exactly #4538's change, so my S1 (pick-mode flicker) is now fixed on main.picker.test.tsdiffers from8d73d3d0by exactly #4538's +35 lines. Both describe blocks are there, "pick mode under a resting pointer" (:355) and "picks what is drawn under the pointer" (:388), both before "escape key handler" (:778). This is the keep-both resolution I described last round.
Checked
vitest run src/runtime/picker.test.tsfrompackages/core: 43/43.- The
packages/corevitest run: 3113 passed, 1 skipped. 6 files could not load because sibling workspace packages are not built in my worktree (generated/runtime-inline,@hyperframes/lint,@hyperframes/studio-server/*). None of them import the picker. tsc --noEmit -p tsconfig.runtime.json: clean.oxlintandoxfmt --checkon the 3 files: clean.
Still open from earlier rounds, not blocking
- S2 (paint the probe does not read) and S3 (hover cost) are unchanged, as expected for a rebase. The test label "a paragraph far below" nit also stands.
— Rames
What was broken
The preview runtime's picker (
window.__HF_PICKER_API:getCandidatesAtPoint,pickAtPoint,pickManyAtPoint, and pick mode's hover and click) listed every pickable element under the point, top first, and stopped at eight. It never asked whether an element draws anything there.pickAtPoint(x, y)picked something the viewer cannot see. In the fixture film, the captions section covers the whole frame, so a pick anywhere outside the caption line returned that section.visibility: hiddenon any ancestor excluded an element, but a child withvisibility: visibleis drawn.The fix
packages/core/src/runtime:pickerDrawn.ts: one drawn-pixels probe per pick. An element draws at the point if it has any of:::before/::afterwith content, paint or a border;Text is measured per line box, so the gap between two words of a title counts as the title's. The rest of a text box does not count. Text counts only for the box that lays it out: an absolutely or fixed positioned descendant is its own box, so its text is not its ancestors'. Text paints with its fill colour or its stroke, so outline text counts and a transparent fill does not. Words at opacity 0 and text that lays out no boxes (an SVG title, fallback content) count for nothing. An outer
<svg>counts through its shapes or its own paint, not its whole box.Cheap enough for pick mode's hover. Per pick, each element's nearby text is collected once and shared by every candidate that holds it, so nested wrappers cost nothing extra. Hidden and out-of-flow subtrees are skipped, as is any box too far above or below the pointer for its lines to reach it (three font sizes, or twice its height up to twenty font sizes, whichever is more). An element with no children is never styled, and only glyphs near the pointer's row are kept.
picker.ts: the candidate list keeps what is drawn, before the cap. A candidate stays if it draws at the point or holds a candidate that does. So a click on a title still climbs to its card and its section, and the first eight kept are returned.Nothing drawn at the point (an empty area of a section): the list is what it was before, so clicking a section's empty area still picks its host.
Visibility counts only on the element itself. The runtime hides container clips with
visibility: hiddenso that a child inside its own window can show itself; those shown children were unpickable.display: noneand opacity still count all the way up.What an embedder sees: a see-through wrapper is now offered only where something inside it draws, or where nothing draws at all. An index taken from
getCandidatesAtPointstill picks the same element inpickAtPoint.Known limits:
::before/::aftercounts across its host's whole box. The page exposes no box for a pseudo-element.overflow: hiddenancestor still counts where its glyphs sit.pointer-events: none: the card is picked only where it draws itself.Hover cost, measured in Chromium: the median of 21
getCandidatesAtPoint(x, y, 1)calls, the call pick mode makes on each mousemove.Measured in Studio
A fixture film at 00:06 (the lockup: two logos under a full-frame captions section). The rig calls
getCandidatesAtPoint(x, y, 8)at the cursor, the way an embedder does. It outlines candidate 0, which is whatpickAtPoint(x, y)picks.#heygen#plusimg#cap2#cap2Tests
picker.test.ts, new cases in "picks what is drawn under the pointer":pickAtPointpicks it);The 21 existing tests pass unchanged, including the one where an empty section click picks the host. Breaking each rule in turn fails its own test: containment, the fallback, own visibility, the SVG box, SVG paint and SVG text, the border band and each axis of its scale, text paint, transparent text, absolute and fixed text, a text-node cap, the row distance and its cap, text without boxes, outer shadows, empty and hidden pseudo-elements.
picker.test.ts42/42, the core runtime suite 1382/1382.entry.test.tsfails now and then under the full parallel run and passes alone; it does not touch the picker. oxfmt, oxlint and the comment ratchet exit 0.Before
Main: the pointer is on the HeyGen logo, and the pick is the see-through captions section around the whole frame.
Capture removed: it showed a private project; a fixture capture will replace it.
After
The same steps on this branch: each pick is the logo or mark under the pointer. The caption line still picks the caption.
Capture removed: it showed a private project; a fixture capture will replace it.