Skip to content

feat(lint): warn when a class="clip" element partially overrides inset:0#1925

Open
miguel-heygen wants to merge 1 commit into
mainfrom
fix/lint-clip-partial-inset-stretch
Open

feat(lint): warn when a class="clip" element partially overrides inset:0#1925
miguel-heygen wants to merge 1 commit into
mainfrom
fix/lint-clip-partial-inset-stretch

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Root cause

The canonical .clip rule (see hyperframes-core/minimal-composition.md) is position: absolute; inset: 0, so every class="clip" element is pinned to all four edges by default. A more specific selector (an #id/.class <style> rule, or an inline style) that overrides only some of top/right/bottom/left without also giving width/height leaves the un-overridden sides pinned at the base 0 — so the element silently stretches between the author's sides and the inherited zeros instead of shrink-wrapping to its content.

Reported four independent times (e.g. a user sets bottom: 40px; left: 40px expecting a corner-anchored box and gets a full-screen stretch). inspect catches the symptom (content_overlap / text_occluded) but never names the CSS root cause, so each reporter had to getBoundingClientRect()-probe to diagnose it — and several asked directly for exactly this lint rule.

The rule (clip_partial_inset_stretch, warning)

  • Self-gating: only fires once the file's own .clip rule actually establishes the base (position:absolute|fixed + all-zero inset). Compositions that define .clip differently never trip it.
  • Per class="clip" element, merges its override declarations — matching #id/.class <style> rules by their selector subject (rightmost compound), plus inline style; the bare .clip base rule is excluded since it's the inherited pin, not an override — and flags an axis only when the author pinned exactly one of that axis's two sides with no size for the axis.
  • That "exactly one" gate is what keeps intentional shapes quiet: full-bleed (0 sides set), fully-constrained 4-side boxes, sized boxes (width+height), and position:relative/static/sticky all stay silent; auto on the opposite side (an explicit release of the base pin) also silences it.

High-precision by design. inspect remains the runtime backstop, so the rule deliberately under-reports (exotic/compound selectors, @media-nested overrides) rather than risk false positives on the lint path that runs constantly.

Test plan

  • 9 end-to-end cases via lintHyperframeHtml: bottom+left corner / single-side / inline-override → flag; sized box / full-bleed / all-four-sides / auto-release / position:relative / no-base → quiet.
  • 7 direct unit tests of the pure box-model helpers (collectClipBoxProps / clipStretchAxes) covering the XOR gate, inset shorthand, auto-release, position-detach, and declaration-block ordering (later wins).
  • Full lint package suite passes; applyDecl refactored to a dispatch table so fallow's complexity gate passes cleanly (no suppression).
  • bun run build clean; oxlint/oxfmt clean.

The canonical `.clip` rule is `position: absolute; inset: 0`, so every
class="clip" element is pinned to all four edges by default. A more
specific selector (an #id/.class rule, or an inline style) that
overrides only SOME of top/right/bottom/left without also giving
width/height leaves the un-overridden sides pinned at the base 0 — so
the element silently stretches between the author's sides and the
inherited zeros instead of shrink-wrapping to its content.

Reported four independent times (a user sets e.g. `bottom:40px;
left:40px` expecting a corner-anchored box and gets a full-screen
stretch). inspect catches the symptom (content_overlap /
text_occluded) but never names the CSS root cause, so each reporter
had to getBoundingClientRect()-probe to diagnose it, and multiple asked
directly for this lint rule.

New warning-level rule `clip_partial_inset_stretch`:
- Self-gating: only fires once the file's own `.clip` rule actually
  establishes the base (position:absolute|fixed + all-zero inset), so
  compositions that define .clip differently never trip it.
- Per class="clip" element, merges its override declarations (matching
  #id/.class <style> rules by selector SUBJECT, plus inline style; the
  bare `.clip` base rule is excluded since it's the inherited pin, not
  an override) and flags an axis only when the author pinned EXACTLY
  ONE of its two sides with no size for that axis. That "exactly one"
  gate keeps intentional shapes quiet: full-bleed (0 sides),
  fully-constrained 4-side boxes, sized boxes (width+height), and
  position:relative all stay silent; `auto` on the opposite side (an
  explicit release) also silences it.

High-precision by design — inspect remains the runtime backstop, so
the rule under-reports (exotic selectors, @media-nested overrides)
rather than risk false positives on the lint path that runs constantly.

Test: 9 end-to-end cases + 7 direct unit tests of the pure box-model
helpers (collectClipBoxProps / clipStretchAxes) covering the XOR gate,
inset shorthand, auto-release, position-detach, and block ordering.
Full lint package suite passes.
@miguel-heygen miguel-heygen marked this pull request as ready for review July 4, 2026 20:16

@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 dab8949.

🟢 Approve — thoughtful, high-precision lint rule with strong test geometry

What's right about the rule design:

  • Root cause is real and load-bearing: canonical .clip { position:absolute; inset:0 } is a genuine footgun when an override sets only some of top/right/bottom/left without a width/height. The "silent stretch" is exactly the CSS specificity + cascade behavior described — I've verified hyperframes-core/minimal-composition.md's .clip rule is the reference contract per the reference comment.
  • Self-gating via clipBaseEstablishesInsetStretch is architecturally right: the rule only fires once the file's own .clip rule actually establishes the stretch-preconditioning base (position:absolute|fixed + all-zero inset). Compositions that redefine .clip differently never trip this — no false positives on non-canonical bases.
  • The "XOR gate" (authorPinned.has(a) !== authorPinned.has(b)) is the correct semantics: exactly-one-side-pinned is the intent signal for "positioning override without size", full-bleed (0 sides) and fully-constrained (4 sides) and sized-box (width+height) are all deliberately silent. This is what makes the rule high-precision.
  • authorReleased (auto-values un-pin the inherited base) is correctly modeled — the left: 40px; right: auto "release the opposite" case is honored, matching CSS cascade semantics.
  • Position-detach (position: relative | static | sticky) correctly disables the rule for that element — those positions don't participate in inset stretching in the first place.

Test coverage geometry is exceptional:

  • 9 end-to-end lint-invocation cases covering: bottom+left corner (flag), single-side (flag), inline-style override (flag), sized box (quiet), full-bleed no-override (quiet), all-four-sides (quiet), auto-release (quiet), position:relative (quiet), no-base (quiet).
  • 7 direct unit tests on the pure collectClipBoxProps / clipStretchAxes helpers — branch coverage for XOR gate, inset shorthand, auto-release, position-detach, and declaration-block ordering (later wins).
  • Exporting the helpers for direct testing is the right call — keeps the pure box-model logic isolated from the tag-matching machinery.

Minor observations (none blocking):

  1. nitparseCssDeclarations splits on ; which will misparse a declaration containing a ; inside a quoted string (e.g. content: "a;b"). CSS content: on a .clip override is exotic, and misparsing here just under-reports (splits the value in half → both halves fail the property lookup → declaration silently dropped). Consistent with the rule's stated "under-report rather than false-positive" philosophy, so this is fine as-is.
  2. nitsubjectCompound splits on [\s>+~]+ which correctly handles >, +, ~ combinators but doesn't handle || (column combinator, rarely used). Same "under-report" posture applies — the runtime inspect backstop is documented as the safety net for exotic cases.
  3. nit — the selector-subject match uses new RegExp('\\.' + c + '(?![\\w-])') per class — technically vulnerable to a class name that's a regex metacharacter (e.g. class="clip clip.subvariant" — though CSS classes can't legally contain . anyway). No real hazard.
  4. question@media-nested overrides are called out as deliberately excluded in the PR body (extractCssRuleBodies skips headers starting with @). Good docstring. Worth considering whether responsive compositions (that DO use @media for viewport tiers) will develop this same class of bug and want a broader rule variant later — but that's a follow-up, not this PR's scope.

Complexity gate handling (positive): applyDecl was refactored to a dispatch table so fallow's complexity gate passes cleanly WITHOUT // fallow-ignore suppression. That's the right pattern per per-repo hygiene. The one // fallow-ignore-next-line complexity on the rule body itself is legitimate — the outer loop touches many concerns.

Cross-PR coupling note: this and #1897 both add error/warning rules to composition.ts in the same commit window. The import { OpenTag } addition to the composition.ts top-level in this PR (type OpenTag from utils) may conflict-in-context if #1897 lands first (or vice versa) — merge order matters for the diff, not the semantics. Trivial to resolve.

Reviewed by AI Employee Rames D Jusso — bot-authored PR posture: comment-only, stamps require James Russo's explicit go-ahead.

@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.

🟢 R1 verdict — well-scoped high-precision lint rule; runtime-interop clean

Runtime-interop lens: rule application at correct AST/CSS level, no interaction with existing rules, no config-flag drift.

Finding-by-finding

1. Rule application level — 🟢

packages/lint/src/rules/composition.ts:1119-1148

The rule reads two composition inputs (tags, styles) that are the same shapes every other compositionRules[] entry consumes, gates on the same readAttr helper for inline styles, and pushes findings with the same shape (code, severity, elementId, snippet). No new fields, no new context surface, no new parser. Rule application level is consistent with siblings.

Self-gating via clipBaseEstablishesInsetStretch(styles) (line 179-197): the rule only fires once the file's own .clip rule pins all four edges. Compositions that redefine .clip never trip it — that's the correct precondition for the whole class of stretch cases the rule diagnoses.

2. False-positive surface (subject-compound matching) — 🟢

packages/lint/src/rules/composition.ts:204-231

The over-report risk with regex-flavored CSS matching is that:

  • .card { top: 40px } on <div class="card clip">matchClasses = ["card"], subject-compound .card matches via \.card(?![\w-]). Correctly flagged.
  • .clip { top: 40px } on <div class="clip">matchClasses filters out clip, so this never matches. Correct: the bare .clip rule IS the inherited pin, not an override. Matches the body comment intent.
  • #foo .clip { top: 40px } on <div id="foo"><div class="clip"> — subject-compound is .clip, filtered out. Under-reported (this IS a real partial-inset override), but matches the "under-report exotic selectors, prefer inspect as runtime backstop" design stance. Not a false-positive; a documented under-report.
  • Attribute/pseudo qualifier at subject (.clip[data-foo], .clip:hover) — subjectCompound line 220 explicitly skips: if (/[[:]/.test(subject)) return false. Correct guard.

The XOR gate (clipStretchAxes, line 137-146) is the real precision lever: requiring pinned(left) && pinned(right) && !hasWidth && authorXor(left, right) per axis means:

  • Full-bleed (0 sides authored) — both pinned(left) and pinned(right) true (inherited), but authorXor false (neither authored) → quiet.
  • All-4-sides (4 sides authored) — pinned + pinned + XOR false → quiet.
  • Sized box (width set) — !hasWidth false → quiet.
  • auto-released on the opposite side — the opposite pinned() reads false → quiet.
  • position: relative/static/stickydetachesFromInset early-return → quiet.

Only "exactly one side pinned, no size, no release" trips. Precision holds.

3. Interaction with rootClassStyledSelectors — 🟢

packages/lint/src/rules/composition.ts:100-109 (existing) vs 248-263 (new)

The new rule adds its own extractCssRuleBodies helper alongside the existing extractCssSelectors used by rootClassStyledSelectors. Both operate on ExtractedBlock[] inputs shared from the lint context; neither mutates them. Both do their own comment-stripping (.replace(/\/\*[\s\S]*?\*\//g, "")) on a local copy — independent. No shared parser state.

Minor duplication (two similar CSS-block parsers now); not a blocker — the new helper needs decl-body access, the existing one only exposes selectors. Extracting a shared parser would be a follow-up refactor.

4. Rule-array position / execution order — 🟢

packages/lint/src/rules/composition.ts:1099-1147

New rule is appended to compositionRules[]. The lint pipeline aggregates findings from all rules (no short-circuit on prior findings). Order doesn't affect correctness. Sibling PRs #1893 and #1884 also add/extend entries in the same array — since findings are aggregated additively, all three merge without conflict.

The // fallow-ignore-next-line complexity comment on line 1121 is scoped to the rule callback specifically. Verified — no complexity waiver leaks to other rules.

5. Config gating — universal, no flag — 🟢

The rule fires unconditionally when clipBaseEstablishesInsetStretch(styles) returns true. No config flag. This matches the pattern of every other compositionRules[] entry (all universal). Since the rule is severity: "warning", an existing passing error-threshold lint run stays passing — warnings don't fail CI by default.

For compositions that redefine .clip differently (which the self-gate detects), the rule never runs at all. Zero false-positive surface for non-standard base-styles.


Peer-lens split: Rames covers rule-mechanic completeness + test-geometry AST coverage. My lens confirms application level, false-positive surface, cross-rule interaction, and config gating are all clean.

Review by Via (runtime-interop lens)

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