Skip to content

fix(cli): avoid multiline overlap false positives#2468

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/layout-multiline-overlap
Jul 15, 2026
Merged

fix(cli): avoid multiline overlap false positives#2468
miguel-heygen merged 2 commits into
mainfrom
fix/layout-multiline-overlap

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Summary

Rotated multiline text no longer triggers content_overlap when 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

  • Added a regression with two separated line fragments and a label in their empty gap
  • layout-audit.browser.test.ts: 70/70 passed
  • Full monorepo build passed
  • Real-browser check fixture changed from persistent content_overlap across 9 samples to zero layout findings
  • Pre-commit lint, format, typecheck, tracked-artifact, and Fallow checks passed

Compound Engineering
GPT-5

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, textClientRects returns [singleRect], rectsArea degenerates to rectArea(singleRect), and fragmentIntersectionArea degenerates to intersectionArea(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. ✓
  • textClientRects filter for tiny rects (width > 0.5 && height > 0.5 at :231-233) guards against zero-area range-rect noise. Same filter pre-PR, unchanged. ✓
  • collectSolidTextBlocks guard — only pushes blocks where textRectFor(element, true) returns non-null, so a.rects is always non-empty and rectsArea doesn't divide/threshold against zero. ✓
  • Test mock aligned. getClientRects spy at layout-audit.browser.test.ts:1350-1360 now returns the array of rects (instead of the previous single-element wrapper), matching what Range.getClientRects() returns for multiline text in a real browser. getBoundingClientRect spy at :1342-1345 uses the new boundingTextRect helper to compute the AABB of the fragment array — same shape textRectFor would 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 getClientRects AABBs partly overlap each other in the direction of rotation (the diamond-AABBs of two adjacent 45°-rotated lines share a corner). Consequences: rectsArea slightly over-counts actual text visual area (denominator up → threshold looser); fragmentIntersectionArea double-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.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-585collectSolidTextBlocks collects rects alongside rect, so the emitted issue (rect: a.rect at :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's rectsArea(a.rects) replacing rectArea(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-876 covers 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 trip content_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 / rectsArea at :600-610: one-liner comment noting the ratio denominator is sum(fragmentArea) rather than union.area on 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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) at layout-audit.browser.js:952-971: outer for (const textRect of textRects) loop wraps the existing 3×9 y/x-fraction grid. coveredFraction normalizes hits against OCCLUSION_GRID_POINTS * textRects.length — a scene-wide 0-1 fraction that stays comparable to pre-PR values for single-fragment blocks.
  • Call site in analyzeElementOcclusion at :1002-1012: textClientRects(element, true) collected alongside the existing textRectFor union rect; passed via textRects.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 at y=500-540 and y=580-620, opaque overlay at y=540-580 (the gap). elementFromPoint mock returns overlay for 540 < y < 580 and headline elsewhere. With OCCLUSION_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.15 gate skipped; no text_occluded finding. ✓

Verification

  • Single-fragment behavior is byte-identical. For textRects.length === 1, the outer loop runs once with the sole fragment as textRect and 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 at top=T, height=H, samples are at T + 0.25H, T + 0.5H, T + 0.75H — always well within the fragment, no leakage into inter-line gaps regardless of fragment count. ✓
  • occluderAt still filters the block itself via the element arg, so a fragment's own text pixels don't count as occluders. Unchanged from R1. ✓
  • Test helper installOcclusionGeometry at :1832-1901: signature widened to DOMRect | DOMRect[], getClientRects mock normalizes via normalizeTextRects (introduced in R1). Every existing test call passes a single rect(...) → 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

  • 🟢 textClientRects is called twice per element. analyzeElementOcclusion calls textRectFor(element, true) (which internally calls textClientRects) and then calls textClientRects(element, true) again a line later. Two range creations + getClientRects invocations for the same subject. Not measurable in profiles at typical text-block counts, but a textFragmentsAndUnion(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. textRectFor returned non-null only if textClientRects returned non-empty (union is derived from fragments). So the [textRect] branch can't fire. Harmless — reads as belt-and-suspenders — but if simplified into textRects alone, 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.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 18cd90a5 the test at layout-audit.browser.test.ts:1507-1531 WOULD have failed. Union textRect covers top=500, bottom=620, height=120. Old occlusionCoverage samples at y = 500 + 120 * {0.25, 0.5, 0.75} = 530, 560, 590. The mock's 540 < y < 580 gap window catches y=560 → 9/27 = 33.3% coverage → clears PROSE_COVERAGE_FLOOR=0.15 → false text_occluded. Exact numerics from my R1 finding.
  • At R2 SHA 7cacbe8c the same test passes. Fragment 1 (top=500, h=40) samples at y=510, 520, 530; fragment 2 (top=580, h=40) samples at y=590, 600, 610. All six y-values sit outside the 540 < y < 580 window; elementFromPoint returns headline (not overlay) at every probe; 0 hits, occluder stays null, occludedTextIssue returns null.
  • Test drives production code, not a local reproduction. installAuditScript() at :1939-1941 runs window.eval(script) where script is the actual audit source; runAudit() at :2027-2033 invokes window.__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 textClientRects at :1006-1008 (called via textRectFor on :1006 and directly on :1008) — two document.createRange() + getClientRects() invocations per element. Extraction to textFragmentsAndUnion(element, true) → { union, fragments } would remove the redundancy at both collectSolidTextBlocks:583-584 and 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 :1012textRectFor at line 244-246 returns null iff textClientRects(element, directOnly) returns empty, same subject same flag. The [textRect] branch cannot fire. Reads as defensive; simplifying to textRects alone 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_occluded still fires would parameterize the multi-fragment arm across BOTH branches of the PROSE_COVERAGE_FLOOR conditional. 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: Claude trailers. 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

@miguel-heygen miguel-heygen merged commit c54bf13 into main Jul 15, 2026
42 checks passed
@miguel-heygen miguel-heygen deleted the fix/layout-multiline-overlap branch July 15, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants