fix(check): bill a clipped text box once, not twice#2803
Draft
xuanruli wants to merge 1 commit into
Draft
Conversation
`nearestConstraint` returns the element itself when the element clips, because a clipping box is a constraint candidate. `textOverflowIssues` then reported the same physical defect a second time as `text_box_overflow <sel> inside <same sel>`, so every `clipped_text` arrived as two errors and inflated `errorCount`. The guard is `container === element && containerClips`, not `container === element`: a painted, sized box is also a constraint candidate without clipping anything, and there `clipped_text` cannot speak (it needs `clipsOverflow`). An earlier revision omitted the second condition and silenced a real defect — a painted 200px chip whose text ink ran 520px past its edge reported nothing at all. The new test pins that case; it fails against the over-broad guard and the duplicate test fails against HEAD. Measured over 31 fixtures: 9 changed, all the same shape — the duplicate gone, the `clipped_text` error and `ok: false` kept. Includes four fixtures that are genuine defects wearing `overflow:auto` / `ellipsis` / `line-clamp`; all four still gate. 13 registry examples byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
nearestConstraintreturns the element itself when the element clips — a clipping box is a constraint candidate (isConstraintCandidate).textOverflowIssuesthen reported the same physical defect a second time astext_box_overflow <sel> inside <same sel>, so everyclipped_textarrived as two errors and inflatederrorCount.The guard needs both conditions
container === element && containerClips, notcontainer === elementalone. A painted, sized box is also a constraint candidate without clipping anything, and thereclipped_textcannot speak for it (it requiresclipsOverflow). An earlier revision of this PR omitted the second condition, and review caught it: a painted 200px chip whose text ink ran 520px past its edge went fromerror text_box_overflow {right: 792px}to reporting nothing at all.Both cases are now pinned:
clipped_textdouble-count testcontainer === elementonlyVerification
31 fixtures (
ovfcorpus + the reviewer's own): 9 changed, all the same shape — the duplicatetext_box_overflowremoved, theclipped_texterror andok: falseretained. 22 unchanged. Among the 9 are four fixtures that are genuine defects wearing deliberate-truncation CSS (overflow:autobody copy cut off,overflow:scrollheadline, anellipsisstat truncated to$1,2…, a headline clamped from 3 lines to 1) — all four still gate, so no recall was traded for the de-duplication.13 registry examples byte-identical.
packages/cli: 2148 tests pass; the single failure (src/telemetry/agent_runtime.test.ts, agent-env detection) reproduces on cleanHEADand is environment-dependent, unrelated. oxlint / oxfmt /tsc --noEmitclean.Not in scope — three
error-severity false positives remainConfirmed by measurement in the same round, and deliberately left out of this PR after two reviews rejected my first attempt at them. All three currently fail the build on correct compositions:
-webkit-line-clamp,text-overflow: ellipsis, andoverflow: auto/scrolleach produceclipped_textaterror. My first attempt exempted them wholesale; that silenced real defects — 4 of 7 lines of body copy invisible in anoverflow:autocard, a stat truncated to$1,2…, a headline clamped away — because deliberate-truncation CSS is a mechanism, not a statement that the truncation is acceptable. It was also axis-blind:text-overflowapplies only to inline overflow and-webkit-line-clamponly to block overflow, but the gate returned early on either. The reviewers' concrete design: keepauto/scrollreporting (nobody scrolls an MP4 — content past the fold is unconditionally invisible in the output) and exemptellipsis/line-clamponly below a hidden-fraction threshold, per axis, using the already-computedoverflowX / scrollWidthandscrollHeight / clientHeight.text_box_overflowaterror. My first attempt vetoed it downstream; that threw the element away instead of continuing the constraint walk, so text escaping two nested boxes reported nothing, andescaped_containerdoes not cover the gap (it requiresposition: absoluteand explicitly skips the detached-but-hugging case). The right fix is inisConstraintCandidate— a non-clipping candidate should have to intersect the text rect, lettingnearestConstraintwalk on — which is a bigger change with its own corpus sweep.Also unfixed, larger, and deliberately separate:
staticIssueKeyincludesformatOverflow(issue.overflow), so a breach whose depth changes between samples splits per-sample and every group is demoted toinfo— un-gating real errors for any moving element across all 9 tiered codes. That fix makes the audit louder and touches the same tuple as #2801; landing it alongside "stop failing on ellipsis" would make the net corpus effect unattributable.Doc note
skills/hyperframes-core/references/data-attributes.md:59states the current doctrine: "overflow: hiddenclips the visual but does not suppress a layout finding. This attribute is the escape hatch; CSS overflow is not." This PR does not change that contract — it only stops double-billing — so the doc stays accurate. The deferred FP fixes above will contradict it and must update it.🤖 Generated with Claude Code