Add template-valid-input-attributes: flag input attributes incompatible with declared type#46
Add template-valid-input-attributes: flag input attributes incompatible with declared type#46johanrd wants to merge 1 commit into
template-valid-input-attributes: flag input attributes incompatible with declared type#46Conversation
🏎️ Benchmark Comparison
Full mitata output |
There was a problem hiding this comment.
Pull request overview
This PR adds a new opt-in template rule to detect <input> attributes that are incompatible with a statically-declared type, preventing “silently ignored by the browser” authoring mistakes in Ember templates.
Changes:
- Added
template-valid-input-attributesrule implementing an attribute/type compatibility check for statictypevalues. - Added comprehensive rule tests for both
.gjsand.hbsparsing modes. - Added rule documentation and listed the new rule in the README.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/rules/template-valid-input-attributes.js |
Implements the rule logic and the restricted attribute/type table. |
tests/lib/rules/template-valid-input-attributes.js |
Adds test coverage for allowed/forbidden combinations in both template modes. |
docs/rules/template-valid-input-attributes.md |
Documents the rule’s intent, examples, and references. |
README.md |
Adds the rule to the “Possible Errors” rules table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1304651 to
c152804
Compare
…ompatible with declared type
d6d34d5 to
a48260c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const attr of node.attributes || []) { | ||
| const validTypes = RESTRICTED.get(attr.name); | ||
| if (!validTypes) { | ||
| continue; | ||
| } | ||
| if (validTypes.has(type)) { | ||
| continue; | ||
| } | ||
| context.report({ | ||
| node: attr, | ||
| messageId: 'incompatible', | ||
| data: { attr: attr.name, type }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
HTML attribute names are ASCII case-insensitive, but the lookup uses attr.name as-is. This can miss invalid attributes written in uppercase/mixed case (e.g. ACCEPT, MaxLength). Normalize attribute names (e.g. const attrName = attr.name.toLowerCase()) before RESTRICTED.get(...), and consider reporting the normalized name for consistent messaging.
| // Per HTML §4.10.5, an <input> with missing, valueless, empty, or | ||
| // unknown `type` falls back to the Text state. Only a DYNAMIC type | ||
| // (mustache/concat) is opaque to static analysis — skip those. | ||
| const typeAttr = node.attributes?.find((a) => a.name === 'type'); |
There was a problem hiding this comment.
The PR description says inputs with no explicit type are skipped, but the implementation does not skip them—it treats missing/valueless/empty/unknown type as the Text state and validates accordingly (as these comments describe). Please align the PR description with the implemented behavior so reviewers/users don’t get conflicting expectations.
There was a problem hiding this comment.
DISAGREE — the current implementation treats missing type as 'text' (per spec). The PR description was updated to match. See disagree.md entry 9.
| let normalized; | ||
| if (!typeAttr || !typeAttr.value) { | ||
| // Missing attribute OR valueless `<input type />` — Text state. | ||
| normalized = ''; | ||
| } else if (typeAttr.value.type === 'GlimmerTextNode') { | ||
| normalized = typeAttr.value.chars.trim().toLowerCase(); | ||
| } else if ( | ||
| typeAttr.value.type === 'GlimmerMustacheStatement' && | ||
| typeAttr.value.path?.type === 'GlimmerStringLiteral' | ||
| ) { | ||
| normalized = typeAttr.value.path.value.trim().toLowerCase(); | ||
| } else { | ||
| // Dynamic value — can't statically determine; skip. | ||
| return; | ||
| } | ||
| const type = KNOWN_INPUT_TYPES.has(normalized) ? normalized : 'text'; |
There was a problem hiding this comment.
normalized = '' is being used as a sentinel for “missing/valueless type”, but it’s not itself a meaningful type value. Setting normalized directly to 'text' in that branch (or using a more explicitly named variable like rawType → type) would make the control flow easier to read and reduce reliance on magic values.
| code: '<input type="text" accept="image/*" />', | ||
| errors: [{ message: err('accept', 'text') }], | ||
| }, | ||
| { |
There was a problem hiding this comment.
Given the rule should follow HTML’s case-insensitive attribute semantics, add a test case that uses an uppercase/mixed-case restricted attribute name (e.g. <input type=\"text\" ACCEPT=\"image/*\" />) and assert it is flagged. This will prevent regressions if attr.name casing varies by parser or template authoring style.
| { | |
| { | |
| code: '<input type="text" ACCEPT="image/*" />', | |
| errors: [{ message: err('accept', 'text') }], | |
| }, | |
| { |
Note
This is part of a series where Claude has audited
eslint-plugin-emberagainst jsx-a11y, vuejs-accessibility, angular-eslint, lit-a11y and html-validate,ember-template-lint, and the HTML and WCAG specs.Summary
Add
template-valid-input-attributes: flag<input>attributes whose applicability depends on thetypewhen the declaredtypedoesn't permit them. For example,patternontype="number"is silently ignored by the browser.typeattribute pins most attributes to specific type states.patternis only defined for Text / Search / URL / Telephone / E-mail / Password.acceptis only for File Upload.min/max/stepapply only to Date / Month / Week / Time / Local Date and Time / Number / Range. The spec's per-state tables (e.g. §4.10.5.1.2 Text state and Search state) enumerate exactly which attributes each state accepts.<input type="number" pattern="\d+">, thepatternattribute is ignored by the browser — it doesn't cause a validation error, but the author's expectation isn't met. This is the "silent failure" class of bug that html-validate'sinput-attributesrule catches.<input>with a statictype, look up the attribute/type compatibility table (ported verbatim from html-validate) and flag attributes that aren't defined on that type. Skip inputs without a static type (the author'stype={{this.t}}can be anything).Fix
lib/rules/template-valid-input-attributes.js; tests intests/lib/rules/template-valid-input-attributes.js(38 cases).RESTRICTEDtable (template-valid-input-attributes.js:3-90) maps each restricted attribute (accept,alt,capture,checked,dirname,height,list,max,maxlength,min,minlength,multiple,pattern,placeholder,readonly,required,size,src,step,width) to the set oftypestates that accept it.Prior art
input-attributes— specPeer absence verified by
grep -rli "input-attributes"on 2026-04-22.Flags
Allows
Notes
pattern(text-like only),accept(file only),min/max/step(date/month/week/time/datetime-local/number/range),readonly(not applicable to checkbox/radio/file/hidden/submit/image/reset/button/color/range),required(not applicable to hidden/range/color/submit/image/reset/button). No divergence found in the sampled rows. If WHATWG adds a new input state, both tables will need updating.