feat(sdk): expose a paint query for transparent-composition hit-testing - #3070
Conversation
A host layering a transparent composition over other content has to know whether a point carries ink before it decides to swallow a click. The adapter only answered "what element is here", so AI Studio wrote its own answer and could not reach the per-pixel alpha the adapter already samples for <img>. Add PreviewAdapter.paintsAt, plus the pieces it is built from on a new ./adapters/iframe subpath so a host with different hit-test policy can compose its own walk. The walk is geometric rather than elementsFromPoint-based: that stack omits pointer-events:none nodes, and a decorative overlay carrying it still paints, so a z-stack query would report no ink over visible artwork — the direction that makes a composition vanish from under the cursor. fullBleedFraction is an option rather than a constant because "a layer covering the whole frame is background, not artwork" is host policy, not a fact about the composition.
10c6989 to
46d405e
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 46d405e.
Solid piece of work — the diagnosis is right (a z-stack query built on elementsFromPoint reports no ink over pointer-events: none overlays and would make transparent artwork vanish from under the cursor), the fix goes the correct direction (geometric box walk, blind to pointer-events and z-index by design), and the alpha-routing exception for <img> / <picture>-wrapped <img> closes the transparent-PNG-over-gap corner cleanly. The mutation-tested pins on element-only opacity and eager ink evaluation (per PR body) are the right places to defend — those are the two shapes a well-intentioned refactor would slip back into.
Small details I liked:
<picture>deferring to the inner<img>rather than painting unconditionally — the standard-SDK bug I would have expected in a first cut.hasOwnTextfiltering direct-child text nodes, so a wrapper around a caption doesn't paint. The comment explaining why (else every ancestor of a text node counts) is exactly the shape of comment that saves the next reader a diff walk.- Lazy alpha sampling ordered smallest-area-first with the counting-canvas test pinning "one read for two candidates" (
iframe.test.ts:1299). The right kind of pin for a silent per-frame cost. _MAX_ALPHA_TEST_PIXELS = 16_000_000fail-safe to opaque above the 4K×4K threshold — the right direction, and the guard is at the top ofimageAlphaOpaqueAtwhere the memory allocation hasn't happened yet.nullfrompaintsAtdocumented as "callers must treat as painted" both in the interface docstring and the guides recipe (docs/sdk/guides/canvas-integration.mdx:131). The failure direction is the one that keeps content visible; naming it in three places raises the odds every consumer gets it right.- Cross-origin same-origin contract asymmetry:
paintsAtguardscontentDocumentaccess in a try/catch (returnsnull),elementAtPointdoes not (throws). Documented as "iframe MUST be same-origin" for the older path and "not knowable" for the new one — the ambient contract forpaintsAtis honestly softer thanelementAtPoint's, and separating them here is right. - The
INTRINSIC_PAINT_TAGSexport path (@hyperframes/sdk/adapters/iframe) is the right minimal surface — hosts whose hit-test policy differs get the primitives without inheriting the adapter.
Findings inline — one blocker on a docs↔code contract mismatch (fullBleedFraction's reference-frame semantics), one small concern on the transparent-background set-based match, one nit on INTRINSIC_PAINT_TAGS documentation. The blocker matters more than usual because the AI Studio consumer (PRINFRA-397) is the first caller and the shape locks in once this ships.
Cross-repo coordination note. The PR body names the follow-up — HeyGen AI Studio deletes its own heuristic and calls paintsAt once this releases. Two things worth pinning while the shape is still soft:
- The
boolean | nulltri-state onpaintsAtneeds the consumer to treatnullas painted (per the docs' three placements). Worth a one-liner in the PRINFRA-397 delete PR pointing at the recipe incanvas-integration.mdxso the pattern lands in the consumer identically. - If AI Studio's current heuristic passes any non-default
fullBleedFraction, the blocker above affects the migration diff. If it always passes 0 (literal-ink), the blocker doesn't move the needle for Studio's specific use — but customers reading the SDK docs still see the promise that isn't kept.
Bundle-size impact. packages/sdk/package.json adds one subpath (./adapters/iframe) and re-exports PaintsAtOptions from the barrel. The concrete adapter factories on the main barrel are unchanged. The new module-level state (_imgCanvasCache, _warnedTaintSrcs, _MAX_ALPHA_TEST_PIXELS, _IMG_CANVAS_CACHE_MAX) is scoped to adapters/iframe.ts, so a consumer that only pulls the headless adapter doesn't get any of it. Fine.
Fallow config change (.fallowrc.jsonc:552) exempts packages/sdk/src/adapters/iframe.test.ts from the pre-existing dupes-clone finding — the paint-query suite appends and the line-shift re-flags legacy per-case arrange blocks in the selection / draft-loop suites. Correct call; the comment names it clearly.
| if (rect.width > 0 && rect.height > 0) smallest = Math.min(smallest, rect.width * rect.height); | ||
| }); | ||
| return smallest; | ||
| } |
There was a problem hiding this comment.
🔴 Docs promise point-relative, code does document-global. docs/sdk/reference/adapters.mdx:183 says:
Nested sub-compositions carry
data-composition-idtoo, so the reference frame is the innermost composition root containing the point.
but this function takes Math.min across every [data-composition-id] in the document, regardless of whether the point falls inside those roots. When a click lands outside a small nested sub-composition but inside the outer frame, the outer frame gets measured against the sub-composition's tiny area.
Failure scenario. Outer composition 1920×1080 with a 200×200 nested sub-composition in one corner. Click at (1500, 500) — outside the nested, inside the outer. Winner: a 1000×600 painted div living only in the outer (area 600 000). fullBleedFraction: 0.9.
- Per the docs (frame = outer 2 073 600):
600 000 < 0.9 × 2 073 600 = 1 866 240→ paints (correct). - Per the code (frame =
min(2 073 600, 40 000) = 40 000):600 000 < 0.9 × 40 000 = 36 000is false → not painted.
Studio would see the composition vanish under the cursor at the outer regions any time a smaller nested sub-composition exists. The AI Studio consumer (PRINFRA-397) is the first caller and the shape locks in once this ships, so worth resolving before the SDK release rather than as a follow-up.
packages/cli/src/cloud/detectAspectRatio.test.ts:163-164 documents nested sub-compositions as a real production shape, and this PR's own test (iframe.test.ts:1234) covers point-INSIDE-nested where both interpretations happen to agree — the point-outside-nested case is the untested one.
Either route is fine (pick the one that matches your intent):
A. Make the code point-relative (match the docs). One-liner in compositionFrameArea: instead of every [data-composition-id], walk the winner's ancestor chain and take the smallest ancestor's area — or, since the winner is what's being measured, just find the smallest [data-composition-id] whose rect contains (x, y).
function compositionFrameArea(doc: Document, x: number, y: number): number {
let smallest = Infinity;
doc.querySelectorAll("[data-composition-id]").forEach((root) => {
const rect = root.getBoundingClientRect();
if (rect.width > 0 && rect.height > 0 && boxContains(rect, x, y)) {
smallest = Math.min(smallest, rect.width * rect.height);
}
});
return smallest;
}Then thread (x, y) through, and add a test for point-outside-nested-inside-outer with a mid-size painter to pin the correction.
B. Relax the docs to match the current code — "any nested sub-composition present in the document tightens the reference frame for the whole document" — and add a note that this is deliberately global, not point-relative. Losing the point-relative promise makes fullBleedFraction less useful for compositions with size-diverse nested sub-comps, so my read is that (A) is the better direction, but you'd know Studio's use pattern better than I do.
Either way, please pin the currently-untested case (point outside nested, winner in outer) so the two branches don't diverge again. — Rames D Jusso
There was a problem hiding this comment.
Confirmed, and it's worse than nested-only — thanks for catching it.
The two roots need no containment relationship at all. Math.min was document-global, so a 300×300 badge anywhere on the canvas set the reference frame for points nowhere near it: in a 1920×1080 outer frame, every painter ≥ 81,000px² then read as full-bleed and went click-through under artwork the user can plainly see. That's the direction the whole null-is-painted fail-safe exists to avoid, so it's a blocker on its own terms.
Fixed in 15d0133 by scoping the walk to roots that contain the point — which is what the docs already promised, so the code moved rather than the prose. Infinity when no root contains the point, which disables the full-bleed rule rather than guessing at a frame.
One design note on the shape of the fix: I used point-containment rather than "nearest root ancestor of the winner", which was the other candidate. A painter that overflows its sub-composition's box (an image scaled up for an animation, say) would otherwise be measured against a frame it visibly escapes, and read as full-bleed at exactly the points where it's most obviously artwork.
Pinned by ignores a sub-composition the point is NOT inside when sizing the frame, which fails if the containment guard is removed — I checked that by reverting it rather than trusting the test's presence.
| ]); | ||
|
|
||
| /** Computed background-color values that put down no ink. */ | ||
| const TRANSPARENT_BACKGROUNDS = new Set(["transparent", "rgba(0, 0, 0, 0)", "rgba(0,0,0,0)"]); |
There was a problem hiding this comment.
🟠 TRANSPARENT_BACKGROUNDS matches by string, so rgba(255, 255, 255, 0), hsla(0, 0%, 0%, 0), and the CSS Color 4 rgb(0 0 0 / 0) slash-notation all read as painting even though they're fully transparent.
Chromium's serializer for background-color preserves the non-black component ordering — rgba(255, 255, 255, 0) comes back through getComputedStyle verbatim, not normalized to rgba(0, 0, 0, 0). So a wrapper styled background: rgba(255, 255, 255, 0) (a common pattern for hover states / opacity-driven fades authored with a non-black base) reads as painting across its full box.
This is on the erring-toward-"paints" side, which per the PR body is the intended safe direction, so it's not blocking — but the docs at docs/sdk/reference/adapters.mdx:109 say:
Ink is a computed-style test — background colour, background image, visible border, the element's own text, or intrinsic media
without the non-black-transparent caveat. Either extend the check to parse alpha out of any rgba(...) / rgb(... /...) / hsla(...) — cheap regex or a tiny helper, and it keeps the set from growing every time a new CSS color-notation lands — or add a one-line docs mention alongside the existing background-image KNOWN OVER-COUNT so a debugger reading the docs knows why a rgba(255,255,255,0) wrapper isn't being ignored. — Rames D Jusso
There was a problem hiding this comment.
Valid — fixed in 15d0133. The set now reads the alpha channel instead of matching spellings:
const inner = /^(?:rgba?|hsla?)\(([^)]*)\)$/.exec(value)?.[1];
const parts = inner.split(/[\s,/]+/).filter(Boolean); // legacy commas and CSS Color 4 slashes
const alpha = parts[3];
return alpha !== undefined && Number.parseFloat(alpha) === 0;Covers rgba(255, 255, 255, 0), hsla(0, 0%, 0%, 0) and the slash notation, and still treats a 3-component rgb() as opaque.
You're right that computed background-color comes back serialized to rgb()/rgba() on the engines we target, so hsla shouldn't reach this in practice — I matched it anyway because the alternation costs one token and removes the dependency on that serialization staying put.
Agreed on severity: this erred toward absorbing clicks, so it was a false positive rather than something that loses artwork. Test cases added for each spelling, plus rgba(…, 0.01) to pin that a faint colour still paints.
| "video", | ||
| "canvas", | ||
| "svg", | ||
| ]); |
There was a problem hiding this comment.
🟡 img in this set is dead code on the point-supplied path — the earlier el instanceof win.HTMLImageElement branch at :545 intercepts and routes to imageAlphaOpaqueAt. It only fires on the point-free callers ("could this paint at all") and on environments where win.HTMLImageElement is missing (e.g. some jsdom variants; the tests supply a stub to work around this at :756). A one-line comment on the set explaining that img is the fallback for the tagRule case would save a future reader the "wait, isn't img handled before this?" trip.
(Non-blocking; feel free to drop.) — Rames D Jusso
There was a problem hiding this comment.
Comment added in 15d0133, stating exactly the two paths you identified — point-free callers, and environments without window.HTMLImageElement:
/**
* Elements that paint by virtue of what they are, whatever their CSS says.
*
* `img` and `picture` look redundant against the branches above them in `elementPaintsInk`,
* and are not: those branches need a `point` to sample alpha against, so a point-free call
* ("could this paint at all?") falls through to this set — as does any environment without
* `window.HTMLImageElement`. The set is also exported, where it reads as a description of
* intrinsic painters rather than a switch in one function.
*/One correction to the framing, for the record: picture is the entry that's genuinely unreachable from inside elementPaintsInk — that branch returns unconditionally, whether or not it finds an inner <img>. I kept it because the set is exported, so for a consumer it reads as a description of intrinsic painters rather than a dispatch table, and a <picture> does paint intrinsically.
compositionFrameArea took the smallest [data-composition-id] in the whole document, so an unrelated sub-composition sized the reference frame for points nowhere near it: a 300x300 badge in a corner made every mid-size painter in a 1920x1080 outer frame read as full-bleed, and the composition went click-through under artwork the user can plainly see. That is the direction the fail-safe exists to avoid, and the docs already described the intended behaviour — the innermost root CONTAINING the point. Also read the alpha channel instead of matching known transparent spellings. Only the `transparent` keyword computes to rgba(0, 0, 0, 0); a faded-out white stays rgba(255, 255, 255, 0), which the set counted as painted. That erred toward absorbing clicks rather than losing them, so it was a false positive rather than a hazard, but it is wrong. Both are pinned by tests that fail when the fix is reverted.
d64a707 to
15d0133
Compare
|
Thanks for the review — all three addressed in 15d0133, replies in each thread. 🔴 Frame reference — real, and broader than nested sub-comps: the two roots need no containment relationship, so a badge in any corner poisoned points across the whole frame, in the unsafe direction. 🟠 Transparent colours — real. Now reads the alpha component of 🟡 Both fixes are pinned by tests I verified by reverting the fix and watching them fail, rather than by their existence:
Also added 536 tests green, typecheck clean, oxlint and |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 15d01339 (delta vs. 46d405e).
All three findings landed cleanly:
-
Blocker (
compositionFrameArea) — resolved per option A.boxContains(rect, x, y)filter added,(x, y)threaded through both call sites,frameArea === Infinityfallback preserved for the no-containing-root case. The rewritten docstring names containment as the load-bearing part and captures the exact failure the old shape produced. Two new tests pin the fix: "ignores a sub-composition the point is NOT inside" (iframe.test.ts:1272) exercises the point-outside-nested-inside-outer case, and "disables the full-bleed rule when no root contains the point" (:1289) covers the no-containing-root branch. Existing "measures full-bleed against the innermost root CONTAINING the point" case renamed to reflect the new semantics. Docs atadapters.mdx:183already matched, no update needed. -
Concern (
TRANSPARENT_BACKGROUNDS) — replaced withisTransparentColorparsing alpha out of anyrgb/rgba/hsl/hslaform (comma + CSS Color 4 slash). New test iterates overrgba(255, 255, 255, 0)/rgba(12, 34, 56, 0.0)/rgb(255 255 255 / 0)/hsla(0, 0%, 0%, 0)/hsl(120 50% 50% / 0)plus the0.01positive control andrgb(1, 2, 3)no-alpha-is-opaque control (iframe.test.ts:1081). Right shape. -
Nit (
INTRINSIC_PAINT_TAGS) — docstring added explainingimg/picturein the set serve the point-free callers and the missing-HTMLImageElementenv. Reads better as documentation of intrinsic painters than a switch in one function.
LGTM from my side. Leaving as a comment.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 15d0133. The earlier thread's findings (frame-area containment, transparent-colour matching, INTRINSIC_PAINT_TAGS docs) are fixed there and not re-raised; everything below is against the current head.
The geometric-walk direction is right and the fail-safe framing ("err toward paints") is the correct contract to design around. The findings below are mostly cases where the implementation lands on the wrong side of that contract — visible artwork reading as no-ink — plus a docs snippet that doesn't compile and a few smaller items.
Blockers (both reproduce with small fixtures):
fullBleedFractionvetoes by border-box area even when ink was just per-pixel verified — a full-bleed transparent-PNG/SVG overlay (the flagship use case) has no correct setting. Inline atiframe.ts:709.- Ink painted by a composition root or by
bodyis structurally invisible to the walk, so a comp whose only ink at the point is its background answers false at fraction 0, where the docs promise every painting box counts. Inline atiframe.ts:657.
Should-fix: the <img> early-return skips the element's own background/border ink (iframe.ts:571), the sort comparator isn't a strict weak ordering so the winner is engine-dependent (iframe.ts:698), the default walk can't see SplitText spans or runtime-mounted sub-composition scenes (iframe.ts:690), a still-loading same-origin iframe returns false instead of the contractual null (iframe.ts:821), and the guide's pass-through recipe calls a function that doesn't exist (canvas-integration.mdx:112).
Smaller items inline: generated-content/box-shadow blindness, the exported compositionPaintsAt's undocumented preconditions, hot-path cost of the per-frame walk, the boolean | null footgun on the interface, consolidation opportunity with studio's hasVisualPresence, and one comment/code contradiction.
PR's own test suite, typecheck, and lint all pass at head — none of the above is caught by the shipped tests because each sits in a case the fixtures don't distinguish.
| if (fullBleed <= 0) return true; | ||
|
|
||
| const frameArea = compositionFrameArea(doc, x, y); | ||
| return frameArea === Infinity || winner.area < fullBleed * frameArea; |
There was a problem hiding this comment.
🔴 The full-bleed veto tests the winner's border-box area even when ink at the point was just per-pixel verified by the alpha sampler, so full-bleed <img>/<svg> overlays read as background over visibly opaque artwork.
Repro: a full-frame transparent PNG lower-third (or full-frame <svg> motion graphic — intrinsic paint) over video. imageAlphaOpaqueAt confirms alpha=255 at the click, but winner.area === frameArea >= 0.9 * frameArea → compositionPaintsAt returns false → host sets pointer-events: none → the visible logo/strokes are unclickable except via the Alt escape hatch. With fraction 0 the same SVG overlay reads painted everywhere instead — there is no correct fullBleedFraction for full-bleed intrinsic media, which is the transparent-overlay case this feature exists for.
Box area ≠ ink area. One shape that fixes it: skip the veto when the winner's ink was established per-pixel (img alpha path) rather than inferred from computed style — the veto exists to discount assumed full-box paint like background-image, and pixel-verified ink is exactly the case where that assumption isn't being made. The scrim test still passes under that rule (its full-coverage paint is style-inferred).
There was a problem hiding this comment.
Confirmed and fixed in 2c65241. This was the worst of the set — it broke the case the feature exists for.
Ink now reports how it was established, and a measured pixel is never vetoed:
type InkKind = "none" | "verified" | "inferred";
...
if (winnerInk === "verified") return true; // box area is not ink areaYour framing is exactly the rule I implemented: the veto exists to discount paint assumed to fill a box, and a sampled pixel is the one case where nothing is being assumed.
One wrinkle worth flagging, because it decides whether the fix is safe. imageAlphaOpaqueAt returns true on six paths where nothing was read — unloaded, no src, rotation/skew, over the 16MP budget, no OffscreenCanvas 2D, tainted. Exempting those from the veto would mean every full-frame CDN-backed overlay (uncorsed, so tainted) absorbs clicks forever, which breaks the feature in the opposite direction just as badly. So the sampler now takes an optional probe: { sampled: boolean } and only sets it where pixels were genuinely read, or where the point provably missed the rendered image. "verified" requires that flag; the fail-safe paths stay "inferred" and still yield to the veto.
Both directions pinned: never vetoes a PIXEL-VERIFIED hit, however large the box and still vetoes a full-bleed image whose pixels could NOT be read.
| y: number, | ||
| ): PaintCandidate | null { | ||
| // Composition roots set the frame reference; they are never candidates themselves. | ||
| if (el.hasAttribute("data-composition-id")) return null; |
There was a problem hiding this comment.
🔴 Ink painted by a composition root — or by body — is structurally invisible. This line excludes every [data-composition-id] element from candidacy even when it paints, and ensureHfIds never stamps body (its walk covers body's children), so at fraction 0, where the docs promise "0 counts every painting box", a comp whose only ink at the point is its background answers false.
Repro: a lower-third block whose authored root div carries data-composition-id plus background:#111 (legal, common block authoring), or a comp with body{background:...} and sparse children — click over the solid visible background → no candidate paints → paintsAt = false → clicks pass through fully painted pixels regardless of options. addressableOnly: false rescues body via "*" but never the excluded roots; no option reaches "roots can paint".
The exclusion exists so roots can serve as frame references, but those are separable concerns: a root could stay out of the frame-reference set while still being ink-tested as a candidate (perhaps exempt from the full-bleed veto, since a root's box is by definition the frame).
There was a problem hiding this comment.
Confirmed and fixed in 2c65241. Roots are ordinary candidates now.
You're right that the two concerns are separable, and once separated the veto turns out to handle roots with no special case at all: a root's box is the frame, so winner.area >= fullBleedFraction * frameArea discounts it for any non-zero fraction, and at fraction 0 its ink counts exactly as the docs promise. I deliberately did not take the "exempt roots from the veto" option you floated — that would make any comp with a root background permanently absorb clicks, which is the pacific behaviour ("clicking the background steps out") inverted.
So the only change was deleting the exclusion; the frame reference already came from its own query after the earlier fix.
Pinned by counts ink painted by the composition root itself, which asserts both halves — true at fraction 0, false at 0.9.
body I've left alone and documented instead: it isn't stamped, so it's invisible under the default, and addressableOnly: false reaches it. Special-casing one unstamped element while the rest of the runtime-mounted content stays invisible felt like the wrong shape — that's the same gap as your :690 comment, and worth solving once.
| return img ? elementPaintsInk(img, win, point) : true; | ||
| } | ||
|
|
||
| if (point && win.HTMLImageElement && el instanceof win.HTMLImageElement) { |
There was a problem hiding this comment.
🟠 With a point supplied, an <img> early-returns through the alpha sampler and never reaches the background/border checks below, so the image's own painted CSS background, letterbox/padding region, and visible border read as no-ink — adapters.mdx promises "visible border" counts.
Repro: <img src="transparent-logo.png" style="object-fit:contain; background:#fff; padding:8px; border:2px solid #000"> rendered as a visible white chip. A transparent PNG pixel over the white plate returns false, and a click on the border/letterbox maps outside the content box → mapPointToImagePixel null → false (the sampler's z-stack heritage, where null meant "miss, keep walking the stack"). If no other candidate paints there, click-through over visibly painted pixels — the same background/border on any non-img element would count.
Fix shape: for images, alpha-opaque || background/border ink rather than early return.
There was a problem hiding this comment.
Confirmed and fixed in 2c65241, with the shape you suggested — alpha-opaque || style ink rather than an early return:
if (imageAlphaOpaqueAt(el, point.x, point.y, win, probe)) {
return probe.sampled ? "verified" : "inferred";
}
// A clear pixel does not settle the element: its own background plate, padding and
// border still paint, and the point may have landed on them rather than the bitmap.
return styleInk(el, win);The letterbox half of your repro is the part I'd have missed reasoning about it abstractly — mapPointToImagePixel returning null is a *geometric_ miss on the bitmap, which the z-stack caller correctly read as "keep walking", but for a paint query it says nothing about the plate underneath. Now it falls through to the same background/border/own-text tests any other element gets.
Pinned by a clear pixel still paints when the image itself has background or border ink.
| }); | ||
|
|
||
| // Smallest area first; on a near-tie prefer the deeper (more specific) element. | ||
| candidates.sort((a, b) => |
There was a problem hiding this comment.
🟠 This comparator isn't a strict weak ordering — the ≤0.5px² tie relation is intransitive (areas 100.0 ~ 100.4 and 100.4 ~ 100.8 but 100.0 ≁ 100.8) — so Array.prototype.sort's output is implementation-defined and "smallest painting box first" isn't guaranteed. A three-candidate fixture with areas [100.9, 100.5, 100.4] sorts big-first under V8.
Consequences: with fullBleedFraction > 0 and near-tied areas straddling the threshold, the same document and point can flip between painted/background across engines or input orderings, and the lazy-alpha "smallest box sampled first" guarantee (pinned by the counting-canvas test) silently degrades.
Since the winner is only ever consumed as winner.area and the depth tie-break is otherwise unobservable, sorting by area alone is simpler and correct — or make the tie-break transitive by bucketing areas before comparing.
There was a problem hiding this comment.
Confirmed and fixed in 2c65241 — comparing exactly, with depth as the tie-break:
candidates.sort((a, b) => a.area - b.area || b.depth - a.depth);I kept the depth tie-break rather than dropping to area-only. You're right that it's unobservable through winner.area, but it's observable in which element gets ink-tested first, and on an exact tie (a wrapper and its child with identical rects) the deeper one is the likelier painter — so it keeps the single-sample property pointed at the right node. Exact - comparison makes it a proper ordering; the intransitivity was entirely in the epsilon.
| y: number, | ||
| opts?: PaintsAtOptions, | ||
| ): boolean { | ||
| const selector = (opts?.addressableOnly ?? true) ? "[data-hf-id]" : "*"; |
There was a problem hiding this comment.
🟠 The default addressableOnly: true walk sees only [data-hf-id] elements, but the framework's own runtime output is unstamped: SplitText word/char spans (splitting also empties the stamped parent's direct text nodes, so hasOwnText goes false), clones, and entire runtime-mounted sub-composition scenes — registry block files don't carry data-hf-id, compositionLoader injects fetched content as-authored, and stamping runs only on the document the SDK opened.
So kinetic typography — canonical transparent-overlay content — reads as no-ink under defaults: the caption's stamped parent has no own text after splitting and the visible word spans aren't candidates → clicks pass through visible glyphs. A registry lower-third mounted via data-composition-src is worse: zero stamped elements, host div excluded as a root → the whole visible scene reports false.
Minimum: the PaintsAtOptions docs and the overlay recipe should say the default fails these (currently only "split-text word spans, cloned nodes" is disclosed, not whole sub-composition scenes). Better: attribute unstamped-descendant geometry to the stamped ancestor, or stamp runtime-mounted content.
There was a problem hiding this comment.
Taking the minimum now and filing the rest. PaintsAtOptions in adapters.mdx now names all three classes of invisible content rather than just the two:
Stamping happens once, on the document
openCompositionwas given, so anything the runtime creates or fetches afterwards is invisible to the default walk: split-text word and character spans (splitting also empties the stamped parent's own text nodes, so the parent stops counting too), cloned nodes, and whole sub-composition scenes mounted fromdata-composition-src. Kinetic typography and registry-mounted lower-thirds — both canonical transparent-overlay content — therefore read as no-ink by default.
The parenthetical is the part I hadn't understood until your comment: splitting doesn't just add unstamped children, it empties the stamped parent, so hasOwnText flips false and the one candidate that could have covered the glyphs stops painting. That makes kinetic typography a total miss rather than a partial one.
On the real fix — attributing unstamped-descendant geometry to the stamped ancestor is the one I'd pick over stamping runtime content, since stamping mutates a document the runtime owns and would race GSAP's own DOM writes. Both are more than a docs change, so I'd rather do it deliberately than bolt it onto this PR. Happy to file it, or leave it to you given you're closer to the registry-mount path.
| ): boolean { | ||
| const selector = (opts?.addressableOnly ?? true) ? "[data-hf-id]" : "*"; | ||
| const candidates: PaintCandidate[] = []; | ||
| doc.querySelectorAll(selector).forEach((el) => { |
There was a problem hiding this comment.
🟡 Hot-path cost: each call is an uncached O(all stamped elements) walk — querySelectorAll + getBoundingClientRect per element (ensureHfIds stamps every body descendant, so addressableOnly shrinks nothing), up to three getComputedStyle resolutions per contained candidate (isRenderedVisible, isOpacityVisible's inclusive re-read, elementPaintsInk), an eager full ancestor opacity walk plus a second depthOf walk per candidate, and a fresh root query per painted hit when fullBleedFraction > 0. Under the documented recipe (re-run per mousemove AND per frame) a ~1000-element comp pays ~100k+ rect/style reads per second, forcing layout flushes interleaved with GSAP's writes — janking the animation the overlay sits on.
Cheap wins that don't change semantics: read computed style once per candidate and thread it through; defer the visibility check into the lazy find predicate beside the ink test; single-pass min-tracking instead of a full sort; cache frame area per document. And the docs recipe should coalesce to one gated rAF query (pointer moved || playhead advanced || Alt changed) instead of mousemove + per-frame.
There was a problem hiding this comment.
Accepted as real, and I've taken the docs half now: the recipe's Step 2 says to coalesce pointer / Alt / playhead into one gated rAF query and to short-circuit before the walk when the pointer is outside the composition's box. The consumer already does exactly that — its evaluate bails before the query unless the pointer is inside and Alt is up, which is what keeps the per-frame trigger affordable — so the guide was under-describing the shape that makes it viable.
The code-side wins you list are all real and all semantics-preserving, and I'd rather do them as a measured change than by eye: one computed style per candidate threaded through, visibility deferred into the lazy predicate, single-pass min instead of a full sort, frame area cached per document. Two of them interact with the laziness property the counting-canvas test pins, so I want a benchmark on a ~1000-element comp before and after rather than trusting the reasoning — which is more than belongs in this PR.
One correction to the estimate: addressableOnly does shrink the walk in practice, just not by much. ensureHfIds skips script, style, template, meta, link, noscript and base, so "*" picks up those plus html/head/body. Rounding error against your point, which stands.
Happy to file it with your list as the description.
| * Transparency by CSS (`display: none`, `visibility: hidden`, `opacity: 0`) is NOT | ||
| * handled here; `compositionPaintsAt` filters those before asking. | ||
| */ | ||
| export function elementPaintsInk( |
There was a problem hiding this comment.
🟡 Consolidation note: this is now the repo's second independently-maintained "does this element visually paint" heuristic — studio ships hasVisualPresence + VISUAL_LEAF_TAGS (packages/studio/src/components/editor/domEditingElement.ts:29) with the same computed-style ink test, and the transparent-background logic is also duplicated in studio's captions parser and cli's layout-audit. They already disagree (studio counts box-shadow and audio; sdk counts own text and per-pixel img alpha) and will drift. Dependency direction (studio → sdk) means this PR couldn't import the studio copy, so the right follow-up is the reverse: make this exported elementPaintsInk the shared home and have studio delegate to it.
There was a problem hiding this comment.
Agreed, and agreed on the direction — sdk as the shared home, studio delegating, since the dependency arrow already points that way.
Worth recording that the two aren't a straight merge: studio's hasVisualPresence counts box-shadow and audio, this one counts own-text and per-pixel <img> alpha, and they're answering subtly different questions — "is this element worth showing as an editable leaf" versus "is there ink at this exact point". The per-point half has no meaning for studio's use, and the audio entry has none for this one. So the shared core is probably styleInk plus the transparent-colour parse, with each caller keeping its own policy on top, rather than one predicate serving both.
The transparent-colour duplication you note in studio's captions parser and cli's layout-audit is the easiest first slice — that's a pure function with no policy in it at all.
Out of scope here, but I'll file it with that shape unless you'd rather own it.
| */ | ||
| function isRenderedVisible(el: Element, win: Window & typeof globalThis): boolean { | ||
| const cs = win.getComputedStyle(el); | ||
| if (cs.display === "none" || cs.visibility === "hidden") return false; |
There was a problem hiding this comment.
🟡 Nit: the comment above says display: none "needs no check here — it collapses the box, which the area guard already rejects", and then this line checks it anyway. The branch is dead in real DOMs (display:none rects are 0×0) and live only under the test fake that grants such nodes a 100×100 box — so the test iteration pins a DOM state that can't occur. Either drop the clause and that test case, or rewrite the comment to declare deliberate redundancy.
There was a problem hiding this comment.
Fixed in 2c65241 by rewriting the comment — I kept the clause. Dropping a guard to make a comment true is the wrong direction when the guard is free and the function is exported for hosts to call with whatever DOM-alike they have.
display: noneis checked belt-and-braces: a real engine collapses the box and the area guard already rejects it, so the clause only earns its place against hosts that report a box anyway.
That also makes the test case honest about what it's pinning — the fake is exactly such a host.
| * nodes — those still paint, and reporting no ink over visible artwork is the failure | ||
| * this fail-safe direction exists to avoid. | ||
| */ | ||
| paintsAt?(x: number, y: number, opts?: PaintsAtOptions): boolean | null; |
There was a problem hiding this comment.
🟡 The null-means-painted convention is documented in three places, but the shape still hands callers three falsy bottom values with opposite safe readings: false (no ink → pass through), null (unknowable → treat as painted), and undefined from an adapter that doesn't implement the optional method (also treat as painted). The idiomatic if (!preview.paintsAt?.(x, y)) passThrough() does the dangerous thing for two of the three. This PR itself shows the convention needs active defense — the headless adapter needed a dedicated comment plus a pinning test to keep null from regressing to false.
Worth considering before the AI Studio consumer locks the shape in (per the coordination note in the earlier review): non-optional with a safe default implementation, or inverted polarity (isProvablyEmptyAt — true only when safe to pass through, so every bottom value fails safe).
There was a problem hiding this comment.
This is the sharpest observation in the review and I don't have a fix I'm confident in, so I'd rather flag it than pick wrong under time pressure.
You're right about the failure mode: if (!preview.paintsAt?.(x, y)) passThrough() is what someone writes, and it does the dangerous thing for two of the three bottom values. The evidence you cite is fair too — the headless adapter needed a dedicated comment plus a test precisely to stop null regressing to false, which is a smell about the shape rather than about that adapter.
On the two options:
- Non-optional with a safe default removes
undefinedbut not thefalse/nullconfusion, and it forces every existingPreviewAdapterimplementor to change — including hosts outside this repo. isProvablyEmptyAt(true only when it is safe to pass through) is the one I find convincing: every bottom value —false,null,undefined— then fails safe, and the name states the burden of proof instead of leaving it to a convention documented in three places. Cost is that the "does it paint" reading, which is how everyone will describe it out loud, becomes the negation.
The coordination point is the real constraint: AI Studio is the first caller and is already running this branch locally, so the shape does lock in on release. I'd rather settle it before merge than after. Do you want to make the call here, or should I open it as a short design thread so @james-russo-rames-d-jusso can weigh in too? I'm happy to do the rename in this PR if you land on the inverted polarity — it's mechanical, and the tests pin the semantics either way.
There was a problem hiding this comment.
Take the rename. isProvablyEmptyAt — true only when it is safe to pass the click through — makes every bottom value fail safe by construction: false, null-free, and undefined from an adapter without the method all land on "keep the composition clickable". The tri-state collapses to a plain boolean, and the null-means-painted prose in the interface docstring, adapters.mdx, and the guide recipe stops being load-bearing (keep it as rationale if you like, but nothing depends on readers remembering it). The naming reads less naturally than paintsAt, but a shape the compiler enforces beats a convention three documents defend — and it's nearly free now versus locked once AI Studio ships against it. Worth a one-liner in the PRINFRA-397 PR pointing at the new polarity so the consumer branch is written against it from the start.
There was a problem hiding this comment.
Renamed in 95e2e52 — thanks for making the call rather than leaving it to a follow-up.
isProvablyEmptyAt?(x: number, y: number, opts?: PaintQueryOptions): boolean;True only when the composition was readable and nothing painted there. Ink, an unreadable or still-loading document, and an adapter without the method all come back falsy, so the call site is safe by construction:
if (preview.isProvablyEmptyAt?.(x, y)) passThrough();Three things fell out of it that are worth recording:
- The headless adapter stopped needing its argument. It used to return
nullwith a comment and a pinning test defending "null is not false"; now it returns plainfalse— an adapter with no surface simply cannot prove emptiness. The test remains, but it pins something obvious rather than something counter-intuitive, which is the tell that the shape got better. PaintsAtOptions→PaintQueryOptions. The old name pointed at a method that no longer exists, and the type is shared with the exportedcompositionPaintsAtanyway — it describes the walk, not one method's arguments.compositionPaintsAtkeeps its ink-positive name. It answers the other question and genuinely has no "not knowable" channel; its docstring now sends callers who want the fail-safe contract to the adapter instead. Two names with opposite polarity sitting next to each other looked like a smell until I tried collapsing them — the primitive really is computing "does it paint", and renaming it would have made it lie.
The prose survives as rationale in the interface docstring, adapters.mdx and the guide recipe, but as you say nothing depends on a reader remembering it now. The guide's Step 3 became "let the polarity do the work", with an explicit warning against re-inverting it into a painted variable.
Consumer side: the PRINFRA-397 branch is already written against the new polarity — shouldPassPointerThrough now takes provablyEmpty rather than painted: boolean | null, so the burden of proof stays on the same side end to end, and its unit tests moved with it. Nothing was released under the old name.
| const wrapper = document.querySelector<HTMLElement>("#composition-wrapper")!; | ||
|
|
||
| function updatePassThrough(clientX: number, clientY: number, altKey: boolean) { | ||
| const point = clientToCompositionCoords(clientX, clientY); |
There was a problem hiding this comment.
🟠 clientToCompositionCoords is defined nowhere in the guide or the repo — the snippet doesn't compile as copied. And the mapping it hand-waves is load-bearing: paintsAt expects the iframe's internal client coordinates, so a host substituting raw host-page clientX/clientY (or scaling to the declared design size while the iframe renders at another internal size) samples the wrong point and toggles pass-through over the wrong pixels. This is the only snippet in the guide that listens on the host document, so it can't lean on surrounding context.
Two fixes: inline a real implementation (iframe getBoundingClientRect + scale), and state where the listener must attach — attached to the wrapper or iframe document, the first pointer-events: none flip starves the listener and the pass-through state sticks.
There was a problem hiding this comment.
Both fixed in 2c65241. toCompositionPoint is now a real implementation in the snippet — iframe getBoundingClientRect over the composition's native size, with the scale divided out — since as you say the mapping is load-bearing and silently samples the wrong pixel when it's wrong.
The listener placement is now a <Warning> rather than prose, because it's the failure that looks like a bug in paintsAt rather than in the host:
Listen on the host document, not on the wrapper or the iframe. The first time this sets
pointer-events: nonethe wrapper stops receiving events, so a listener attached there can never turn it back on — the pass-through state sticks.
I also folded your rAF point from the perf thread into Step 2 of the recipe: coalesce pointer / Alt / playhead into one gated query, and short-circuit before the walk when the pointer is outside the composition's box.
Four cases where the walk landed on the wrong side of its own fail-safe. The full-bleed veto tested the winner's border-box area even when the alpha sampler had just read an opaque pixel there, so a full-frame transparent PNG or SVG overlay — the case this feature exists for — reported background over visibly opaque artwork, with no fullBleedFraction that worked. Ink now carries how it was established, and a measured pixel is never vetoed. An image whose pixels could NOT be read stays inferred, so a tainted CDN overlay still yields to the veto rather than absorbing every click. Composition roots were excluded from candidacy outright, so a root carrying a background answered false even at fraction 0, where the docs promise every painting box counts. Roots are candidates now; the veto discounts them without a special case, since a root's box is the frame. An <img> with a clear pixel early-returned past its own background, padding plate and border, which any other element would have counted. A same-origin iframe mid-navigation exposes a readable but empty document, so the !doc guard never fired and a loading composition answered a confident "no ink" — the exact failure the null convention exists to prevent. Also: the sort comparator's epsilon tie was intransitive, leaving the smallest-first guarantee (and the lazy single-sample property that rides on it) engine-dependent; the guide's pass-through recipe called a function that does not exist and hand-waved the coordinate mapping that makes it correct; and the reference now states the under-counts alongside the over-counts, the walk's blindness to runtime-mounted content, and compositionPaintsAt's preconditions. Each fix is pinned by a test that fails when the fix is reverted.
|
Thanks @vanceingalls — this was a genuinely load-bearing review. Every item checked out against the code; nothing here was a false positive. All fixes in 2c65241, replies in each thread. Both blockers were real, and the first was the worst bug in the PR. A full-frame transparent PNG/SVG overlay — the exact shape this feature exists for — had its measured opaque pixels vetoed by border-box area, with no Composition roots are candidates now; the veto discounts them with no special case, since a root's box is the frame. I skipped the "exempt roots from the veto" variant — it inverts the background-steps-out behaviour the consumer depends on. Also fixed: the Documented rather than coded, each with reasoning in-thread: generated-content and Deferred with a shape, not a shrug: the hot-path work (semantics-preserving but interacts with the pinned laziness property — wants a benchmark, not eyeballing), and the studio One open question I'd like settled before merge — the 541 tests green (+5 this round), each fix pinned by a test verified to fail when the fix is reverted. Typecheck, oxlint, docs-snippet check and |
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed at 2c65241a. Every finding from the last round is addressed and each fix is pinned by a test — verified the fixes in the diff, not just the replies. The InkKind split with the probe.sampled distinction is the right mechanism: verified pixels are never vetoed while the fail-safe opaque paths still are, and both directions are pinned. Roots-as-candidates, the strict-weak-ordering comparator, the loading-iframe null, and the guide's real toCompositionPoint all check out. The documented-not-coded calls (under-counts, addressableOnly gaps, compositionPaintsAt preconditions) are the right altitude, and deferring the perf pass behind a benchmark is correct given the pinned laziness property.
Approving with the polarity question answered in-thread: take the isProvablyEmptyAt rename — happy for it to land under this approval. Deferred follow-ups (perf benchmark, studio hasVisualPresence consolidation) are worth filing as issues so they don't evaporate.
paintsAt handed callers three falsy bottom values with opposite safe readings: false meant "no ink, pass the click through", null meant "not knowable, treat as painted", and undefined from an adapter without the method also meant painted. The idiomatic `if (!preview.paintsAt?.(x, y)) passThrough()` therefore did the dangerous thing for two of the three, and the convention needed defending in the interface docstring, the reference and the guide — plus a dedicated comment and a pinning test on the headless adapter to stop null regressing to false. Inverting the polarity collapses the tri-state to a plain boolean and makes the safe reading structural: true only when the composition was readable and nothing painted there, so ink, an unreadable or still-loading document, and a missing implementation all land on "keep the composition clickable". The prose stays as rationale, but nothing depends on a reader remembering it. PaintsAtOptions becomes PaintQueryOptions, since it now describes the walk that both the adapter method and the exported compositionPaintsAt share rather than one method's arguments. compositionPaintsAt keeps its ink-positive name: it answers the other question, and its docstring points callers who need the fail-safe contract at the adapter. Nothing is released yet, so no consumer is on the old name.
|
Thanks both — @vanceingalls for the approval and the polarity call, @james-russo-rames-d-jusso for the earlier pass. Rename landed in 95e2e52, under the approval as offered. The clearest sign it was the right call: the headless adapter no longer needs its explanatory comment. It used to return Follow-ups filed as asked, both with the shape rather than just a pointer:
Consumer already written against the new polarity. The PRINFRA-397 branch takes 541 tests green, typecheck, oxlint, docs-snippet check and |
What
Adds a paint query to the preview adapter:
and exposes the pieces it is built from —
elementPaintsInk,compositionPaintsAt,imageAlphaOpaqueAt, alongside the existingalphaIsOpaque/mapPointToImagePixel— on a new@hyperframes/sdk/adapters/iframeexport path.Why
Any host that layers a transparent composition over other content has to answer one question
before it swallows a click: is the user pointing at artwork, or through an empty gap at whatever
sits beneath? Geometry alone can't tell — a composition is mostly full-bleed wrapper
<div>s withno visual presence, and those boxes cover every pixel of the frame.
PreviewAdapteronly answered what element is at this point, so HeyGen AI Studio wrote its owncomputed-style heuristic. It could not reuse the per-pixel
<img>alpha this adapter alreadysamples for
elementAtPoint:imageAlphaOpaqueAtwas private, andadapters/iframehad no packageexport path, so even the module-level helpers were unreachable. Every host layering a transparent
composition needs this, and today each one reinvents it invisibly to the others.
How
A geometric box walk, deliberately not
elementsFromPoint. The z-stack omitspointer-events: nonenodes — and a decorative overlay carrying that still paints. Built on thestack, the query would confidently report "no ink" while the user points straight at visible
artwork, which is the direction that makes a composition vanish from under the cursor. The paint
walk is therefore blind to
pointer-eventsandz-indexby design.<img>routes through the existing alpha sampler (and<picture>defers to the<img>itwraps, rather than painting unconditionally), so a transparent PNG paints only where its pixels do.
Everything else stays a computed-style test: background colour, background image, visible border,
the element's own direct-child text, or intrinsic media.
Ink is evaluated lazily down candidates ordered smallest-box-first, so a hit costs one
getImageDatarather than one per candidate. There's a regression test asserting exactly one pixelread with two candidate images — that cost is silent otherwise.
fullBleedFractionis an option, not a constant. "A layer covering ~the whole frame isbackground, not artwork" is host policy — an editor's step-out rule — not a fact about ink.
Default
0; an editor passes ~0.9.addressableOnly(defaulttrue) scopes the walk to[data-hf-id];falsewidens it to nodes created after stamping (split-text spans, clones).nullmeans not knowable (document unloaded or unreadable) and callers must treat it aspainted: erring toward "paints" costs a click that selects the composition, erring the other way
makes the composition unreachable. The headless adapter returns
null, notfalse.Known limits kept deliberately and documented in the code:
background-imagealpha remains anover-count (narrowing it needs a second sampling path — resolve the
url(), map throughbackground-size/-position), and<video>/<svg>/<canvas>stay unconditionally opaque.Test plan
Unit tests added/updated
26 new cases in
iframe.test.tsplusheadless.test.ts: the ink heuristic;<img>alpha(opaque, transparent, tainted, unloaded,
object-fitletterbox);<picture>deferral;fullBleedFractionon and off; the innermost-root frame reference for nested sub-compositions;an ancestor
opacity: 0; thenullpaths; and the lazy-sampling guard. Two of the subtlest weremutation-checked — reverting to element-only opacity and to eager ink evaluation each fail their
test.
bun run --cwd packages/sdk test→ 533 passing.typecheckclean. Repolintgate and thefallowaudit both green.Manual testing performed
Verified live in HeyGen AI Studio against a local build of this branch: empty regions of a
transparent composition pass the pointer through to the avatar beneath, painted regions absorb it,
Alt-click always takes the composition, and press-and-drag through a gap moves the avatar in one
gesture — the case that matters, since a re-implemented hit test gets selection right and drops
the drag.
Documentation updated
docs/sdk/reference/adapters.mdx(interface,paintsAt,PaintsAtOptions, export map, headlessnote) and a transparent-overlay recipe in
docs/sdk/guides/canvas-integration.mdxcovering thethree things that are easy to get wrong: decide before the press, re-evaluate per frame, and treat
nullas painted.Consumer side: HeyGen AI Studio deletes its own heuristic and calls
paintsAtonce this releases(pacific, tracked in PRINFRA-397).