Suppress wcag/h32 / wcag/h71 on yield-bearing <form>/<fieldset>#17
Open
Suppress wcag/h32 / wcag/h71 on yield-bearing <form>/<fieldset>#17
wcag/h32 / wcag/h71 on yield-bearing <form>/<fieldset>#17Conversation
A thin <form>{{yield}}</form> wrapper component lets the consumer
provide the form body (including the submit button); same shape with
<fieldset> for the legend. The blanker erases {{yield}} and html-
validate then sees an empty body, FP-firing wcag/h32 ("form must have
a submit button") or wcag/h71 ("fieldset must have legend").
A length-preserving in-place injection of a synthetic submit-button
child isn't workable: {{yield}} is 8 chars, `<button type=submit>` is
19. Instead, extend the per-Source directive prefix already used for
no-unused-disable on branched templates: `BlankResult` now carries a
`disableForRules` list, populated by `detectStructuralYieldRules`
which walks the AST for `<form>`/`<fieldset>` containing yield (or
has-block). Transform.ts builds an inline
`<!--html-validate-disable wcag/h32 wcag/h71-->` directive from the
union of branched-derived rules and structural rules, prepends it to
the Source's data, and shifts offset/column to compensate.
Trade-off: a yield-bearing form that ALSO has body content with no
submit will be silenced. Acceptable — yield-bearing forms in real
codebases are almost always thin component wrappers (no extra body
content), and the alternative is a permanent FP with no escape hatch.
Same shape applies to fieldset/h71. Tests cover both.
There was a problem hiding this comment.
Pull request overview
This PR extends the Ember template “blanking” + transformation pipeline to suppress wcag/h32 and wcag/h71 false-positives when <form> / <fieldset> bodies are provided via {{yield}} (or similar opaque constructs) and therefore appear empty after blanking.
Changes:
- Added
BlankResult.disableForRulespopulated by an AST scan for yield-bearing<form>/<fieldset>structures. - Updated
transform.tsto prepend an inline<!--html-validate-disable …-->directive built from the union of multipass (no-unused-disable) and structural suppression rules, with offset/column compensation. - Added integration tests and new fixtures for yield-only
<form>and<fieldset>wrappers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
transform.ts |
Builds and prepends a disable-directive prefix per Source, adjusting offset/column accordingly. |
blank.ts |
Adds disableForRules to BlankResult and detects yield-bearing <form>/<fieldset> to suppress wcag/h32/wcag/h71. |
test/integration.test.ts |
Adds regression tests ensuring wcag/h32/wcag/h71 do not fire on yield-only wrappers. |
examples/yield-only-form.gts |
Fixture exercising <form …>{{yield}}</form> wrapper behavior. |
examples/yield-only-fieldset.gts |
Fixture exercising <fieldset …>{{yield}}</fieldset> wrapper behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review on #17: 1. detectStructuralYieldRules previously flagged any <form>/<fieldset> containing yield, even if the body also had a real <button type=submit> / <legend> that would make wcag/h32 / wcag/h71 not fire. The injected disable directive was then itself "unused" and triggered no-unused-disable. Replaced elementContainsYield with isElementBodyYieldOnlyOpaque, which requires the body to have yield AND no structural element children — exactly when the runtime DOM could have the required child but the static analysis can't see it. 2. Added examples/yield-only-form.hbs + an integration test asserting no wcag/h32 fires for the same pattern in a classic .hbs template. The .hbs path previously always used offset:0/column:1; with the directive prefix it goes negative. This test verifies html-validate tolerates the negative offset cleanly for the .hbs flow too.
Per Copilot review on #17: 1. The buildDisableDirective comment said to keep the rule list in sync with MULTIPASS_INCOMPATIBLE_RULES, but only the no-unused-disable rule needs that — the structural rules (wcag/h32, wcag/h71) are intentionally not in that set. Clarified. 2. isElementBodyYieldOnlyOpaque's comment said 'whitespace-only text doesn't disqualify' but the implementation accepts any TextNode. Updated the comment to match the implementation: text content doesn't disqualify because the rules in question only care about structural element children, not text.
Per Copilot review on #17: r2's `isElementBodyYieldOnlyOpaque` was overcorrected — it bailed on ANY ElementNode child, even harmless wrapper markup like `<form><div>{{yield}}</div></form>`. Those patterns still blank `{{yield}}` to "no submit visible" and FP-fire wcag/h32 / wcag/h71, but no longer get added to disableForRules. Replaced with two targeted predicates: - elementYieldsAndLacksSubmit: form has yield somewhere AND no statically-detectable submit. Submit-detection counts any `<button>` (default type=submit inside a form), `<input type='submit'>`, `<input type='image'>`, OR an ambiguously-typed `<button type={{x}}>` / `<input type={{x}}>` (conservative — could be submit at runtime, so we bail to avoid no-unused-disable). - elementYieldsAndLacksLegend: fieldset has yield AND no static `<legend>` child. Both walk the full subtree, so wrapper markup doesn't disqualify and real static submit/legend at any depth does. Two new regression tests: - yield-only-form-with-wrapper.gts: wrapped yield is suppressed. - yield-form-with-static-submit.gts: static submit alongside yield blocks suppression (asserts no-unused-disable does NOT fire).
Comment on lines
+1355
to
+1360
| if (stmt.type === 'ElementNode') { | ||
| if (stmt.tag === 'button' || isSubmitInput(stmt) || isAmbiguouslyTypedInputOrButton(stmt)) { | ||
| hasStaticSubmit = true; | ||
| return; | ||
| } | ||
| walk(stmt.children); |
Comment on lines
+1406
to
+1413
| function isSubmitInput(node: AST.ElementNode): boolean { | ||
| if (node.tag !== 'input') return false; | ||
| for (const attr of node.attributes ?? []) { | ||
| if (attr.name !== 'type') continue; | ||
| if (attr.value.type === 'TextNode') { | ||
| return attr.value.chars === 'submit' || attr.value.chars === 'image'; | ||
| } | ||
| return false; |
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
A thin
<form>{{yield}}</form>wrapper component lets the consumer provide the form body (including the submit button); same shape with<fieldset>for the legend. The blanker erases{{yield}}and html-validate then sees an empty body, FP-firingwcag/h32("form must have a submit button") orwcag/h71("fieldset must have a legend as the first child").A length-preserving in-place injection of a synthetic submit-button child isn't workable:
{{yield}}is 8 chars,<button type=submit>is 19. Instead, extend the per-Source directive prefix already used forno-unused-disableon branched templates:BlankResultnow carries adisableForRules: string[]list, populated bydetectStructuralYieldRuleswhich walks the AST for<form>/<fieldset>containingyieldorhas-block.transform.tsbuilds an inline<!--html-validate-disable wcag/h32 wcag/h71-->directive from the union of branched-derived rules and structural rules, prepends it to the Source's data, and shiftsoffset/columnto compensate.MULTIPASS_DIRECTIVE_PREFIXbecomes a smallbuildDisableDirective(rules)helper.Trade-off
A yield-bearing form that ALSO has body content with no submit will be silenced. Accepted because (a) yield-bearing forms in real codebases are almost always thin component wrappers (no extra body content), and (b) the alternative is a permanent FP with no escape hatch —
no-implicit-button-typeand friends survive a directive scoped to wcag/h32 only.Surfaced by
Ecosystem CI on
universal-ember/ember-primitives(4 findings: 2×wcag/h32on<Form>/<OTP.Input>, 2×wcag/h71on<Fieldset>-shaped components) andhashicorp/design-system(10 findings: 6×wcag/h32on<HdsForm>, 4×wcag/h71on<HdsForm.Fieldset>).Test plan
examples/yield-only-form.gts—<form ... ...attributes>{{yield}}</form>must not firewcag/h32examples/yield-only-fieldset.gts—<fieldset ... ...attributes>{{yield}}</fieldset>must not firewcag/h71npm test— 129/129 passing (was 128, +1 net new test; the previously-failing yield-only-form test now passes via the fix)npm run typecheck:tests— cleannpm run build— clean