fix(cli): avoid multiline overlap false positives#2468
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 18cd90a5b00fc839f0c7308b47b88bc2e1281070.
What this does
Fixes a false-positive content_overlap finding for multiline (typically rotated) text blocks where the audit's union bounding rect covers the empty gap between text lines and another label happens to sit only in that gap.
Concretely, contentOverlapIssues was measuring intersectionArea(a.rect, b.rect) where both rects were the union rectangles returned by textRectFor. overlapIssue at layout-audit.browser.js:646-647 now measures fragmentIntersectionArea(a.rects, b.rects) — sum of pairwise intersection areas across the individual Range.getClientRects() fragments (one per rendered text line) — and the 20% threshold is applied against rectsArea(a.rects) (sum of fragment areas) instead of rectArea(a.rect) (union area).
The union rect is still stored on each block and still surfaced in the finding's rect: field (layout-audit.browser.js:645) so diagnostics remain readable.
Regression at layout-audit.browser.test.ts:864-877 pins a two-line multiline block (line 1 at top=100,h=60; line 2 at top=260,h=60) with a 240×50 block positioned entirely at top=180-230 in the empty gap → no content_overlap. Real-browser check-fixture evidence in the PR body: 9 persistent samples → 0.
Verification
- Single-fragment case is byte-identical. For a block with one text-line fragment,
textClientRectsreturns[singleRect],rectsAreadegenerates torectArea(singleRect), andfragmentIntersectionAreadegenerates tointersectionArea(a[0], b[0]). So the fix only changes behavior for genuinely multi-fragment (rotated / wrapped) text. ✓ - Threshold direction is correct for the fix scenario. A block sitting only in the vertical gap between line-fragments intersects zero fragments of the multiline block → numerator collapses to 0 → threshold not met → no finding. ✓
- Threshold direction for real cross-line overlap.
rectsArea(sum of per-line AABBs) is ≤ union area for multiline, so the denominator shrinks slightly — i.e. the audit is more sensitive to overlaps that actually touch a text line. Aligns with "did a block cover real text" intent. ✓ textClientRectsfilter for tiny rects (width > 0.5 && height > 0.5at:231-233) guards against zero-area range-rect noise. Same filter pre-PR, unchanged. ✓collectSolidTextBlocksguard — only pushes blocks wheretextRectFor(element, true)returns non-null, soa.rectsis always non-empty andrectsAreadoesn't divide/threshold against zero. ✓- Test mock aligned.
getClientRectsspy atlayout-audit.browser.test.ts:1350-1360now returns the array of rects (instead of the previous single-element wrapper), matching whatRange.getClientRects()returns for multiline text in a real browser.getBoundingClientRectspy at:1342-1345uses the newboundingTextRecthelper to compute the AABB of the fragment array — same shapetextRectForwould compute. Consistent mock surface. ✓ - CI: 30 SUCCESS + skips at head. Windows shards + Producer integration all green. ✓
Concerns
-
🟢 Rotated multi-line AABB overlap between fragments. For heavily-rotated multi-line text, adjacent per-line
getClientRectsAABBs partly overlap each other in the direction of rotation (the diamond-AABBs of two adjacent 45°-rotated lines share a corner). Consequences:rectsAreaslightly over-counts actual text visual area (denominator up → threshold looser);fragmentIntersectionAreadouble-counts a third block that overlaps the shared-AABB region (numerator up → threshold reached sooner). Nets in the "flag more overlaps" direction, which is safe for the audit's role. Note-only, no fix suggested. -
🟢 The multi-frag arm is untested for cross-line real overlap. The new regression pins the gap-only case (no fragment intersection). A block that genuinely covers ≥20% of one line's fragment but 0% of the other would be a good complement — proves the threshold-against-sum-of-fragment-areas math didn't accidentally raise the bar for real overlaps. Non-blocking.
Verdict framing
Small, well-scoped fix with a clean pairwise-fragment formulation, backwards-identical for single-fragment blocks, and real-browser evidence. LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
Position: RIGHT direction — the fix mechanism is structurally sound (measure intersection on per-fragment geometry, preserve union rect for diagnostics), not the coarser "exclude multiline" shape. Blocking on one extrapolation to the sibling detector that carries the same multiline blind spot.
Additive to Rames — his review pins the byte-identical single-fragment degeneration and the threshold-direction analysis; I converge on the local fix and won't duplicate that lens. My substantive divergence is scope: the SAME union-rect blind spot lives in occlusionCoverage one screen away and produces the analog false positive. Rames's "no positive-hit test for cross-line real overlap" nit matches my Important below — belt-and-suspenders on the same ask.
Strengths
packages/cli/src/commands/layout-audit.browser.js:583-585—collectSolidTextBlockscollectsrectsalongsiderect, so the emitted issue (rect: a.rectat:663) stays backward-compatible with the union-bbox shape downstream consumers already read.- Real-fixture verification (9-sample
check→ 0 findings) is empirically load-bearing, not hypothetical.
Blockers
extrapolation-blocker — same multiline blind spot in occlusionCoverage
File: packages/cli/src/commands/layout-audit.browser.js:950-963 (occlusionCoverage), consumed by occludedTextIssue at :996-1018.
The bug this PR fixes lives verbatim in the occlusion detector. occlusionCoverage(element, textRect) samples a 3×9 grid inside the union textRect:
const y = textRect.top + textRect.height * yFraction; // yFractions = [0.25, 0.5, 0.75]
const hit = occluderAt(element, textRect.left + textRect.width * xFraction, y);For a rotated / wrapped multiline text block (the same shape this PR is fixing), the union textRect spans across the inter-line gap. The y=0.5 probe row lands squarely in that gap. Any opaque decorative element sitting between the rendered lines — badge, frame, icon — hits elementFromPoint at 9 probe points → 9/27 = 33% coverage. PROSE_COVERAGE_FLOOR = 0.15 at :938 clears with headroom, yielding a false text_occluded finding on the same multiline shape the current PR just fixed for content_overlap.
Under the extrapolation-blocker rule you pushed in #hyperframes-work 2026-07-14 ("if we see a fix that can be extrapolated to other areas we should propose that fix as requested changes ... not merging prs that don't solve root issues just for merging them"), this is the sibling site the fix generalizes to:
- Grid probes belong inside per-fragment rects, not the union bbox.
- Distribute probes across fragments (weighted by fragment area), aggregate
hits / gridPointsUsed— same denominator shift as this PR'srectsArea(a.rects)replacingrectArea(a.rect). - Add a regression: two-fragment text with an occluder placed only in the inter-line gap must not produce
text_occluded.
Scope note on the rest of the file — I checked every textRectFor consumer:
textOverflowIssues:439-500,panelOutOfCanvasIssues:1158-1160, geometry candidates at:1362— text-vs-container overflow, union bbox is semantically correct (any painted text outside the container is still overflow, even across a gap).invisibleTextIssue:1032— existence gate + emitted diagnostic, no spatial comparison.occlusionCoverage:950— the one true sibling. Same bug family.
Important
packages/cli/src/commands/layout-audit.browser.test.ts:864-876covers only the negative-hit case. Add a positive-hit parameterization for the same two-fragment block: label placed to overlap the second fragment ≥20% of the smaller-fragment area must tripcontent_overlap. Confirms the new ratio math still fires when the collision is real. (Same "parameterize across each branch of the conditional" discipline you pushed on #2443/#2454 R1; Rames flagged the same gap in his second green note.)
Nits
fragmentIntersectionArea/rectsAreaat:600-610: one-liner comment noting the ratio denominator issum(fragmentArea)rather thanunion.areaon purpose — sum-of-fragments < union when gaps exist, which makes the 20% threshold more sensitive to real collisions, not less. Non-obvious on read; matches Rames's "threshold direction" analysis.
Envelope
Clean — no Co-Authored-By: Claude trailer on the commit; Compound Engineering + GPT-5 badges are your standard PR-body style, not the CC-default footer.
Verdict: REQUEST CHANGES (extrapolation to occlusionCoverage:950)
Reasoning: Fix is right; the sibling detector carries the same union-rect blind spot in ~5 lines of parallel code, and the rule you pushed 2026-07-14 makes that request-changes-tier rather than follow-up-tier.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 7cacbe8c13756514d6d3ced318a2bc03ae71cd92 — R1 (18cd90a5) → R2 delta is +50 / -14 across the same two files.
R2 delta
Closes Via's extrapolation blocker by applying the R1 union-rect → per-fragment refactor symmetrically to the text_occluded occlusion check. Structural mirror of R1:
| Path | R1 (overlap) | R2 (occlusion) |
|---|---|---|
| Scans against | fragmentIntersectionArea(a.rects, b.rects) |
Per-fragment occlusionCoverage(element, textRects) grid |
| Threshold denominator | rectsArea (sum of fragment areas) |
OCCLUSION_GRID_POINTS * textRects.length (sum of sample counts) |
| Backwards-compat for single-fragment blocks | Byte-identical | Byte-identical |
| Diagnostic union rect | still stored on block | still returned by textRectFor |
Key changes:
occlusionCoverage(element, textRects)atlayout-audit.browser.js:952-971: outerfor (const textRect of textRects)loop wraps the existing 3×9 y/x-fraction grid.coveredFractionnormalizes hits againstOCCLUSION_GRID_POINTS * textRects.length— a scene-wide 0-1 fraction that stays comparable to pre-PR values for single-fragment blocks.- Call site in
analyzeElementOcclusionat:1002-1012:textClientRects(element, true)collected alongside the existingtextRectForunion rect; passed viatextRects.length > 0 ? textRects : [textRect]to preserve pre-PR behavior on edge cases. - Regression at
layout-audit.browser.test.ts:1507-1531: two 40-px-high line fragments aty=500-540andy=580-620, opaque overlay aty=540-580(the gap).elementFromPointmock returnsoverlayfor540 < y < 580andheadlineelsewhere. WithOCCLUSION_PROBE_Y_FRACTIONS = [0.25, 0.5, 0.75], per-fragment y-samples land at offsets 10/20/30 within each 40-px fragment → line 1 samples at 510/520/530, line 2 at 590/600/610. None reach the gap;coveredFraction = 0;PROSE_COVERAGE_FLOOR = 0.15gate skipped; notext_occludedfinding. ✓
Verification
- Single-fragment behavior is byte-identical. For
textRects.length === 1, the outer loop runs once with the sole fragment astextRectand denominator =OCCLUSION_GRID_POINTS * 1= old denominator. Every pre-PR occlusion assertion holds. ✓ - Multi-fragment behavior direction is correct for both scenarios.
- Gap-only overlay: samples never reach the gap → 0 hits → 0 coverage → no finding. Fixes the false positive.
- Real overlay covering one full line of a 2-line block: 27 hits on that line, 0 hits on the other → coverage = 27/54 = 0.5 → clears
PROSE_COVERAGE_FLOOR→ finding fires. Sensitivity to real overlays is preserved. ✓
- Sample-fraction resilience. Y-fractions are
[0.25, 0.5, 0.75]. For a fragment attop=T, height=H, samples are atT + 0.25H, T + 0.5H, T + 0.75H— always well within the fragment, no leakage into inter-line gaps regardless of fragment count. ✓ occluderAtstill filters the block itself via theelementarg, so a fragment's own text pixels don't count as occluders. Unchanged from R1. ✓- Test helper
installOcclusionGeometryat:1832-1901: signature widened toDOMRect | DOMRect[],getClientRectsmock normalizes vianormalizeTextRects(introduced in R1). Every existing test call passes a singlerect(...)→ normalized to[rect]→ identical behavior. Only the new gap test passes an array. ✓ - CI progressing. 10 SUCCESS + 20 pending at read time, no FAILURES. Layout-audit suite 71/71 in Miguel's local run.
Concerns
-
🟢
textClientRectsis called twice per element.analyzeElementOcclusioncallstextRectFor(element, true)(which internally callstextClientRects) and then callstextClientRects(element, true)again a line later. Two range creations +getClientRectsinvocations for the same subject. Not measurable in profiles at typical text-block counts, but atextFragmentsAndUnion(element, true)helper returning{ union, fragments }would remove the redundancy for future callers too. Note-only. -
🟢 The
textRects.length > 0 ? textRects : [textRect]fallback is defensively unreachable.textRectForreturned non-null only iftextClientRectsreturned non-empty (union is derived from fragments). So the[textRect]branch can't fire. Harmless — reads as belt-and-suspenders — but if simplified intotextRectsalone, the invariant becomes explicit and the code shrinks by one branch. Note-only. -
🟢 Missing complementary cross-line real-overlap regression. Same note I flagged for the R1 overlap side: the multi-fragment arm is now well-pinned for the gap-only case, but there's no regression that a block genuinely covering one full line of a two-line block still fires
text_occluded. The math math checks out (I traced the sample yield above), but a test would prove the sensitivity direction under future refactors. Non-blocking, worth as a follow-up commit.
Verdict framing
Clean R2 — symmetric extension of R1's fix pattern, backwards-identical for single-fragment blocks, and the regression pins the exact gap-in-multiline case Via extrapolated to. LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 7cacbe8c13756514d6d3ced318a2bc03ae71cd92 — R1 (18cd90a5) → R2 delta is +50/-14 across the same two files.
Position: RIGHT direction — R2 closes the extrapolation blocker I raised on occlusionCoverage:950-963 at R1. The mechanism I asked for landed verbatim (per-fragment sampling with a normalized denominator), the regression pin fails at R1 head and passes at R2 head, and the shared textClientRects helper anchors both sibling detectors on the same fragment geometry with no drift. Rames posted an LGTM at 06:20:24Z; I converge and add one differentiated verification below.
1. extrapolation-blocker → ✅ resolved
Diff at layout-audit.browser.js:952-970 matches the ask verbatim:
for (const textRect of textRects) {
for (const yFraction of OCCLUSION_PROBE_Y_FRACTIONS) {
const y = textRect.top + textRect.height * yFraction;
...
coveredFraction: round(hits / (OCCLUSION_GRID_POINTS * textRects.length)),Probes are per-fragment (each fragment's own top + height * yFraction) rather than per-union, and the denominator scales with fragment count so coveredFraction remains a scene-normalized 0-1 ratio (single-fragment path is byte-identical). Call site at :1010-1013 feeds textClientRects(element, true) — the same helper collectSolidTextBlocks:583 uses for the R1 content_overlap fix, so both sibling detectors now share fragment geometry through one helper with no drift risk.
Cross-check I ran that differentiates from Rames. Traced the new regression against the R1 code to confirm it's a real tripwire, not aspirational:
- At R1 SHA
18cd90a5the test atlayout-audit.browser.test.ts:1507-1531WOULD have failed. UniontextRectcoverstop=500, bottom=620, height=120. OldocclusionCoveragesamples aty = 500 + 120 * {0.25, 0.5, 0.75} = 530, 560, 590. The mock's540 < y < 580gap window catchesy=560→ 9/27 = 33.3% coverage → clearsPROSE_COVERAGE_FLOOR=0.15→ falsetext_occluded. Exact numerics from my R1 finding. - At R2 SHA
7cacbe8cthe same test passes. Fragment 1 (top=500, h=40) samples aty=510, 520, 530; fragment 2 (top=580, h=40) samples aty=590, 600, 610. All six y-values sit outside the540 < y < 580window;elementFromPointreturnsheadline(notoverlay) at every probe; 0 hits,occluderstays null,occludedTextIssuereturns null. - Test drives production code, not a local reproduction.
installAuditScript()at:1939-1941runswindow.eval(script)wherescriptis the actual audit source;runAudit()at:2027-2033invokeswindow.__hyperframesLayoutAudit. No aspirational-characterization-test shape here — this is a genuine tripwire against future refactors that reintroduce union-bbox probing.
2. sibling-consumer scope re-scan → clean
Re-audited every textRectFor / textClientRects consumer at R2 head to confirm no new union-bbox blind spot slipped in:
| Site | Line | Consumer intent | R2 scope |
|---|---|---|---|
textOverflowIssues |
:439 |
text-vs-container overflow | union is semantically correct (painted text outside container is still overflow, gaps and all) — unchanged |
collectSolidTextBlocks |
:583-584 |
overlap fragments + union diagnostic | R1's fix; now paired symmetrically with occlusion |
occludedTextIssue |
:1006-1008 |
fragment probes + union diagnostic | THIS PR's R2 fix ✅ |
invisibleTextIssue |
:1043 |
existence gate + diagnostic, no spatial compare | unchanged, correct |
panelOutOfCanvasIssues |
:1169 |
text-vs-canvas overflow | union is semantically correct — unchanged |
| geometry candidate | :1373 |
selector fallback, no spatial compare | unchanged, correct |
No new spatial-comparison consumer using the union bbox. The extrapolation is fully bounded to the two sites that needed the fix.
3. Rames's converged nits → ⚠️ mitigated, accepting
Rames flagged two note-only items and one non-blocking follow-up in his R2. I converge and won't duplicate the write-up; briefly:
- Redundant
textClientRectsat:1006-1008(called viatextRectForon:1006and directly on:1008) — twodocument.createRange()+getClientRects()invocations per element. Extraction totextFragmentsAndUnion(element, true) → { union, fragments }would remove the redundancy at bothcollectSolidTextBlocks:583-584and here. Non-blocking; picking it up requires touching R1's already-shipped call site so the follow-up is real cleanup, not stealth-scope. textRects.length > 0 ? textRects : [textRect]fallback is provably unreachable at:1012—textRectForat line 244-246 returns null ifftextClientRects(element, directOnly)returns empty, same subject same flag. The[textRect]branch cannot fire. Reads as defensive; simplifying totextRectsalone makes the invariant explicit. Note-only.- Missing complementary positive-hit multi-fragment regression — the new test pins the gap-only false-positive class. A two-fragment block where an overlay fully covers one fragment (27/54 = 50% coverage) proving
text_occludedstill fires would parameterize the multi-fragment arm across BOTH branches of thePROSE_COVERAGE_FLOORconditional. Math checks out analytically (I ran the same trace as Rames), and single-fragment tests upstream still exercise the atomic-label + prose-floor paths, so the multi-fragment real-hit branch is untested but bounded in risk. Mitigation-accepting rather than block-worthy: a full "positive-parameterize" follow-up is worthwhile per the regression-coverage extrapolation rule you pushed 2026-07-14, but not shipping-block-tier when the analytical proof is this tight and the single-fragment corpus already pins the floor behavior. Follow-up worth filing.
4. CI + envelope
- CI at freshness pull: 30+ SUCCESS + skips, 0 FAILURES, 5 IN_PROGRESS (
Analyze javascript-typescript,Render on windows-latest,Tests on windows-latest,CLI smoke (required),Smoke: global install). Fallow, Lint, Format, Studio smoke, CLI shims on ubuntu/macos, Producer unit + integration, Typecheck, Test all green. No red on the diff's own code. Monitoring the Windows shards + CLI-smoke completion. - Envelope clean. Both commits authored by Miguel Ángel, no
Co-Authored-By: Claudetrailers. PR body carries Compound Engineering + GPT-5 badges (author style, not the CC-default🤖 Generated with [Claude Code]footer).
Verdict
APPROVE. Extrapolation resolved with the exact per-fragment mechanism R1 asked for, single-fragment behavior is provably byte-identical, the regression pin is a real tripwire against the specific inter-line-gap shape (verified end-to-end at both SHAs), and the sibling content_overlap and text_occluded detectors now share fragment geometry through one helper with no drift. Two note-only cleanups + one non-blocking positive-hit follow-up land as mitigation-accepting, not blockers.
— Via
Summary
Rotated multiline text no longer triggers
content_overlapwhen another label occupies only the empty space between its rendered lines. The audit now measures intersections across the browser's individual text fragments while retaining the union rectangle for diagnostics, so genuine text collisions still use the existing 20% threshold.Verification
layout-audit.browser.test.ts: 70/70 passedcheckfixture changed from persistentcontent_overlapacross 9 samples to zero layout findings