Skip to content

Record arg-bound component attrs as DynamicValue placeholders#15

Merged
johanrd merged 9 commits intomainfrom
fix/fp-arg-bound-required-attrs
May 8, 2026
Merged

Record arg-bound component attrs as DynamicValue placeholders#15
johanrd merged 9 commits intomainfrom
fix/fp-arg-bound-required-attrs

Conversation

@johanrd
Copy link
Copy Markdown
Owner

@johanrd johanrd commented May 7, 2026

Summary

A component template like:

<template>
  <iframe ...attributes title={{@label}} src={{@src}} />
</template>

provides title and src at runtime via consumer-passed @args, but the splatted-root extractor previously skipped non-literal values (skipping MustacheStatement and ConcatStatement) per a "caller can't anticipate the value" comment. Result: html-validate's element-required-attributes FP-fires on the substituted <iframe> because no title or src was injected at the consumer's call site.

This change records those attrs with a 3-space whitespace placeholder. The blanker then injects title=' ' / src=' ' at the consumer's open tag into a Glimmer-only blank slot, and processAttribute's existing DynamicValue conversion (transform.ts:116-122) treats the value as "present, value unknowable" — exactly the right signal for required-attribute rules.

The header comment for lib/component-attrs.ts is updated to reflect both value forms now recorded.

Surfaced by

Ecosystem CI on hashicorp/design-system (39× <iframe> missing title, all from <ShwFrame @label="..." @src="..." /> showcase wrapper).

Test plan

  • New test asserts <iframe ...attributes title={{@label}} src={{@src}} /> records title and src as whitespace placeholders
  • npm test — 128/128 passing
  • npm run typecheck:tests — clean
  • npm run build — clean

A component template like
  <template>
    <iframe ...attributes title={{@Label}} src={{@src}} />
  </template>
provides `title` and `src` at runtime via consumer-passed @Args, but
the splatted-root extractor previously skipped non-literal values
(skipping `MustacheStatement` and `ConcatStatement`) per a "caller
can't anticipate the value" comment. Result: html-validate's
`element-required-attributes` FP-fires on the substituted `<iframe>`
because no `title` or `src` was injected at the consumer's call site.

Record those attrs with a 3-space whitespace placeholder. The blanker
then injects `title='   '` / `src='   '` at the consumer's open tag
into a Glimmer-only blank slot, and processAttribute's existing
DynamicValue conversion (transform.ts:116-122) treats it as "present,
value unknowable" — exactly the right signal for required-attr rules.

Surfaced by ecosystem CI on HDS (39× <iframe> missing title from
`<ShwFrame @Label="..." @src="..." />`).
@johanrd johanrd added the bug Something isn't working label May 7, 2026
@johanrd johanrd requested a review from Copilot May 7, 2026 11:05
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves how html-validate-ember propagates component template attributes to the substituted native element when the component’s splatted root (...attributes) uses arg-bound (non-literal) attribute values. This prevents false positives from html-validate rules (notably required-attribute checks) when required attributes are supplied at runtime via @args.

Changes:

  • Record MustacheStatement and ConcatStatement attribute values on splatted roots as a 3-space whitespace placeholder so downstream processAttribute can convert them to DynamicValue.
  • Add a Glint fixture (typed-iframe.gts) and a test asserting arg-bound required attrs are recorded as placeholders.
  • Update lib/component-attrs.ts module header docs to reflect the new extraction behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
lib/component-attrs.ts Extends splatted-root attribute extraction to record dynamic (mustache/concat) attrs as DynamicValue placeholders.
test/glint.test.ts Adds an integration test covering arg-bound required attrs being recorded for splatted roots.
test/glint-fixtures/typed-iframe.gts Adds a cross-file .gts fixture with <iframe ...attributes title={{@label}} src={{@src}} /> to reproduce the FP scenario.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/glint.test.ts Outdated
Comment thread lib/component-attrs.ts Outdated
…line ref

Per Copilot review on #15:

- Test now asserts the title/src placeholders are ≥3 whitespace chars
  (\s{3,}) instead of just any whitespace (\s+). The 3-char minimum
  is what processAttribute requires to convert to DynamicValue —
  asserting just whitespace would let a regression to 1-2 spaces pass
  silently and stop satisfying required-attribute rules.
- Removed the hard-coded `transform.ts:116` reference from the
  sentinel comment in lib/component-attrs.ts; line numbers drift.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comment thread lib/component-attrs.ts Outdated
Comment thread test/glint.test.ts
Comment thread lib/component-attrs.ts Outdated
…+ centralize sentinel

Per Copilot review on #15:

1. substituteSelfClosingComponent now embeds the splatted-root attrs
   (attrCtx.attrs) in the rewritten <RESOLVED ...></RESOLVED> output,
   not just `type` for buttons. Without this, components whose
   Signature['Element'] resolves to a non-void native carrying
   *required* arg-bound attrs (e.g. <iframe title={{@Label}}
   src={{@src}}>) would emit a bare <iframe></iframe> at the consumer's
   self-closing call site and FP-fire element-required-attributes —
   exactly the FP this PR claims to fix, but only the block-form path
   was actually addressed before.
2. Added end-to-end integration test:
   examples/typed-iframe-consumer.gts uses <TypedFrame /> self-
   closingly, integration test runs the full transform with HVE_GLINT
   enabled and asserts no element-required-attributes fires on the
   substituted iframe. Catches regressions in either the extraction
   or substitution side.
3. Centralized the DynamicValue sentinel in a new lib/dynamic-value.ts
   module: exports DYNAMIC_VALUE_PLACEHOLDER ('   ') and
   isDynamicValuePlaceholder. blank.ts, transform.ts, and
   component-attrs.ts now all reference these — the value and
   threshold can no longer drift independently.

Also: header comment in lib/component-attrs.ts now describes which
substitution paths embed the recorded attrs (block-form, void
in-place, non-void rewrite) so future readers don't expect coverage
of paths that aren't there.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Comment thread lib/dynamic-value.ts Outdated
Comment thread blank.ts
Comment thread blank.ts
Per Copilot review on #15:

1. Updated lib/dynamic-value.ts header comment to drop the reference
   to tryInjectImgRequiredAttrs — that helper lives on a sibling
   branch (#13), not this one. The list now generalizes ("multiple
   injection sites") rather than naming each function.

2. Boolean attrs (`disabled`, `required`, `selected`, `checked`, …)
   now emit presence-only in BOTH tryInjectComponentAttrs and
   substituteSelfClosingComponent regardless of the recorded value
   (literal `''`, the DynamicValue placeholder, or any literal-safe
   string). Per HTML5 any value on a boolean attr is equivalent to
   "true"; emitting `disabled='   '` (or any explicit-value form)
   unnecessarily triggers `attribute-boolean-style`.
   - tryInjectComponentAttrs: changed the gate from
     `literal === '' && isBooleanAttr(...)` to just `isBooleanAttr(...)`.
     This now covers the new arg-bound case (`disabled={{@x}}` recorded
     as the placeholder) which used to fall through to literal-emit.
   - substituteSelfClosingComponent: added the same isBooleanAttr
     branch when emitting splatted-root attrs into the rewritten
     `<RESOLVED ...></RESOLVED>` open tag.

New regression test in blank.test.ts covers the placeholder boolean
case explicitly.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Comment thread transform.ts Outdated
Comment thread test/glint.test.ts Outdated
Comment thread test/blank.test.ts
…ded length

Per Copilot review on #15:

1. transform.ts: comment about the bare-mustache placeholder said
   "3-char whitespace". Reworded to reference the
   `DYNAMIC_VALUE_PLACEHOLDER` / `isDynamicValuePlaceholder` exports
   from `lib/dynamic-value.ts` instead of a fixed length, so it
   doesn't go stale if the sentinel ever changes.

2. test/glint.test.ts: the typed-iframe assertion used a `^\\s{3,}$`
   regex. Switched to `isDynamicValuePlaceholder(...)` — same single
   source of truth `processAttribute` consults, so the test follows
   any future sentinel change automatically.

3. test/blank.test.ts: replaced hard-coded `'   '` literals in the
   test data setup AND in the output-content assertions with
   `${DYNAMIC_VALUE_PLACEHOLDER}` template-literal interpolation.
   Comments and `it()` titles still describe the placeholder
   concretely (3 spaces today) for readability.

130/130 pass. No behavioral change.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment thread lib/component-attrs.ts Outdated
Comment thread lib/component-attrs.ts Outdated
Per Copilot review on #15:

- The header referenced `element-required-attributes-style` — html-
  validate's actual rule is `element-required-attributes` (no
  "-style"). Reworded to "`element-required-attributes` and similar
  required-attribute rules" so a reader searching for either name
  lands on the right place.

- Said `extractSplattedRoot returns:` — that function name doesn't
  exist; the exports are `getSplattedRootsForFile` and
  `extractSplattedRootFromTemplate`. Pointed at the latter.

Comment-only change.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment thread transform.ts Outdated
Comment thread lib/component-attrs.ts Outdated
Per Copilot review on #15:

1. transform.ts comment about `processAttribute` previously suggested
   blank.ts and component-attrs.ts always inject through the
   centralized `DYNAMIC_VALUE_PLACEHOLDER` constant. That's
   misleading: blank.ts also produces variable-length whitespace by
   blanking the original mustache spans (the result has the same
   length as the original `{{x}}` source — variable, can be much
   longer than the sentinel). Reworded to clarify that
   `isDynamicValuePlaceholder` is a length-tolerant predicate
   (matches any whitespace-only value of length >= the sentinel) and
   that only explicit injections use the literal constant.

2. lib/component-attrs.ts header still hardcoded the sentinel as
   "3-space placeholder" / `name='   '` / `value: '   '`. Replaced
   with references to `DYNAMIC_VALUE_PLACEHOLDER` so the docs follow
   any future change to the constant.

Comment-only.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

blank.ts:914

  • substituteSelfClosingComponent now appends all recorded splatted-root attrs into openTag, which increases minLen and can make substitutions fail (sourceLen < minLen) for short invocations. This is a behavioral regression vs the prior implementation (which only added the type attr for buttons) and can reintroduce false-positives when the function returns false and the caller falls back to transparent neutralization. Consider a fallback strategy that still substitutes but drops/limits extraAttrs when the span is too short (e.g. try without extraAttrs, or include attrs until the tag fits).
  // Embed the rest of the splatted-root attrs (other than type, handled
  // above for button). Without this, components whose Signature['Element']
  // resolves to a non-void native carrying *required* attrs sourced from
  // arg-bindings (e.g. `<iframe title={{@label}} src={{@src}}>`) would
  // emit a bare `<iframe></iframe>` and FP-fire
  // `element-required-attributes`.
  let extraAttrs = '';
  for (const [name, value] of Object.entries(attrCtx?.attrs ?? {})) {
    if (resolved === 'button' && name === 'type') continue; // already in typeAttr
    // Boolean attrs emit presence-only; see tryInjectComponentAttrs's
    // matching branch for the rationale.
    if (isBooleanAttr(resolved, name)) {
      extraAttrs += ` ${name}`;
      continue;
    }
    const safeValue = isLiteralSafeForAttr(value) ? value : DYNAMIC_VALUE_PLACEHOLDER;
    extraAttrs += ` ${name}='${safeValue}'`;
  }
  const openTag = `<${resolved}${typeAttr}${extraAttrs}>`;
  const closeTag = `</${resolved}>`;
  const minLen = openTag.length + closeTag.length;
  const sourceLen = elementEnd - elementStart;
  if (sourceLen < minLen) {
    return false;
  }
  const inner = ' '.repeat(sourceLen - minLen);
  ctx.renames.push([elementStart, elementEnd, openTag + inner + closeTag]);
  ctx.fullyBlankedRanges.push([elementStart, elementEnd]);

Comment thread blank.ts
Per Copilot review on #15: the comment block above `const plan = …`
in `tryInjectComponentAttrs` still described the pre-r2 behavior
("empty-string literal on a known boolean attr … emits just the
attr name"). Round-2 of this PR moved boolean attrs to a hard
presence-only emission regardless of recorded value (including the
DynamicValue placeholder), which the inline comment in the .map
arm already documents accurately.

Replaced the stale outer block with a one-liner pointing at the
inline rules. No behavioral change.
johanrd added a commit that referenced this pull request May 8, 2026
Re-merged combined-fp-fixes into ecosystem-ci with all latest fix
tips (PR #13 narrow-slot, PR #15 stale-comment cleanup, PR #17
Thread B + fieldset-with-component, PR #19 native-tag guard,
cache-invalidation-on-source-change). Cleared all target caches
before re-baseline so cache-stale findings (e.g., the abbr-FP from
power-select pre-PR-#12) re-resolve correctly.

Per-target deltas:

  super-rentals    -2 errors (img wcag/h37, form wcag/h32 cleared
                   by Thread A + Thread B)
  ember-primitives -1 error  (2 wcag/h71 cleared by fieldset-with-
                   component-content; +1 no-implicit-button-type
                   newly visible on tabs.gts:247 — TabButton now
                   resolves to <button>)
  ember-power-select -1 error -1 warning (abbr-FP cleared by cache
                   fix re-resolving via PR #12; one trigger.gts
                   prefer-native warning gone for the same reason)
  hds-design-system -10 errors (much shuffling: -39 element-
                   required-attributes from img-splat fix, -19
                   element-permitted-content, +31 aria-label-misuse
                   newly visible, +6 no-implicit-button-type)
  limber           +1 error  (Editor iframe missing title — newly
                   visible after better resolution; needs
                   investigation as potential plugin gap)

Total errors across all targets: −13.
@johanrd
Copy link
Copy Markdown
Owner Author

johanrd commented May 8, 2026

@copilot resolve the merge conflicts in this pull request

@johanrd
Copy link
Copy Markdown
Owner Author

johanrd commented May 8, 2026

@copilot resolve the merge conflicts in this pull request

…ired-attrs

# Conflicts:
#	test/integration.test.ts

Co-authored-by: johanrd <4601554+johanrd@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 8, 2026

@copilot resolve the merge conflicts in this pull request

Resolved — I merged origin/main into this branch and fixed the conflict in test/integration.test.ts in commit 33c7859.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 8, 2026

@copilot resolve the merge conflicts in this pull request

Done — conflicts are already resolved on this branch in commit 33c7859.

Copilot finished work on behalf of johanrd May 8, 2026 07:07
@johanrd johanrd merged commit 5116c2a into main May 8, 2026
3 checks passed
@github-actions github-actions Bot mentioned this pull request May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants