Conversation
…ing into validateByType Removes the top-level string-"undefined" short-circuit in `isValidAriaValue` (previously a two-layer dance: a token-type precheck then a boolean-ish short-circuit). Absorbs the `allowundefined` handling directly into `validateByType` via a new `allowsUndefinedLiteral(attrDef, value)` helper that honors aria-query's convention for the 4 boolean-type attrs that encode "string 'undefined' is spec-valid": aria-expanded, aria-hidden, aria-grabbed, aria-selected. ## Before/after — same outcome, cleaner structure | Attribute | aria-query type / allowundefined | `"undefined"` string accepted? | |---|---|---| | aria-orientation | token / (unset) with `'undefined'` in values | yes — via token-values check | | aria-expanded | boolean / true | yes — via allowundefined | | aria-hidden | boolean / true | yes — via allowundefined | | aria-grabbed | boolean / true | yes — via allowundefined | | aria-selected | boolean / true | yes — via allowundefined | | aria-pressed | tristate / (unset) | NO — no allowundefined flag | | aria-checked | tristate / (unset) | NO — no allowundefined flag | All behavior preserved; just structured so that validity decisions happen in one place. ## Why keep the allowundefined path at all The 4 boolean-type attrs with `allowundefined: true` have spec-valid `"undefined"` values per WAI-ARIA 1.2 (e.g. aria-expanded accepts true/false/undefined). aria-query encodes this. Peers (jsx-a11y, lit-a11y, angular-eslint) effectively reject `aria-expanded="undefined"` because their `allowundefined` branch only fires on JS undefined, not the string. That's a peer bug; our path keeps us spec-compliant. Tests added for each of the 4 allowundefined attrs; new negative test for `aria-pressed="undefined"` (tristate without allowundefined) pins the correct rejection path.
🏎️ Benchmark Comparison
Full mitata output |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors ARIA attribute value validation so the literal string "undefined" is handled in a single place (validateByType) via a helper, improving spec-compliance and simplifying control flow.
Changes:
- Introduces
allowsUndefinedLiteral()and movesallowundefinedhandling intovalidateByType(). - Simplifies
isValidAriaValue()into a thin dispatcher tovalidateByType(). - Expands rule tests to cover boolean
allowundefinedattributes andaria-orientation’s token-values behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/rules/template-no-invalid-aria-attributes.js | Centralizes "undefined" literal handling in type validation and simplifies isValidAriaValue. |
| tests/lib/rules/template-no-invalid-aria-attributes.js | Adds valid/invalid fixtures for "undefined" across relevant ARIA attrs and for invalid aria-orientation values. |
💡 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 2 out of 2 changed files in this pull request and generated no new 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 2 out of 2 changed files in this pull request and generated no new 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 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
lib/rules/template-no-invalid-aria-attributes.js:33
- For attributes where
attrDef.allowundefinedis true, validation now accepts the literal string "undefined", but the reportedexpectedType(viagetExpectedTypeDescription) will still describe these as just "a boolean". This can produce a misleading error message for other invalid values (e.g.aria-expanded="foo"). Consider updatinggetExpectedTypeDescriptionto include "undefined" as an allowed literal whenallowundefinedis set, so the message matches the actual accepted values.
// In aria-query 5.3.2, `allowundefined: true` is set only on the four
// boolean-like ARIA state attributes — `aria-expanded`, `aria-hidden`,
// `aria-grabbed`, `aria-selected` — whose WAI-ARIA 1.2 value tables list
// the literal string `"undefined"` as a spec-valid value meaning "state
// is not applicable" (e.g. https://www.w3.org/TR/wai-aria-1.2/#aria-expanded).
// The flag is nominally type-agnostic, but in practice this function only
// green-lights `"undefined"` for that boolean-like subset; no non-boolean
// ARIA attribute in aria-query currently sets `allowundefined`.
function allowsUndefinedLiteral(attrDef, value) {
return value === 'undefined' && Boolean(attrDef.allowundefined);
}
function validateByType(attrDef, value) {
if (allowsUndefinedLiteral(attrDef, value)) {
return true;
}
switch (attrDef.type) {
case 'boolean': {
return isBoolean(value);
}
💡 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 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+71
to
+77
| function isValidAriaValue(attrName, value) { | ||
| const attrDef = aria.get(attrName); | ||
| if (!attrDef) { | ||
| return true; | ||
| } | ||
| return validateByType(attrDef, value); | ||
| } |
There was a problem hiding this comment.
Since the rule now explicitly accepts the string "undefined" for attributes where attrDef.allowundefined is true, the error message shown for other invalid values of those attributes (via getExpectedTypeDescription) is currently incomplete (it will still say "a boolean." and omit the valid "undefined" literal). Consider updating getExpectedTypeDescription to include "or the string "undefined"" when attrDef.allowundefined is true so reported expectations match actual validation.
Comment on lines
+14
to
+21
| // In aria-query 5.3.2, `allowundefined: true` is set only on the four | ||
| // boolean-like ARIA state attributes — `aria-expanded`, `aria-hidden`, | ||
| // `aria-grabbed`, `aria-selected` — whose WAI-ARIA 1.2 value tables list | ||
| // the literal string `"undefined"` as a spec-valid value meaning "state | ||
| // is not applicable" (e.g. https://www.w3.org/TR/wai-aria-1.2/#aria-expanded). | ||
| // The flag is nominally type-agnostic, but in practice this function only | ||
| // green-lights `"undefined"` for that boolean-like subset; no non-boolean | ||
| // ARIA attribute in aria-query currently sets `allowundefined`. |
There was a problem hiding this comment.
The new block comment ties behavior to an exact aria-query version (5.3.2), but package.json depends on aria-query: ^5.3.2, so consumers may run newer versions where allowundefined coverage changes. To avoid the comment becoming stale/misleading, consider rewording it to be version-agnostic (e.g., "currently in aria-query") or pinning the dependency if the version-specific assertion is important.
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.
[Mirror of ember-cli#2723 for Copilot review]
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.This PR is part of a broader a11y parity audit comparing our rules against jsx-a11y, vue-a11y, angular-eslint-template, and lit-a11y.
Premise
Per
aria-query5.3.2, a handful of ARIA attributes accept the literal string"undefined"as a spec-valid value:type/allowundefinedaria-orientationtoken/(unset)with'undefined'in values listaria-expandedboolean/truearia-hiddenboolean/truearia-grabbedboolean/truearia-selectedboolean/trueTwo orthogonal encodings: token with
'undefined'in values (aria-orientation) and boolean withallowundefined: true(the other four). Both should produce "accept the string'undefined'"; neither is the user's fault to write.Problem
The pre-fix
isValidAriaValuehad a clunky two-layer structure:The precheck was a late patch that landed because
aria-orientation="undefined"(the token case) was falsely rejected by the short-circuit. The short-circuit itself exists for the 4 boolean-type attrs. So validation was split across three locations for one attribute.Fix
Absorb
allowundefinedhandling directly intovalidateByTypevia a single helperallowsUndefinedLiteral(attrDef, value). The token-values check in the'token'case naturally handlesaria-orientation="undefined"without any special-casing.isValidAriaValuebecomes a thin dispatcher tovalidateByType.Behavior — unchanged outcomes, cleaner structure
aria-orientation="undefined"aria-expanded="undefined"aria-hidden="undefined"aria-grabbed="undefined"aria-selected="undefined"aria-pressed="undefined"aria-orientation="sideways"Prior art — peers don't handle the 4 boolean+allowundefined attrs correctly
Verified each peer in source:
aria-expanded="undefined"aria-proptypesvalidityCheck('boolean', value)istypeof value === 'boolean'; string"undefined"fails.allowUndefined && value === undefinedonly fires on JSundefined, not the string — and is effectively dead code because jsx-a11y readsattributes.allowUndefined(camelCase) but aria-query emitsallowundefined(lowercase). Rejects the string.aria-attr-valid-valueallowundefinedlowercase matches aria-query correctly, but still only triggers on JSundefined. Rejects the string.valid-ariaif (allowundefined && isNil(attributeValue)) return true;—allowundefinedonly triggers on null/undefined, not the string. Rejects the string.aria-propsaria.has(name). Trivially accepts the string (by not checking).So of the four peers, three reject a spec-valid construct, one doesn't check values at all. This PR makes our rule more spec-compliant than the three value-validating peers on these 4 attrs.
Note — case-insensitivity gap (out of scope)
jsx-a11y and lit-a11y lowercase the value before the token check (
permittedValues.indexOf(value.toLowerCase()) > -1). Our rule does NOT lowercase; a case-variant likearia-orientation="UNDEFINED"or"Horizontal"would still fail here even after this PR. Same class of HTML-enumerated-attribute case-insensitivity gap as ember-cli#2718 (kind="captions") and ember-cli#2728 (role tokens). Worth a follow-up PR; explicitly out of scope for this narrow change.Tests
aria-orientation="undefined"/"horizontal",aria-expanded="undefined",aria-hidden="undefined",aria-grabbed="undefined",aria-selected="undefined",aria-pressed="true"/"false"/"mixed".aria-orientation="sideways",aria-pressed="undefined"(no allowundefined),aria-required="undefined"(boolean without allowundefined — pre-existing test).