Extract rule: template-no-redundant-role#2569
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-no-redundant-role
Compared with the original ember-template-lint no-redundant-role rule.
What looks good
- Faithful port of the original rule's logic including the
ROLE_TO_ELEMENTSmapping,ALLOWED_ELEMENT_ROLESexceptions,LANDMARK_ROLESset, andcheckAllHTMLElementsconfiguration option. - Error messages match the original's two-tier approach (landmark-specific message vs. generic message).
- Autofix implementation correctly removes the
roleattribute with preceding whitespace. - Comprehensive test suite covering both GJS and HBS parsers.
- Tests cover allowed exceptions (
nav+navigation,form+search,input+combobox,ol/ul+list,a+link). - Good edge case coverage: dynamic roles,
checkAllHTMLElements: false, multiple attributes on element.
Potential issues
-
context.getSourceCode()is deprecated: The fix function usescontext.getSourceCode()which is deprecated in ESLint v9+ in favor ofcontext.sourceCode. Consider usingcontext.sourceCodefor forward compatibility (note: PR #2568 already usescontext.sourceCode). -
Regex escaping in fix: The fix uses
replaceAll(/[$()*+.?[\\\]^{|}]/g, '\\$&')to escape the attribute text for regex matching. This is clever but fragile -- if the attribute text contains special characters or the element has complex content, the regex approach could fail. A range-based approach (usingfixer.removeRange) as done in PR #2568 would be more robust. -
Missing
<a role="link">in allowed list with context: The original allows<a role="link">(it's inALLOWED_ELEMENT_ROLES). The PR correctly includes this. However, the valid test'<a role="link" aria-disabled="true">valid</a>'only tests it witharia-disabled. Consider also testing<a role="link">without additional attributes. -
<nav role="navigation">in HBS valid tests has a typo: In the HBS tests, line'<nav class="navigation" role="navigation></nav>'is missing the closing"afternavigation. This should berole="navigation". This would silently pass as valid because the parser may treat the broken attribute differently, masking a real test. -
Good parity with original: The test cases match the original ember-template-lint test file closely --
dialog,button,checkboxoninput,columnheaderonth,listboxonselectare all covered. -
Schema matches original: The
checkAllHTMLElementsboolean option correctly mirrors the original rule's configuration.
Overlap note
This rule and PR #2568 (template-no-redundant-landmark-role) overlap significantly. This rule is the comprehensive version and should be the preferred one.
Overall, this is a thorough and well-structured port. The main action item is fixing the HBS test typo.
🤖 Automated review comparing with ember-template-lint source
The test string '<nav class="navigation" role="navigation></nav>' was missing the closing double-quote after "navigation", causing the role value to not be properly parsed in the test. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
547e978 to
ae24d4c
Compare
Split from #2371.