Extract rule: template-require-context-role#2601
Extract rule: template-require-context-role#2601NullVoxPopuli wants to merge 3 commits intoember-cli:masterfrom
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-require-context-role (PR #2601)
Comparison with ember-template-lint require-context-role.js
General correctness:
-
Role mapping: The original builds its role mapping from a
ROOTSobject (parent → children) and inverts it to getVALID_PARENTS_FOR_CHILD. The PR definesROLES_REQUIRING_CONTEXTdirectly (child → valid parents). Comparing the mappings:- Original's inverted mapping produces:
cell→[row]— Missing from the PR. The original'sROOTS.rowincludes'cell'but the PR'sROLES_REQUIRING_CONTEXTdoes not have acellentry.listitem→[group, list]— PR has[list, group, directory]. The PR addsdirectorywhich is NOT in the original.directoryis a deprecated ARIA role, so this is a debatable addition.option→[listbox]— PR has[listbox, group]. The PR addsgroupwhich is NOT in the original.rowgroup→[grid, table, treegrid]— PR matches (has all three).
So there are discrepancies in both directions.
cellrole is completely missing from the PR's map — this means<div role="cell">inside a<div>(withoutrole="row") will NOT be flagged. The original would flag it. - Original's inverted mapping produces:
-
directoryrole: The PR includesdirectoryas a valid parent forlistitem. This role is deprecated in ARIA 1.2. Not wrong per se but deviates from the original. -
optionwithgroupparent: The PR allowsrole="option"insiderole="group", but the original does not. According to WAI-ARIA spec,optionrequireslistboxcontext.groupis allowed for grouping options but only within alistbox. This is an over-relaxation. -
Parent traversal approach: The original walks up using
path.parent(ember-template-lint's AST path) and stops at the first non-presentation parent element. The PR uses anelementStackmaintained via enter/exit visitors. This is the standard ESLint approach and works correctly. -
aria-hidden handling: The original checks
aria-hiddenon the current node AND ancestor nodes. The PR checksaria-hiddenonly on ancestors (viaisInsideAriaHidden). However, the original also returns early if the current element hasaria-hidden, essentially skipping all children. The PR's approach of checking ancestors seems functionally similar since child elements would find thearia-hiddenancestor.Actually, there's a difference: the original checks
aria-hiddenon the element with the context-requiring role itself and returns early. The PR only checks ancestors viaisInsideAriaHidden(which excludes the current element atelementStack.length - 1). So if you have<div aria-hidden="true" role="listitem">, the original would skip it, but the PR would still flag it (since it only checks ancestors). This is a behavioral difference — consider also checking the current node foraria-hidden. -
Root-level elements: The PR skips reporting when
elementStack.length <= 1(no parent elements) or whengetAccessibleParentRolereturnsundefined. The original'swhileloop naturally handles this. The PR's approach is correct — you can't validate context when there's no parent. -
Template tag transparency: The PR treats
<template>tag as transparent ingetAccessibleParentRole. This is correct for the gjs/gts case where<template>is a wrapper, not a semantic HTML element. -
Error message format: Different from original but conveys the same information. The original includes a W3C reference URL; the PR does not. Consider adding the reference URL.
Scope analysis (gjs/gts):
This rule only checks role attribute values on GlimmerElementNode. Pure structural/attribute checking. No scope analysis needed.
Overall: The migration has some mapping discrepancies:
- Missing:
cell→[row]role context (present in original, absent in PR) - Added:
directoryas valid parent forlistitem(not in original) - Added:
groupas valid parent foroption(not in original) - Behavioral:
aria-hiddenon the current element itself is not checked
These should be reviewed to ensure they are intentional deviations or corrected to match the original.
🤖 Automated review comparing with ember-template-lint source
Align ROLES_REQUIRING_CONTEXT with the original require-context-role rule from ember-template-lint by: - Adding missing `cell` role (requires `row` context) - Removing `directory` from `listitem` parents (not in original) - Removing `group` from `option` and `menuitemcheckbox` parents (not in original) - Adding `grid` to `rowheader` parents (present in original) - Sorting parent arrays alphabetically to match the original's sorted output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-mapping Fix role-to-context mapping to match original ember-template-lint rule
Split from #2371.