Extract rule: template-require-aria-activedescendant-tabindex#2599
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-require-aria-activedescendant-tabindex (PR #2599)
Comparison with ember-template-lint require-aria-activedescendant-tabindex.js
General correctness:
-
Interactive element check: The original uses
isInteractiveElement(node)from a shared helper which comprehensively checks interactive elements (button, input, select, textarea, a[href], and more based on roles/attributes). The PR uses a simple hardcodedINTERACTIVE_ELEMENTSset withinput,button,select,textareaand a manual check for<a>withhref. This is a reasonable simplification but may miss some edge cases the original's more comprehensive helper handles (e.g., elements withcontenteditable,role="button", etc.). Consider noting this as a known simplification. -
HTML tags check: The original uses
domfromaria-queryto get all valid HTML tags, filtering out custom components. The PR usesisCustomComponent(tag)which checks for PascalCase or dotted paths. This is a good pragmatic approach for Ember/Glimmer where components follow these naming conventions. -
Tabindex value logic — potential bug: The original has a bug where it checks
tabindexValue === Number.naN— this comparison always returnsfalsesinceNaN !== NaNin JavaScript. This means in the original, if tabindex cannot be parsed, it falls through to the< 0check, and sinceNaN < 0isfalse, it doesn't report an error when tabindex is unparseable.The PR's
getTabindexNumericValuefunction returns{ exists: true, known: false }for unparseable values, and the rule skips reporting for unknown values ("Unknown dynamic values are assumed valid"). This is actually more correct behavior than the original's accidental NaN handling. -
Different treatment of text vs mustache tabindex: The PR differentiates: for
GlimmerTextNode, it allows-1and above; forGlimmerMustacheStatement, it only allows non-negative. This asymmetry is intentional but different from the original which doesn't make this distinction. The original treats all tabindex values uniformly with< 0. The PR's approach of rejectingtabindex={{-1}}(mustache) while allowingtabindex="-1"(text) seems inconsistent. Atabindex="-1"element IS keyboard-focusable programmatically which is valid witharia-activedescendant. Consider treating both forms the same way. -
Test for
tabindex="0"without quotes:<div aria-activedescendant="some-id" tabindex=0>— the0without quotes is parsed asGlimmerTextNodewith chars"0", which parses to0, so this correctly passes. Good.
Scope analysis (gjs/gts):
This rule only checks HTML element attributes (aria-activedescendant, tabindex) on GlimmerElementNode. No helper/component name string matching. No scope analysis needed.
Overall: Good migration. The main concern is the asymmetric treatment of tabindex values between text nodes and mustache statements — consider aligning them.
🤖 Automated review comparing with ember-template-lint source
a5f675e to
01fbb79
Compare
Split from #2371.