fix(rules): adopt glimmer-attr-presence for runtime-correct attribute classification by johanrd · Pull Request #2770 · ember-cli/eslint-plugin-ember

johanrd · 2026-04-28T11:17:20Z

Summary

Adopts the new lib/utils/glimmer-attr-presence.js shared utility (introduced in #2769) across 4 master rules that misclassified attributes whose source-AST shape differs from runtime rendering. Each fix is a separate commit with regression-locking test cases that demonstrate the bug being closed.

Depends on #2769 (docs/glimmer-attribute-behavior + the classifyAttribute utility). This PR's diff includes #2769's commits until that lands; rebase will clean it up.

Why

A recent empirical audit against [email protected] (documented in #2769) verified that several Glimmer attribute-rendering forms produce runtime DOM that doesn't match the obvious AST reading. Five common audit footguns that affect master rules:

Bare-mustache attr={{"false"}} is JS-truthy → renders attr="false" (kept), not omitted.
Concat attr="{{X}}" is never falsy at runtime — IDL property is set true regardless of inner literal value.
Bare-mustache attr={{false}} / {{null}} / {{undefined}} on boolean / ARIA / numeric attributes causes Glimmer to omit the attribute at runtime — findAttr AST-presence is the wrong check.
Plain string attrs (e.g. role, autocomplete, href) do not participate in falsy-coercion — bare {{false}} renders the literal "false".
aria-hidden={{true}} (h5) renders aria-hidden="" (contested per ARIA), not aria-hidden="true" (h7).

Each commit fixes one rule against the specific row IDs from the verified reference table.

Per-rule changes (one commit each)

1. `template-link-rel-noopener` — recognize mustache forms; conservative-skip dynamic `rel`

Before: target and rel were inspected as GlimmerTextNode only. Dynamic rel={{this.x}} (or concat rel="x{{y}}") was treated as the empty string and false-positive flagged. target={{"_blank"}} (i2 analog) was silently missed.

After:

Both attributes flow through classifyAttribute. Static text, bare-string-literal, and concat-with-literals all map to the same resolved string.
Dynamic rel values are conservative-skipped (the doc's concat is never falsy observation guarantees the attribute is rendered; lint can't verify the tokens).
Autofix becomes report-only when rel is present in a non-text form, to avoid duplicate-attribute or binding-destruction.

Doc rows cited: i1, i2, i3, concat is never falsy.

2. `template-no-nested-interactive` — `tabindex` AST-presence → effective-runtime check (rows t6, t7)

Before: hasAttr(node, 'tabindex') returned true for any AST attribute, including bare {{false}} / {{null}} / {{undefined}} that Glimmer omits at runtime. False-positive flag on <button><div tabindex={{false}}></div></button>.

After: new isTabindexEffectivelySet helper uses classifyAttribute and returns true only when the attribute will actually render. Both call sites in isInteractive and isInteractiveOnlyFromTabindex are updated.

Doc rows cited: t6, t7.

Other hasAttr sites (usemap, disabled, contenteditable) are intentionally unchanged — usemap and disabled are non-falsy-coerced or already value-checked; the doc-verified bug is tabindex.

3. `template-require-context-role` — `aria-hidden` truthy-detection broadened (rows h3, h7, h12, h14)

Before: hasAriaHiddenTrue matched only aria-hidden="true" as a GlimmerTextNode. Mustache forms that render the same aria-hidden="true" at runtime were missed:

aria-hidden={{"true"}} (h7)
aria-hidden="{{true}}" (h12)
aria-hidden="{{'true'}}" (h14)

Each was a false positive: a child with role="treeitem" inside a hidden ancestor was reported as missing-context, even though the entire subtree is hidden from AT.

After: presence === 'present' && value === 'true' via classifyAttribute matches exactly h3/h7/h12/h14. Note that h5 (bare {{true}} → renders aria-hidden="", contested) deliberately does not match.

Doc rows cited: h3 (existing), h7, h12, h14. h5 deliberately excluded.

4. `template-require-mandatory-role-attributes` — two bugs (rows i2, h6, h9, h10)

Bug 4a — bare-mustache string-literal role={{"option"}} was silently skipped (treated as dynamic) even though it renders role="option". The required-ARIA validation never ran for these forms.

Bug 4b — required-ARIA satisfaction was checked by AST attribute name presence. The fixture <div role="option" aria-selected={{false}} /> was encoded as a passing valid case despite the attribute being omitted at runtime per row h6 (the bug was locked into the test suite).

After:

getStaticRolesFromElement uses classifyAttribute — recognizes i1 / i2 / i3 forms uniformly.
The aria-attribute filter excludes attributes whose classifyAttribute(attr).presence === 'absent', so bare aria-foo={{false}} no longer pretends to satisfy the role.
The bug-encoding fixture moved from valid to invalid; replaced with aria-selected="true" / "false" static-value cases that preserve the original "aria-selected satisfies role=option" intent.

Doc rows cited: i1, i2, i3 (role classification); h6, h9, h10 (aria-* falsy-coercion); cross-attribute observation on falsy-coercion list.

Not changed: `template-require-iframe-title`

The audit pass against this rule flagged title={{""}} as a REAL_BUG. On closer inspection the rule already handles bare-mustache string-literal title via processStaticTitle (lib/rules/template-require-iframe-title.js:156-158), and the case is asserted at tests/lib/rules/template-require-iframe-title.js:41 (emptyTitle error). No change needed; audit finding withdrawn.

Test plan

All four rules pass their own test suites (existing + new fixtures).
Full suite: 9552 / 9552 passing.
Prettier and ESLint clean.
Visual diff review against feat(utils): add glimmer-attr-presence + verified-behavior reference doc #2769 (this PR includes its commits until feat(utils): add glimmer-attr-presence + verified-behavior reference doc #2769 merges).

Doc cross-references

Every fix commit message cites the specific doc rows from docs/glimmer-attribute-behavior.md (introduced in #2769) that justify the new behavior. Where the new behavior diverges from a doc-verified row, the rule code carries an inline comment with the row ID so future maintainers can confirm.

🤖 Generated with Claude Code

Captures empirically-verified Glimmer rendering behavior for HTML attributes with mustache values, so rule authors classifying GlimmerBooleanLiteral / GlimmerStringLiteral / GlimmerConcatStatement have a ground-truth reference instead of intuition. Notable findings the doc pins down: - attr={{"false"}} (bare string "false") renders as attr="false" — TRUTHY, not falsy as the literal suggests. - attr="{{false}}" (concat) sets the IDL property to true regardless of the literal value inside, even when HTML serialization shows nothing. Verified against <video muted="{{false}}"> → videoEl.muted === true. - Non-reflecting boolean attrs (muted, autoplay) and reflecting ones (disabled, hidden) diverge in HTML serialization but agree at the IDL property layer. Includes a copy-pasteable reproduction template so future readers can re-verify if Glimmer behavior changes. Adds a pointer in README's "Creating a New Rule" section.

Replaces the prior tables (which mixed verified data with extrapolations marked "(assumed)") with strictly-verified per-attribute tables. Every cell populated from rendering and IDL inspection in ember-source 6.12. Structure: - One "Reference table" section, five per-attribute sub-tables (muted, disabled, aria-hidden, tabindex, autocomplete) - One "To reproduce the reference table" section with the exact template and JS console snippet, inline (no separate fixture file) - Cross-attribute observations summarizing the rules the data reveals Findings the new tables make explicit: - Bare-mustache falsy set is {{false}}/{{null}}/{{undefined}}/{{0}} for boolean-coerced attrs (boolean HTML, ARIA, numeric); {{""}} is kept as attr="". - Bare-mustache string literals never coerce — attr={{"false"}} renders as attr="false". - Concat-mustache for boolean HTML attrs sets the IDL property to true regardless of the literal value inside (verified for both reflecting and non-reflecting attrs). - Concat-mustache for ARIA/string attrs renders the stringified value literally — no boolean coercion. aria-hidden="{{false}}" is visible. - Plain string attrs (autocomplete) skip Glimmer's boolean coercion entirely; autocomplete={{false}} renders as autocomplete="false". The video.muted snapshot reads IDL muted=false for static attribute forms (m1-m4, m7-m8, m11) because the test runs before media load — the doc explains how defaultMuted reflects to muted at load time, so the rule's "At play time" column is the lint-truth column rule authors should use.

…les" guide Adds a practical-implementation section between the reference table and the reproduction template. It maps each AST shape (GlimmerTextNode / GlimmerMustacheStatement with each path type / GlimmerConcatStatement) to a verdict, citing the row IDs from the reference table so rule authors can implement classification correctly without re-deriving the model. Includes: - AST-shape verdict table — direct mapping rule authors can copy from - Six common mistakes section, each tied to specific row IDs - Pointer to the (forthcoming) lib/utils/glimmer-attr-presence.js utility that will encode the verdict table once and let rules consume the resolved kind + value rather than re-walking the AST The audit of master rules and the open feature PRs found 18 REAL_BUG findings (12 in PRs, 6 in master) — all classifiable into the bullet-1 through bullet-4 footguns this guide enumerates.

…ng model Adds lib/utils/glimmer-attr-presence.js exporting: - classifyAttribute(attrNode, options?) → { presence, value } Maps every AST shape (valueless / GlimmerTextNode / GlimmerMustacheStatement with each path type / GlimmerConcatStatement) to a (presence, value) pair per the verified model in docs/glimmer-attribute-behavior.md. Each branch cites the relevant doc row IDs (m1–m19, h1–h15, d1–d10, t1–t7, i1–i5). - inferAttrKind(name) → 'boolean' | 'aria' | 'numeric' | 'plain-string' Used when classifyAttribute callers don't pass options.kind explicitly. - BOOLEAN_HTML_ATTRS, NUMERIC_ATTRS — exported sets, useful for callers that want to extend the kind model. Key empirical asymmetries this util encodes correctly (and that audit findings show several rules currently misclassify): - Bare {{false}} / {{null}} / {{undefined}} on falsy-coerced kinds (boolean / aria / numeric) → presence='absent' (Glimmer omits attribute). Same forms on plain-string → presence='present', value='false' / etc. - Bare {{"false"}} (StringLiteral) is JS-truthy, never coerced — renders the literal value across all attribute kinds. - aria-hidden={{true}} renders aria-hidden="" (h5, contested), not aria-hidden="true" — the util surfaces value='' here so callers comparing value === 'true' don't false-match. - Concat is never falsy: any concat form is presence='present'; the resolved value comes from the existing getStaticAttrValue helper. Tests: 35 unit tests covering every doc row + the kind-override option. Updates docs/glimmer-attribute-behavior.md to reference the actual file and replaces the "(forthcoming)" sketch with a working example.

…ation / html-void-elements Correctness fixes from PR ember-cli#2769 review: - Boolean concat now returns canonical `value: 'true'` instead of leaking the inner literal. Per doc rows m13-m19, d7-d10 the IDL is set true regardless of inner value, so callers checking `value === 'false'` to detect "off" no longer get a wrong answer for `<video muted="{{false}}">`. - {{true}} on numeric / plain-string now returns `unknown` (untested in doc; was previously claiming `value: 'true'` by extrapolation). - `inferAttrKind` is now case-insensitive (`Disabled`, `ARIA-Hidden`, etc.). Drop hand-rolled spec lists in favor of upstream packages: - `property-information` for boolean / overloadedBoolean / number attribute classification, replacing the 24-entry BOOLEAN_HTML_ATTRS and 3-entry NUMERIC_ATTRS Sets. `colspan` is added as a small NUMERIC_OVERRIDES Set to compensate for an upstream gap in property-information 7.1.0 (rowspan and cols are marked `number: true`, colspan isn't). - `html-void-elements` in template-block-indentation.js and template-self-closing-void-elements.js, deduplicating two parallel 16-entry VOID_TAGS Sets. Internal API change: BOOLEAN_HTML_ATTRS and NUMERIC_ATTRS are no longer exported from glimmer-attr-presence. The util's public surface is now `classifyAttribute` and `inferAttrKind`. Callers wanting the underlying classification can use property-information directly.

…oss-attr m6/d3 analog) `controls` on `<audio>`/`<video>` is an HTML boolean attribute, so per the cross-attribute observation in docs/glimmer-attribute-behavior.md, bare `{{false}}` / `{{null}}` / `{{undefined}}` cause Glimmer to omit the attribute at runtime. The helper's previous AST-presence check (hasAttribute) treated `<video controls={{false}}>` as still having controls — a false positive that propagated to every rule using isHtmlInteractiveContent: nested-interactive, no-noninteractive-tabindex, interactive-supports-focus, click-events-have-key-events, etc. After: - `controls` flows through classifyAttribute. Bare-mustache falsy literals now correctly classify as 'absent' → element is NOT interactive at runtime. - `href` and `usemap` continue to use AST-presence — those are plain string attributes that don't falsy-coerce (i4 analog: bare `{{false}}` on a plain string renders as the literal `"false"`, attribute kept). Concat forms (`controls="{{X}}"`) still classify as 'present' because concat is never falsy at runtime — so the existing pass-through behavior for concat is preserved. Tests: - New: `<video controls={{false}}>` → not interactive (regression-locking). - New: `<audio controls={{null}}>` → not interactive. - New: `<video controls="{{false}}">` → IS interactive (concat sets IDL true regardless of inner literal). - All 9555 existing tests pass — no rule was relying on the buggy behavior, so this fix lands cleanly across all consumers.

…ustache forms Before this commit: - `target` and `rel` were inspected as `GlimmerTextNode` only. Any non-text source on `rel` was treated as the empty string, producing false-positive flags on dynamic and concat forms even when the runtime rel value would satisfy the rule. - `target={{"_blank"}}` (i2 analog: renders `target="_blank"`) was silently ignored — a missed flag. After: - Both attributes flow through the new `classifyAttribute` helper. - Static text (`target="_blank"`), bare-string-literal mustache (`target={{"_blank"}}`), and concat with all-static parts (`target="{{'_blank'}}"`) are all recognized as `_blank`. - Dynamic `rel` values (`rel={{this.x}}`, `rel="x{{y}}"`) are conservative- skipped because the runtime value isn't statically extractable. The doc's cross-attribute observation "Concat is never falsy" guarantees the attribute is rendered; the tokens may already be correct. - Autofix is now report-only when `rel` is present in a non-text form, to avoid producing duplicate `rel` attributes or destroying author bindings. New test coverage: - Valid: `rel={{"noopener noreferrer"}}` (bare-string-literal — was false-positive flagged) - Valid: `target={{"_blank"}}` paired with proper rel (was missed) - Valid: `rel={{this.relValue}}` (dynamic — was false-positive flagged) - Valid: `rel="noopener {{this.extra}}"` (concat with dynamic — same) 22 tests pass. Doc rows cited: i2, i3, "concat is never falsy" cross-attribute observation.

…ex (rows t6, t7) Before: - `hasAttr(node, 'tabindex')` returned true whenever a `tabindex` AttrNode existed in the AST, regardless of value. This produced false positives on `<button><div tabindex={{false}}></div></button>` and similar — at runtime the inner div has no tabindex (Glimmer omits per t6/t7), so it isn't interactive, so the nesting isn't a violation. - Same shape on dynamic `tabindex={{this.idx}}` — runtime value could be any of false/null/-1/0/1/etc.; statically claiming "interactive" was overconfident. After: - New `isTabindexEffectivelySet(node)` helper uses `classifyAttribute` from `lib/utils/glimmer-attr-presence.js`. It returns true only when the attribute will actually render at runtime (presence === 'present' per the verified model). Bare-mustache falsy literals classify as 'absent'; dynamic values classify as 'unknown' and are conservative- skipped. - Both call sites updated: the main `isInteractive` check (line 152) and the `isInteractiveOnlyFromTabindex` final-decision branch. Test additions (regression-locking): - Valid: `<button><div tabindex={{false}}></div></button>` (t6) — was a false positive - Valid: `<button><div tabindex={{null}}></div></button>` (t7) - Valid: `<button><div tabindex={{undefined}}></div></button>` (inferred from cross-attribute observation) - Valid: `<button><div tabindex={{this.idx}}></div></button>` (dynamic, conservative-skip) - All variants added in both GJS and HBS suites. 100 tests pass. Other `hasAttr` sites in this rule (`usemap`, `disabled`, `contenteditable`) are intentionally left as-is: `usemap` and `disabled` are plain string / enumerated where bare-falsy renders the literal value (i4 analog), so AST-presence is correct; `contenteditable` already reads the value to decide enabled vs `"false"`. Doc-verified bug is tabindex.

…ssifyAttribute (rows h3, h7, h12, h14) Before: - `hasAriaHiddenTrue` matched only `aria-hidden="true"` as a `GlimmerTextNode`. - Mustache forms that render the same `aria-hidden="true"` at runtime were silently treated as visible: - `aria-hidden={{"true"}}` (h7) → renders `aria-hidden="true"` - `aria-hidden="{{true}}"` (h12) → renders `aria-hidden="true"` - `aria-hidden="{{'true'}}"` (h14) → renders `aria-hidden="true"` - Each was a false positive: a child with role="treeitem" inside a hidden ancestor was reported as missing-context, even though the entire subtree is hidden from assistive tech and "context" is moot. After: - `hasAriaHiddenTrue` flows through `classifyAttribute` from the new util. The check is `presence === 'present' && value === 'true'`, which matches exactly h3 / h7 / h12 / h14 — every source form whose IDL `ariaHidden` is `"true"`. - Note: h5 (bare `{{true}}`) renders `aria-hidden=""` (contested per ARIA spec), so `classifyAttribute` returns `value=""` for that form and the equality check correctly excludes it. The rule deliberately does not treat the contested empty-value case as hidden. Test additions in both GJS and HBS suites: - Valid: `<div aria-hidden={{"true"}} role="tablist"><div role="treeitem">…` - Valid: `<div aria-hidden="{{true}}" role="tablist"><div role="treeitem">…` - Valid: `<div aria-hidden="{{'true'}}" role="tablist"><div role="treeitem">…` 111 tests pass. Doc rows cited: h3 (existing), h7, h12, h14.

…e (rows i2, h6, h9, h10) Two bugs fixed; both stem from AST-based classification ignoring runtime rendering. Bug 1 — bare-mustache string-literal role ignored (i2 analog). Before: getStaticRolesFromElement only recognized role attributes whose value was a `GlimmerTextNode`. The form `role={{"option"}}` (which renders `role="option"` per the doc's i2-analog row — bare-mustache string literals never coerce) was treated as dynamic and silently skipped, so the rule never validated required ARIA attributes for it. After: getStaticRolesFromElement uses classifyAttribute. Static text, bare-string-literal mustache, and concat with all-literal parts (i1, i2, i3 analogs) all resolve to the same string and feed into role-token splitting. Bug 2 — bare-mustache falsy aria attribute counted as satisfying required-aria (h6, h9, h10). Before: required-ARIA satisfaction was checked by AST attribute name presence — `<div role="option" aria-selected={{false}} />` was treated as having aria-selected even though Glimmer OMITS the attribute at runtime per doc rows h6 (`{{false}}`), h9 (`{{null}}`), h10 (`{{undefined}}`). This was *also* encoded as a passing valid fixture, locking the bug in. After: the aria-attribute filter excludes attributes whose `classifyAttribute(attr).presence === 'absent'`. The previously-passing fixture moved from valid to invalid; replaced with positive cases that use static aria values (`aria-selected="true"` / `"false"`) and the new i2 role classification. Test coverage: - Valid (new): `<div role={{"option"}} aria-selected="true" />` - Invalid (new): `<div role="option" aria-selected={{false}} />` (h6) - Invalid (new): `<div role="option" aria-selected={{null}} />` (h9) - Invalid (new): `<div role={{"option"}} />` (i2 + missing aria) - All variants in both GJS and HBS suites. - Replaced the bug-encoding fixture (valid `<div role="option" aria-selected={{false}} />`) with `aria-selected="true"`/`"false"` static-value cases preserving the original "aria-selected satisfies role=option" intent. 106 tests pass. Doc rows cited: i1, i2, i3 (role classification); h6, h9, h10 (aria-* falsy-coercion).

johanrd added 6 commits April 28, 2026 10:33

docs: cite ember-source 6.12, drop separate Glimmer version

6052bae

fix(docs): use querySelector to satisfy unicorn/prefer-query-selector

e0a8d17

johanrd marked this pull request as draft April 28, 2026 11:17

johanrd force-pushed the fix/master-rules-glimmer-attr-presence branch from f3e1e42 to d7feb44 Compare April 28, 2026 11:50

johanrd added 5 commits April 28, 2026 14:35

johanrd force-pushed the fix/master-rules-glimmer-attr-presence branch 2 times, most recently from 76328d2 to f85c914 Compare April 28, 2026 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(rules): adopt glimmer-attr-presence for runtime-correct attribute classification#2770

fix(rules): adopt glimmer-attr-presence for runtime-correct attribute classification#2770
johanrd wants to merge 12 commits intoember-cli:masterfrom
johanrd:fix/master-rules-glimmer-attr-presence

johanrd commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

johanrd commented Apr 28, 2026

Summary

Why

Per-rule changes (one commit each)

1. template-link-rel-noopener — recognize mustache forms; conservative-skip dynamic rel

2. template-no-nested-interactive — tabindex AST-presence → effective-runtime check (rows t6, t7)

3. template-require-context-role — aria-hidden truthy-detection broadened (rows h3, h7, h12, h14)

4. template-require-mandatory-role-attributes — two bugs (rows i2, h6, h9, h10)

Not changed: template-require-iframe-title

Test plan

Doc cross-references

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

1. `template-link-rel-noopener` — recognize mustache forms; conservative-skip dynamic `rel`

2. `template-no-nested-interactive` — `tabindex` AST-presence → effective-runtime check (rows t6, t7)

3. `template-require-context-role` — `aria-hidden` truthy-detection broadened (rows h3, h7, h12, h14)

4. `template-require-mandatory-role-attributes` — two bugs (rows i2, h6, h9, h10)

Not changed: `template-require-iframe-title`