Canonical resolver rewrite + cluster fixes for ecosystem-CI FPs#21
Merged
Conversation
…uristic + classic-by-name) Two fixes that target the same FP class — `element-permitted-content` firing on PascalCase components that transparent-blank, where the wrapper is presumed to render the structurally-correct parent at runtime via a yield chain we can't statically resolve. 1. Heuristic suppression in detectStructuralYieldRules: When a non-native wrapper contains content-restricted structural children (`<option>`, `<th>`, `<li>`, `<optgroup>`, etc.), add `element-permitted-content` to disableForRules for the Source. Catches HDS-style multi-level yield chains (`<HdsFormSelectField as |F|><F.Options>...</F.Options>`) where precise resolution would require ~250+ lines of cross-file yield- chain analysis. Same per-Source-suppression trade-off as Thread B's wcag/h32 fix. 2. Classic-Ember by-name resolver (`lib/classic-resolver.ts`): For `.hbs` consumers (no JS imports, no Glint), walk PascalCase tags, kebab-case them, look up in node_modules under the canonical classic-Ember component template paths. Builds a componentTagMap for the .hbs path that previously had none. Catches the ember-website `<EsCard>` pattern: `EsCard` → kebab `es-card` → ember-styleguide's `addon/components/es-card.hbs` (root `<li>`). Also handles pnpm-style symlinks (treats them as directories). Together: clears HDS's ~107 `<option>`-under-`<div>` family, ember- website's `<EsCard>`-class entries, plus analogous patterns across discourse and others. Two regression fixtures + tests: - `multi-level-yield-chain-options.gts` (heuristic case): unresolvable wrapper containing `<option>` children; asserts no element-permitted-content fires. - `classic-resolver-no-import.hbs` (by-name case): `.hbs` consumer using a fake `<ClassicCard>` from a fixture node_modules; asserts the wrapper resolves to its `<li>` root. 161/161 pass. Verified on real ember-website browser-support.hbs: 18 element-permitted-content findings → 0.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces false positives from element-permitted-content by (1) adding a heuristic suppression when structural-only children appear under unresolvable component wrappers, and (2) adding a classic Ember “by-name” resolver for .hbs templates that maps PascalCase component invocations to their addon template roots found under node_modules.
Changes:
- Add heuristic per-Source suppression of
element-permitted-contentfor unresolvable wrappers containing content-restricted structural children. - Add
lib/classic-resolver.tsand wire it into the.hbstransform path to resolve PascalCase component tags vianode_moduleslookup. - Add integration fixtures/tests covering the classic by-name resolution and multi-level yield-chain suppression behavior.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| transform.ts | Parses .hbs templates to build classic component tag/attr maps and passes them into blankTemplateContent. |
| blank.ts | Extends detectStructuralYieldRules to suppress element-permitted-content for certain unresolvable-wrapper patterns; adds helper + tag list. |
| lib/classic-resolver.ts | Implements a .hbs-only by-name classic resolver scanning node_modules for addon component templates and extracting their splatted-root tags/attrs. |
| test/integration.test.ts | Adds end-to-end regression tests for classic by-name resolution and heuristic suppression in yield-chain cases. |
| examples/multi-level-yield-chain-options.gts | Adds a fixture demonstrating the multi-level yield-chain <option> FP pattern. |
| test/glint-fixtures/classic-resolver-no-import.hbs | Adds a .hbs fixture consuming a PascalCase component without JS imports to validate by-name resolution. |
| test/glint-fixtures/node_modules/classic-card-addon/package.json | Adds a fake addon package used by the classic resolver fixture. |
| test/glint-fixtures/node_modules/classic-card-addon/addon/components/classic-card.hbs | Adds the fake addon's component template root (<li ...attributes>{{yield}}</li>) used for resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two `.fails()`-marked tests + fixtures pinning down the limitations this PR explicitly accepts: 1. heuristic-masks-real-bug.gts — Per-Source `element-permitted-content` suppression. The same template has an unresolvable wrapper with structural children (correct suppression target) AND a real `<p><div></div></p>` violation that html-validate would normally catch. Whole-Source suppression masks the real bug. Asserted as `.fails()` so future yield-chain- precise resolution surfaces the win. 2. namespaced-classic-resolver.hbs — `<Forms::TextInput>`-style namespaced classic-Ember invocations. The by-name resolver currently handles single-segment kebab names only (`<EsCard>` → `es-card.hbs`); doesn't parse `::` separators or probe nested addon paths like `addon/components/forms/ text-input.hbs`. `.fails()`-tagged: when namespaced resolution lands, vitest signals "remove .fails — your fix worked". Both tests use the same `.fails()` mechanism the codebase has used elsewhere (see PR #17's deferred-fix patterns) — the suite passes today (asserted-fail = pass), the test passes for real when we narrow / remove the limitation, vitest tells us so we update the marker. 160/160 passing tests + 2 expected-fail = 162 total.
…isely Verifies the gating in `containsContentRestrictedStructuralChild` defers to Glint when it has a precise resolution. Without this gating the heuristic over-suppresses: a real `<th>`-under-`<select>` violation inside `<C.Options>` (resolved to `<select>` via PR #18) would be silenced. Lives under `test/glint-fixtures/` so the local tsconfig wires up Glint type extraction; `examples/` has no tsconfig and Glint stays disabled there, which would mask the gate.
johanrd
added a commit
that referenced
this pull request
May 8, 2026
Net -228 findings across three targets — exactly the `element-permitted-content` FP class PR #21 targets: ember-website 125 → 31 (-94 ; e-p-c 99 → 1) hds-design-system 282 → 162 (-120; e-p-c 172 → 52) discourse 446 → 432 (-14 ; e-p-c 99 → 85) ember-website surfaces 4 new findings (element-required-attributes, no-implicit-close) that were previously masked by the per-Source suppression — real signals becoming visible. Same gating-works trade-off documented by `glint-resolved-no-suppression.gts`: when Glint resolves precisely the heuristic stays out of the way. Other 9 targets unchanged — they don't use the curried-yield or classic-by-name patterns the PR fixes.
When the classic-by-name resolver substitutes `<MyImg>` to `<img>`
because the addon's template has `<img src={{this.src}} ...attributes />`,
the consumer's narrow Glimmer-attr blank slots (`@src="…"`) can't fit
the projected `src=' '` placeholder via tryInjectComponentAttrs's
source-side rewrite, and `element-required-attributes` FP-fires.
Mirror PR #13's narrow-slot fix: when resolved=='img' and the addon's
attrCtx records `src`/`alt` (literal OR mustache-bound), push the
consumer's offset to `imgSplatOffsets`. The processElement hook then
calls setAttribute at parse time with a DynamicValue, sidestepping
source-width entirely.
Caught by ecosystem CI on PR #21 baseline diff: ember-website's
`<ResponsiveImage @src="…" alt="" />` started FP-firing because the
new by-name resolver mapped it to `<img>` but didn't carry the
mustache-bound `src` from the addon's template through to the
consumer-side substitution.
C1: drop unused `preprocess` import from lib/classic-resolver.ts.
`verbatimModuleSyntax` would otherwise preserve it in emitted JS.
C2: rewrite the cache-comment block in lib/classic-resolver.ts to match
the actual key shape — `(consumer-dir, kebab-name)`, not
`(node_modules-root, kebab-name)`. Drop the misleading "(tests)"
note about `_clearClassicResolverCache`; record it as exported for
manual reset if needed.
C3: docstring for `buildClassicComponentTagMap` claimed it walks
"PascalCase / dotted invocation". The matcher excludes dotted
(`/^[A-Z][A-Za-z0-9]*$/`). Comment now says single-segment
PascalCase, with examples of what's excluded.
C4: comment in `containsContentRestrictedStructuralChild` claimed only
fully-unresolved wrappers trigger suppression. The code also falls
through on `'transparent'`. Comment now states both cases match the
children-check path.
C5: `.hbs` classic resolver in transform.ts called `preprocess(data)`
directly; `blankTemplateContent` first runs
`stripBlockParamTypeAnnotations` to handle `as |x: T|`. A typed-
block-param `.hbs` would have silently lost classic resolution.
Export the stripper from blank.ts and apply it before the classic-
resolver parse so both paths agree. Practically rare (typed params
are a `.gts/.gjs` convention) but keeps the two paths consistent.
C6 (blank.ts): `containsContentRestrictedStructuralChild` descended
into BOTH arms of every `{{#if}}/{{else}}` regardless of which
branch the current multipass pass emits. A structural child living
in the inactive arm could trigger Source-wide
`element-permitted-content` suppression and mask a real violation
in the active arm. Pass `branchSelections` through and use
`selectBranch` (mirrors the outer `detectStructuralYieldRules`
walker) so only the chosen arm contributes to suppression
decisions.
C7 (lib/classic-resolver.ts): probe order for addon component
templates differed from `lib/glint.ts:resolveAddonHbsTemplate`
(`addon/templates/components` → `addon/components` → `app/components`
here vs `addon/templates/components` → `app/components` →
`addon/components` there). Aligned the classic-by-name resolver to
the existing import-based order so a `.gts` consumer and `.hbs`
consumer of the same addon resolve to the same template.
When `resolveComponentElement` returns 'transparent' (the component
declares `Element: HTMLElement` bare, or the type chain didn't
propagate cleanly across a barrel re-export so Glint can't pin a
specific tag), we previously left the component in 'transparent' and
let children float to whatever native ancestor exists in source.
That's the right call when we have no better information — but for a
TOC whose `<template>` literally writes `<li ...attributes>` /
`<button ...attributes>` / etc., we already extracted the splatted
root in `componentAttrMap`. Use that tag instead. The runtime DOM
matches the template-root tag, so the substitution is at least as
accurate as 'transparent' and lets `element-permitted-content`
validate the correct parent context.
Documented limitation (`leaf-element-under-list-wrapper-consumer.gts`,
`.fails()`): when a component declares `Element: HTMLAnchorElement`
(or any leaf-interactive type) but its template wraps the anchor
inside a non-leaf wrapper (`<ListItem><a ...attributes>{{yield}}</a></ListItem>`),
Glint resolves to the leaf tag (`<a>`); our substitution puts `<a>`
directly under the consumer's `<ul>` even though the runtime DOM is
`<ul><li><a></a></li></ul>`. Fixing that needs recursive cross-file
template walking — deferred.
Updated `does NOT resolve generic HTMLElement to <abbr>` test:
component template root is `<div>{{yield}}</div>`, so the resolution
now lands on 'div' (more accurate) instead of 'transparent' (the
PR #12-era conservative fallback).
…ions
When a component declares `Element: HTMLAnchorElement` (or any leaf-
interactive type), Glint's TS-only resolution gives us the LEAF tag
('a'). But if the component's `<template>` wraps that anchor inside
another wrapper:
<template>
<ListItem>
<a ...attributes>{{yield}}</a>
</ListItem>
</template>
…then the OUTERMOST runtime element is whatever `<ListItem>` renders
(`<li>` in this case). A consumer placing this component under `<ul>`
gets `<ul><li><a></a></li></ul>` at runtime — legal — but our
substitution puts `<a>` directly under `<ul>` and
`element-permitted-content` FP-fires.
The new `lib/outer-wrapper-resolver.ts` walks the component's template
AST to find the OUTERMOST ElementNode. If native, returns its tag.
If PascalCase, resolves the local import (relative-path resolver in
the same module) and recurses. Cap depth + cycle detection.
`lib/glint.ts` calls it after Glint's resolveComponentElement and
overrides the resolved tag when the outer wrapper differs and is a
native tag. Single-substitution trade-off: the inner-content
semantics (`<button>`-under-`<a>`) are lost on the consumer's lint
pass for these wrappers; the addon's own template lint catches them
on its side. Outer-context FPs are the dominant pattern, so the
trade-off favors the wrapper.
Verified impact on real HDS files (within-package imports):
app-footer/legal-links.gts: 5 → 0 element-permitted-content
app-side-nav/list/index.gts: clean
Limitation (still not handled): cross-package barrel imports — when
the consumer imports through `@hashicorp/.../components`, TypeScript's
symbol resolution doesn't always reach back to the source `.gts`
file (depends on the package's exports + declarations layout). My
local-import-only resolveComponentImport doesn't bridge package
boundaries either. That's a separate import-based fallback project.
When a consumer imports a component through a cross-package barrel
re-export (`import { X } from '@scope/pkg/barrel'`), Glint's TS symbol
resolution often doesn't reach back to the source `.gts` file through
the package's compiled declarations / pnpm-linked layout. The same-
package outer-wrapper override (commit 9d2848f) sits inside the
declFile-based code path and never gets a chance to run for those
imports.
The new import-based fallback bypasses TS:
1. Look up the consumer's `import` statement for the component name
(regex on the file's source — both default and named imports,
handling `as`-aliasing in either direction).
2. Resolve the import path:
- Relative imports: walk filesystem from the consumer's dir.
- Package imports: walk node_modules upward, prefer `src/<sub>`
source files over compiled `dist/`, fall back to common
package shapes if `src/` isn't shipped.
3. If the resolved file is a `.ts` barrel, parse it for
`export { default as X } from '...'` (or named re-exports) and
follow the re-exported path.
4. Walk the resulting `.gts/.gjs` template AST chain to find the
outermost native ancestor (recursing through nested PascalCase
wrappers via local imports).
The fallback runs only when the same-package override didn't get a
chance (tracked via `sameTransitivePackageOuterRan` flag set inside
the declFile branch). Override condition is the same: outer wrapper
differs from current resolution AND is a native tag.
Earlier draft gated the fallback on a hardcoded `LEAF_INTERACTIVE_TAGS`
set; dropped that — the override-only-when-different condition is
already principled, and the `sameTransitivePackageOuterRan` flag
keeps the fallback from running redundantly for cases the same-
package path already handled.
Verified impact on real HDS files (after building HDS so the
package's `src/` is reachable through the linked node_modules):
showcase/.../base-elements.gts: 51 → 0 element-permitted-content
app-side-nav/.../with-generic-content.gts: clean
30-file sample across HDS components: 2 e-p-c remaining
New regression test (`cross-package-barrel-consumer.gts`) exercises
the full chain: barrel re-export → recursive template walk →
outer-wrapper override.
C8 (blank.ts + transform.ts):
`tryInjectImgRequiredAttrsViaHook` recorded the element offset in a
single `imgSplatOffsets` list, and the downstream `processElement`
hook injected BOTH `src` AND `alt` for any registered offset. A
component template that binds only `src={{this.src}}` (common) would
silently get an `alt=DynamicValue` injected, masking `wcag/h37`
(missing alt) when the consumer forgot to pass an alt.
Split into per-attr offset lists: `imgSplatSrcOffsets` /
`imgSplatAltOffsets`. Each path (`tryInjectImgRequiredAttrs` for
native `<img ...attributes>` and `tryInjectImgRequiredAttrsViaHook`
for component-substituted `<img>`) registers only the attrs it can
guarantee at runtime. The hook injects per-attr.
Native `<img ...attributes>` still registers BOTH (the splat is the
contract — we can't statically know what the parent passes). The
component-substituted path registers only what's in the addon's
`attrCtx.attrs`. Updated 4 tests in `test/blank.test.ts`; new test
asserts the per-attr precision (consumer wrote `src=` literally,
alt is missing — `imgSplatAltOffsets` records the offset,
`imgSplatSrcOffsets` does not).
C9 (lib/classic-resolver.ts):
Custom `isLowercaseHtmlTag` filtered to lowercase-ASCII tag names.
This rejected mixed-case SVG roots (`linearGradient`,
`clipPath`, …) that `lib/glint.ts:resolveAddonHbsTemplate` accepts
via the shared `isNativeTag`. Fix: import `isNativeTag` from
`blank.js` and use it in classic-resolver. The earlier comment
about avoiding circular imports was speculative — blank.ts doesn't
import classic-resolver (the dep flows transform.ts →
classic-resolver), so direct import is safe.
C10 (lib/classic-resolver.ts):
`findClassicComponent` iterated `fs.readdirSync` results in
filesystem order, which varies across OS / filesystem and would
return different addon templates for the same kebab name on
different platforms. Sort entries (and scoped-package entries) by
name before probing so the resolver is deterministic regardless of
filesystem.
Fix-busts a class of `aria-label-misuse` (and assorted other
attribute-dependent rules) FPs caused by chained component wrappers.
A component declares `Element: HTMLAnchorElement` (Glint reads → 'a');
its template wraps the anchor in another component whose template
renders `<a href={{@href}}>`; the consumer places it under a parent
that's fine with anchors-with-href but not with anchors-without-href.
Pre-fix: we extracted attrs only at the IMMEDIATE template level
(non-native PascalCase wrapper) and substituted the leaf tag without
the `href` from the inner native — html-validate then fired
`aria-label-misuse` on a "non-interactive `<a>`" that's actually
interactive at runtime.
Two changes:
1. `findOutermostElement` now descends through `BlockStatement`
bodies (e.g. `{{#if @route}}<X>{{else if @href}}<a>{{/if}}`) and
skips dotted/slot-named elements as wrapper candidates while
continuing to search for a usable one. Without this, HdsInteractive-
style templates whose top-level is conditional would fail to walk
into either branch.
2. `OuterWrapperResolution` now carries `attrs` and `hasSplat` UNIONED
from each level of the chain (inner wins on conflicts — closer to
the rendered DOM). `lib/glint.ts` overrides `componentAttrMap`
with the chain's accumulated attrs, not just the top-level
splatted-root descriptor. The runtime `<a>` carries `href` (from
the inner native) AND `aria-label` (from the outer wrapper's
invocation), so html-validate's role-validity checks pass.
Verified impact on real HDS files (with HDS built so source `src/` is
reachable through pnpm-linked node_modules):
aria-label-misuse: 38 → 5 (33 cleared; remaining are real
"strictly allowed but not
recommended" stylistic warnings)
element-permitted-content: 172 → 11 (161 cleared; chain-attr
collection helps here too —
parent-context checks see the
outer wrapper instead of the
leaf)
Sampled 5 cleared findings to confirm they were FPs:
- HdsButton invocations resolved to `<a>` without href; runtime
DOM is `<button>` (no @href branch) or `<a href>` (with @href) —
both interactive, both allow aria-label.
- HdsAppFooterLink under `<ul>`: runtime is
`<ul><li><a></a></li></ul>` (HdsAppFooterLink wraps `<a>` in
HdsAppFooterItem which renders `<li>`) — fully legal.
New regression test (`conditional-leaf-href-consumer.gts`) asserts
the chain-attr collection at the AST level: a wrapper whose template
contains a top-level `{{#if @href}}<a href={{@href}}>{{else}}<button>`
bubbles `href` up so consumer-side substitutions register it.
Exported `literalAttrs` and `elementHasSplat` from
`lib/component-attrs.ts` so the resolver can read attrs off any
element node directly (rather than going through the splatted-root
helper which has its own selection logic).
Add a note in Known Limitations: the rule fires on every untyped `<button>` regardless of `<form>` ancestry, per html-validate's strict design. We don't try to soften it. Users who only want inside-form buttons flagged can disable the rule project-wide. Static `<form>`-ancestry detection is feasible but inherits the same per-Source-suppression caveat as wcag/h32 (a button inside a wrapper component that someone else's template wraps in `<form>` would be silenced in the wrong direction). Investigated ecosystem findings (HDS 12 cleared by chain-attr collection — pre-fix wrong- tag substitutions; discourse 20 are real per the rule).
A `<form>` whose submit button is provided by a component the static
analyzer can't pin to a native tag (no Glint Element annotation, not
in node_modules, not a builtin, dynamic-element template like
`<this.wrapperElement type={{...}}>`) used to FP-fire wcag/h32 because
the blanker couldn't see any submit candidate. Common in real Ember
codebases that use button-style component wrappers from non-addon
packages or dynamic-element addon components.
Extend `elementYieldsAndLacksSubmit` (PR #17) to also recognize
"form contains unresolved PascalCase / dotted component" as
"may contain submit." Per-Source suppression — same trade-off as the
yield-bearing-form case (real bugs at OTHER locations in the same
template get suppressed too), acceptable given the FP volume.
A previously-`.fails()` test (`namespaced-classic-resolver.hbs`)
now passes for real: the namespaced-component support is still missing
as a feature, but the unresolved-component heuristic catches the same
wcag/h32 FP. Updated the test's narrative.
Verified ecosystem impact:
discourse wcag/h32: 41 → 10 (31 cleared)
HDS wcag/h32: 5 → 2 (3 cleared)
Other rules: unchanged (no regressions on
element-permitted-content, aria-label-misuse,
no-implicit-button-type)
Heuristic detail: when iterating a form's children, an ElementNode
is treated as an unresolved component when it's PascalCase / dotted
AND not a builtin AND either has no entry in `glintComponentTagMap`
or has an entry of `'transparent'`. The `'transparent'` case matters
because Glint may resolve a component to a generic-HTMLElement-style
"transparent" tag that doesn't tell us what runtime tag actually
renders.
C11 (lib/outer-wrapper-resolver.ts): `preprocess(block.contents)` lacked both `stripBlockParamTypeAnnotations` and `mode: 'codemod'`. A `.gts` template using TS-flavored block- param annotations (`as |x: T|`) would throw, the resolver would silently skip the block, and the FPs this resolver is meant to fix would re-surface. Apply the same preprocess preamble blank.ts uses. C12: drop unused `traverse` import from `lib/outer-wrapper-resolver.ts` (the file uses only `preprocess` — `traverse` was left over from an earlier draft). C13: rewrite the docstring for `resolvePackageImport` in `lib/outer-wrapper-resolver.ts`. The old text claimed the function consults `package.json` `exports` / `main`, which it never has — the implementation only probes `src/<sub>` and a few bare paths. New docstring lists the four probe shapes explicitly. C14: add an `isEmberAddonPackage` pre-filter to `lib/classic-resolver.ts`. `findClassicComponent` previously did 3 `existsSync` probes per package per PascalCase tag, even for packages that obviously don't ship classic component templates (anything that's not an Ember addon). Skip non-addon packages by reading `package.json` `keywords: ['ember-addon']` or `ember-addon` field once, cached per-package. Reduces IO in large `node_modules` while preserving the resolver's behavior. C15: update the comment on the `classic-resolver-mustache-bound-attrs.hbs` test to reflect the per-attr offset sets (`imgSplatSrcOffsets` / `imgSplatAltOffsets`) that replaced the original single `imgSplatOffsets` list. Verified 168/168 tests pass; HDS / ember-website ecosystem counts unchanged from previous commit (no regressions from the addon-pre- filter).
…e tags Encodes the HDS form-layout containers shape: <HdsForm as |FORM|> (conditional <form>/<div> → TRANSPARENT) yielding a hash with siblings that resolve to DIFFERENT native tags — HeaderTitle through a class-getter+chain to <div>, HeaderDescription through a literal @tag="p" to <p>. Regression guard against the resolver slipping back to a single-tag fallback (e.g. Glint TS-side picking HTMLHeadingElement → <h1>) that would mask both resolutions.
…efault re-exports + fileURLToPath
- template-source.ts findTemplateSource: cache key now includes
componentName + declRange so repeated invocations of the same
component during a single extraction reuse the parsed template
instead of re-reading + re-parsing on every call. Multi-template
targets keep their disambiguation because the name/range hints
separate cache entries that point at the same file but resolve to
different `<template>` blocks.
- template-source.ts findFromImport: propagate componentName when
following `export { default as Y } from './path'`. The barrel-side
alias still helps readGts pick the right `<template>` in multi-
template targets where the inner declaration shares that name (the
common HDS-style barrel pattern). Renamed-default cases fall back
to the previous behavior.
- test/blank.test.ts, test/resolver/walk.test.ts,
test/resolver/template-source.test.ts: switch FIXTURES from
`new URL(...).pathname` to `fileURLToPath` + `path.join` for
cross-platform consistency with test/glint.test.ts.
The <a> href hook-time fallback comment previously pointed to the old 'imgSplatSrc/imgSplatAlt offset sets' shape; the implementation has since unified those into the attrInjections registry shared with the <img> src/alt and <button> type fallbacks. Update the comment to match the current mechanism.
When applyResolution picks a yield-ancestor inner element over the
component's outer wrapper (e.g. <HdsBreadcrumb> resolved as <ol>
rather than <nav> so yielded <li> children land under the correct
structural parent), the consumer's attributes still splat onto the
outer at runtime via ...attributes. The in-place block-form rename
copies the consumer's open-tag attributes onto the substituted tag,
so 'aria-label="X"' lands on <ol> in the blanked output even though
at runtime it's on <nav>. html-validate then fires aria-label-misuse
('allowed but not recommended on <ol>') against an element that
doesn't carry the attribute in practice.
Track whether yield-ancestor preference fired (ComponentAttrs.
fromYieldAncestor) and, when it did, blank aria-* attributes from
the substituted open tag. Scoped to aria-* because ARIA-in-HTML rules
differ per host element; other attrs (id, class, role, data-*)
generally validate the same against the outer and the yield-ancestor.
The fromYieldAncestor aria-strip path added [startOffset, endOffset]
to blankRanges so the substituted open tag's aria-label region became
whitespace. But the downstream handleElementNode → emitAttribute loop
re-processes every attribute regardless, and for a ConcatStatement
value (multi-line aria-label with `{{if ...}}` interpolation) it
emits `"…"` quote renames at the value boundaries — those quotes
land in the blanked region, producing un-tokenizable open tags like
`<ol " " >` that html-validate's parser rejects.
Push the stripped range into fullyBlankedRanges too, and add an
early-return at the top of emitAttribute that skips attributes
already inside a fully-blanked range. The early-return is general:
any future path that fully-blanks an attribute up-front (component
substitution, attribute injection, etc.) now correctly suppresses
emitAttribute's value-rewriting for that attribute.
These are local AI-assistant triage artifacts (Copilot review state saved by an authoring workflow); they don't belong in the repo and were committed by accident in da39e08.
When a component's OWN template contains
`{{#let (element this.titleTag) as |Tag|}}<Tag …>…</Tag>{{/let}}`
and extractAttrTypeMap runs against that file (the component is its
own consumer), `<Tag>` is a let-block-param whose declaring file IS
the consumer. The canonical resolver bails because declFile isn't a
top-level declaration, and Glint's TS-side falls back to the first
matching member of the (element …) helper's return-type union —
typically <h1> for HTMLHeadingElement when the union includes
heading tags. Downstream `element-permitted-content` FP-fires on
legal <div>/<span> content under what html-validate now thinks is
an <h1>. Real-world example: HdsDialogPrimitiveHeader at
hds-design-system, which uses this pattern with `get titleTag()`
returning `this.args.titleTag ?? HdsDialogPrimitiveHeaderTitleTagValues.Div`.
buildConsumerInfo now tracks `{{#let (element this.X) as |T|}}`
bindings on its scope stack and records each `<T>` PascalCase
element's location alongside the `propName` it depends on. After the
TS-side walk completes, extractAttrTypeMap walks the consumer file's
own class body via resolveThisProp (mirroring the canonical resolver's
`(element this.X)` path) and overrides Glint's TS-side fallback with
the getter's literal return value (typically 'div'). Scoped to
`(element this.X)` — `(element @arg)` at the own-template level
needs a caller's consumerArgs and isn't resolvable from the file in
isolation; literal-string forms are already handled by Glint's TS
side (return type is a string-literal, not a union).
Recurring slip: the copilot-review skill writes triage state to .claude/copilot-reviews/pr-N.md, which is purely a local workflow artifact and shouldn't be tracked. Add .claude/ to .gitignore so future commits don't sweep it in via 'git add -A'.
…dArgs caching
- buildConsumerInfo: dotted-invocation capture moved outside the
PascalCase gate. Glimmer accepts lowercase block-param dotted
invocations (`<Outer as |o|><o.Section>` parses to ElementNode
tag='o.Section'); the consumer-walker now records these too. The
PascalCase-only gate is kept for non-dotted PascalCase invocations
(argsByLoc and let-element-binding lookup).
- walkMapping: the matching PascalCase gate on Glint's transformed
spans is relaxed the same way — dotted invocations are processed
regardless of head casing. HTML elements remain filtered out (they
don't contain `.`).
- Dotted-invocation resolution path now merges three arg sources, in
increasing-priority order, when calling resolveYieldHashBinding:
1. binderArgs from the outermost binder (`<Binder @x="y" as |S|>`)
2. curriedArgs collected at prior hops (e.g.
`(component Inner size="300")` from a parent's yield-hash)
3. invocation args on the dotted call itself
(`<S.Step @tag="li">`)
Invocation args win (the consumer set them most directly). Without
step 3, literals on the dotted invocation were silently dropped —
the inner polymorphic's getter default would win instead.
- binderSourceByKey cache now persists both TemplateSource AND any
curriedArgs collected via resolveYieldHashBindingSource at that hop.
Multi-level chains (HDS form-layout's
`<HdsForm as |F|><F.Section as |FS|><FS.Header as |FSH|>
<FSH.Title>`) now propagate curry literals across hops; previously
curriedArgs was dropped at the cache write, so later hops lost
literals from earlier (component …) curries.
- Drop unused `traverse` import from `@glimmer/syntax`.
Test coverage: lowercase-block-param-dotted-consumer.gts fixture
covers the casing relaxation; an inline fixture for `<P.Title @tag="li">`
covers invocation-arg propagation (assertion targets the specific
dotted-invocation position rather than the whole tag set).
Both gates that previously combined `/^[A-Z]/.test(tag) || tag.includes('.')`
(buildConsumerInfo and walkMapping) now use `!isNativeTag(tag)`.
Reuses the existing HTML5-tag whitelist from blank.ts. Mirrors
Glimmer's internal `isComponent` predicate at
@glimmer/syntax/lib/v2/normalize.ts:818 — not exported from the
package, and ember-eslint-parser similarly carries its own local
copy at src/parser/transforms.js:76. The whitelist approach is
slightly stricter and correctly classifies edge cases the casing
heuristic missed (lowercase let-element bindings, custom-element
names without dashes, etc.); the resolver bails gracefully when a
non-HTML tag has no import or binding behind it.
Test coverage already exercises the relevant paths
(lowercase-block-param-dotted-consumer, own-template-let-element,
the full PascalCase chain).
The previous swap from `/^[A-Z]/ || .includes('.')` to `!isNativeTag(tag)`
was too liberal: it also accepted named-block slots (`<:body>`,
`<:header>`) and `@arg`-bound tags (`<@foo>`) as component
invocations. The resolver then walked their substitution, which
shifted the blanker's output byte layout enough to move some
downstream findings' line numbers by ~5 lines and produced spurious
+/- pairs in the ecosystem diff.
Add `isComponentTag(tag)` to blank.ts that mirrors Glimmer's internal
`isComponent` predicate (normalize.ts:818, not exported):
- excludes known HTML5 tags via isNativeTag
- excludes named-block slots (':'-prefixed)
- excludes arg-bound tags ('@'-prefixed)
- accepts everything else (PascalCase, dotted, let-element bindings)
Used by both buildConsumerInfo and walkMapping. The predicate is now
defined and exported from blank.ts alongside isNativeTag.
- test/cache.test.ts: replace `require('node:crypto')` (with its
eslint-disable) with an ESM `import crypto from 'node:crypto'`.
vitest currently runs `require` for node built-ins via a shim
inside ESM tests, but that's not portable and contradicts the
repo's "type": "module" — using a real ESM import is the right
way to call crypto here.
- lib/resolver/template-source.ts findFromImport: when following
`export { default as Y } from './path'`, recurse back into
`findFromImport` (with `componentName` as the alias) instead of
dropping into `findFromDecl` directly. `findFromDecl` handles the
single-hop leaf case (.gts/.gjs/.d.ts) but doesn't walk barrel
chains. Routing through `findFromImport` lets multi-level barrels
(HDS-style index.ts → components-barrel.ts → leaf.gts) keep
resolving — its first step still tries `findFromDecl` (covers the
leaf case), and its loop walks the re-exports when `next` is
itself a barrel.
On a first run, the Glint summary reported Glint: 311 analyzed, 3 from cache even though the cache was empty for every file — those 3 were `.gts` files with no `<template>` block (rewriteEmpty). We write a tombstone for them AFTER the run so they show up as literal cache hits on the NEXT run, but at summary-print time none of them are actually cached. Tighten the message to report only real disk-cache hits in the "from cache" number. When some `.gts` files had no template, surface the count in a parenthetical instead of folding it into "from cache". Read / rewrite errors continue to surface via HVE_DEBUG=1 only. before: Glint: 311 analyzed, 3 from cache after: Glint: 311 analyzed, 0 from cache (3 .gts files had no <template>)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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.
Summary
Originally opened as a two-thread fix (heuristic suppression + classic-by-name resolver). Now substantially larger: this branch consolidates ~50 commits of FP work driven by ecosystem-CI baselines, and the centrepiece is a canonical resolver rewrite (
lib/resolver/walk.ts) that collapses six prior resolution paths into one ordered algorithm. Most subsequent commits are cluster fixes layered on top of that rewrite.What's in this PR
Canonical resolver (
lib/resolver/walk.ts+lib/resolver/template-source.ts)One algorithm with a clear contract — template AST + consumer args →
Resolution. Replaces the scattered logic inouter-wrapper-resolver,component-attrs's polymorphic/extract paths, and several leaf-fallback helpers. Ordered passes:{{#let (element X) as |Tag|}}<Tag>…</Tag>{{/let}}→ trace X (literal |@argpropagation |this.<prop>via class getter).@args.{{#if}},{{#unless}}) → evaluate the condition statically against consumer args / class getters and pick the matching branch; converge when both branches resolve to the same tag.Cross-package template source resolution is in
lib/resolver/template-source.ts(.gts/.gjs/.hbs/.d.ts/.js-aware, followsexports-map types→default companions,.d.ts → .gtsfallback for HDS-style packages, compiled-JS extraction viaprecompileTemplate/template).Class-getter walking (
analyzeGetterBody)For
(element this.prop)and@arg={{this.prop}}passthrough — walks the class declaration in TS, extracts top-level enum members, follows shallow relative imports for enum sources, and recognises four return shapes:const { tag = 'div' } = this.args; return tag;const { tag = DEFAULT_TAG } = this.args; return tag;(top-level const → string)const { tag = EnumName.Div } = this.args; return tag;(enum member)return this.args.X ?? Default;(RHS = string literal, top-level const, orEnumName.Member)Conditional evaluation against consumer args
{{#if (eq @tag "li")}}<li>…{{else}}<div>…{{/if}}and the(eq this.componentTag "div")variant —resolveConditionalevaluates the predicate against the consumer's@argmap / class-getter walk and picks the matching branch instead of walking both and falling to transparent.Polymorphic-tag chain through compiled JS
extractCompiledJsparses an addon's compiled.jsto pull the template source out ofprecompileTemplate(…)/template(…)(RFC 0931). Works on v2-spec addons that ship only compiled JS +.d.ts. Cross-package.d.ts → .gtsmapping is gated behindresolveGtsPathForPolymorphicso it only fires for the polymorphic chain — earlier whole-pipeline mapping caused ~397 HDS over-resolutions.Cache key includes plugin source SHA (
lib/cache.ts)The disk cache previously keyed on
(pluginVersion, tsconfigSha, fileSha). Local plugin edits don't bump the packageversion, so stored entries silently masked resolver fixes across every dev iteration. NewpluginSourceShahashes the plugin's ownlib/tree once at module load and invalidates the cache when plugin code changes. Includes a regression test (test/cache.test.ts).Multi-level dotted-binder chain (
resolveYieldHashBindingSource)HDS form-layout pattern:
<HdsForm as |FORM|><FORM.Section as |FS|><FS.Header as |FSH|><FSH.Title>. The deepest invocation's binder (FS.Header) is itself a dotted tag —findTemplateSourcecan't resolve dotted names, so resolution previously broke one hop early and Glint's TS-side union pick won (landing on<h1>fromHdsTextDisplay's element union and FP-firingelement-permitted-contentfor legal<div>content underneath). Each level's resolvedTemplateSourceis now cached under the dotted invocation's own key inbinderSourceByKey, so subsequent levels chain through directly without re-walking from the importable root.Pure-yield wrapper descent
HDS
<HdsDropdown>'s template begins with<HdsPopoverPrimitive as |PP|>{<div>…<ul>{{yield (hash Interactive=…)}}</ul>…</div>}</HdsPopoverPrimitive>. The outer is<HdsPopoverPrimitive>— a pure-yielder whose own template body is just{{yield (hash …)}}. Previously the canonical resolver returned transparent on that recursion, the substitution dropped HdsDropdown's<div>/<ul>wrapper, and yielded<li>items (D.Interactive) appeared as siblings of the consumer's outer<li>(SF.Item) — FP-firingno-implicit-close+close-order. NewisPureYieldWrappercheck +findPascalWrapperSourcehelper: when a PascalCase wrapper's template body has no element producers of its own (only{{yield (…)}}), recurse into the invocation's own children for the real DOM outer.Smaller resolver-side fixes (each landed with its own test fixture)
applyResolution— yield-ancestor preference only fires when both the outer wrapper AND the yield-ancestor are NOT structural-child-only (thead/tbody/tr/li/option/…). Otherwise the substitution keeps the outer wrapper.applyResolution—transparentfrom the canonical resolver overrides any TS-side union-pick (e.g. HdsInteractive'sHTMLAnchorElement | HTMLButtonElement— TS picks the first; the canonical resolver returns transparent for the conditional, and that override eliminates the FPs cascading from the wrong branch).resolveElementHelperLet—hasSplatdefaults tofalse(nottrue) when the inner<Tag>element can't be located in the let-block body.resolveBinding— recognisesSubExpression(component PathExpression …)curried calls in yield-hash entries (e.g.Title=(component HdsFormHeaderTitle size="300")), unwraps and merges curried@arg="literal"pairs intoconsumerArgs.<S.Step>from<Parent as |S|>blocks resolves through the parent's{{yield (hash Step=…)}}chain (including class-property indirection).<details>element-required-content: suppress on self-closing component invocation that resolves to<details>.setAttributefor substituted<button>+ multi-template root match.<a>.isLiteralSafeForAttrrejects theDynamicValueplaceholder.Classic-Ember by-name resolver (
lib/classic-resolver.ts).hbsconsumers (no JS imports, no Glint): kebab-case PascalCase tag names, probe canonical addon template paths undernode_modules(addon/templates/components/<name>.hbs,addon/components/<name>.hbs,app/components/<name>.hbs). Handles pnpm-style symlinks.Original threads from this PR's first scope
detectStructuralYieldRules: addelement-permitted-content+element-permitted-parenttodisableForRuleswhen a non-native wrapper contains content-restricted structural children AND Glint hasn't pinned the wrapper precisely.glintComponentTagMapresolves the wrapper to a native tag, so real violations still fire (test/glint-fixtures/glint-resolved-no-suppression.gtscovers this regression).Are the ecosystem-ci regression tests part of this PR?
Yes for the in-repo regression guards — every cluster fixed in this PR landed with a corresponding fixture in
test/glint-fixtures/and an integration assertion intest/integration.test.tsortest/glint.test.ts. 55 new test files / fixtures in this branch alone.The actual ecosystem-ci runner (
ecosystem/run.ts+ baselines) lives on theecosystem-cibranch, not on this branch — it's what surfaces regressions to triage but is not where the regression guards live.Verified impact
@tag="li"under<ul>; HdsTag<div>-under-<span>× 4; HdsFormSelectField<select>-under-<select>; HdsFlyout / HdsModalaria-labelledby; advanced-tableattribute-misuse target/href; rich-tooltip/{options,states,toggle}element-permitted-content× 7; form/radio-cardelement-permitted-content× 4; stepper aria-label; demo-form-{basic,complex}wcag/h32; page-layouts/flex/displayelement-permitted-content.<div>-under-<h1>at 46:14, 49:14, 64:16).<li>implicitly closed by sibling at 333:12, stray</li>at 340:12).+78 -76(down from+87 -78pre-PR). The remaining "added" entries are mostly real TPs the priortransparent-mask was hiding — e.g.<HdsTextBody @tag="p">wrapping block-rendering<RT.Bubble>from HdsRichTooltip (authored HTML5 violation that runtime works around via popover/portal), and pages-header / focus-ring patterns where the showcase intentionally renders multiple<header>landmarks. The genuine remaining FP residue is one cluster (dialog-primitive<Tag>let-block-binding — see "What's NOT in this PR").Test plan
pnpm test— all 233 unit/integration tests passpnpm run typecheck:tests— clean (after fixingas typeof TSpattern in new blank.test.ts cases)(element)helper, polymorphic chain, classic-by-name, etc.)Trade-offs
heuristic-masks-real-bug.gtsdocuments this with a.fails()test). Acceptable given the volume of FPs in baselines.pluginSourceShaadds ~50ms one-time at module load (readslib/files once per process).{{#each}}adjacent to non-each siblings: multipass currently treats{{#each}}as non-branching (preserves iteration body); when an each-block sits next to a non-each sibling that produces the same tag, html-validate may emitno-implicit-closefor the iteration's end (open follow-up — see HDS app-header showcase).What's NOT in this PR
{{#let (element this.titleTag) as |Tag|}}<Tag>in HDSdialog-primitive/header.gts—<Tag>still goes through Glint's TS-side union pick, landing on the first element-type member (<h1>fromHTMLHeadingElement). Currently produces 2 FPs in dialog-primitive's own file. Architectural fix:extractAttrTypeMapneeds to detect let-block-binding declarations and route them through the canonical resolver's(element …)chain. Open follow-up.ecosystem/runner + baselines themselves (separate branch).