Skip to content

Commit 54f5ce0

Browse files
committed
fix: trim+first-token role + skip dynamic <select size> (Copilot review)
- Normalize role via trim() + first-token split before lookup so values like 'COMBOBOX' or 'combobox listbox' are handled consistently (ARIA role attribute is a space-separated fallback list; only the first supported token is effective). - getSelectImplicitRole() now returns 'combobox' | 'listbox' | 'unknown'. A dynamic <select size={{...}}> previously fell through as 'combobox' and produced false positives on role="combobox"; we now bail on 'unknown' instead of flagging.
1 parent bf5ed00 commit 54f5ce0

1 file changed

Lines changed: 29 additions & 14 deletions

File tree

lib/rules/template-no-redundant-role.js

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,20 +40,29 @@ const ALLOWED_ELEMENT_ROLES = [
4040
// Per HTML-AAM, <select> maps to "combobox" only when neither `multiple` nor
4141
// `size > 1` is set; otherwise it maps to "listbox". Mirrors jsx-a11y's
4242
// src/util/implicitRoles/select.js.
43-
function selectHasComboboxImplicitRole(node) {
43+
//
44+
// Returns 'combobox' / 'listbox' for static cases, or 'unknown' when a
45+
// dynamic `size` value blocks a decision. Callers should skip flagging on
46+
// 'unknown' to avoid false positives.
47+
function getSelectImplicitRole(node) {
4448
const attrs = node.attributes || [];
4549
const hasMultiple = attrs.some((a) => a.name === 'multiple');
4650
if (hasMultiple) {
47-
return false;
51+
return 'listbox';
4852
}
4953
const sizeAttr = attrs.find((a) => a.name === 'size');
50-
if (sizeAttr && sizeAttr.value && sizeAttr.value.type === 'GlimmerTextNode') {
54+
if (sizeAttr) {
55+
if (!sizeAttr.value || sizeAttr.value.type !== 'GlimmerTextNode') {
56+
// Dynamic `size` — can't tell whether implicit role is combobox or
57+
// listbox, so bail out instead of risking a false positive.
58+
return 'unknown';
59+
}
5160
const sizeValue = Number(sizeAttr.value.chars);
5261
if (Number.isFinite(sizeValue) && sizeValue > 1) {
53-
return false;
62+
return 'listbox';
5463
}
5564
}
56-
return true;
65+
return 'combobox';
5766
}
5867

5968
// Mapping of roles to their corresponding HTML elements
@@ -148,8 +157,14 @@ module.exports = {
148157

149158
let roleValue;
150159
if (roleAttr.value && roleAttr.value.type === 'GlimmerTextNode') {
151-
// ARIA role tokens are compared ASCII-case-insensitively.
152-
roleValue = (roleAttr.value.chars || '').toLowerCase();
160+
// ARIA role tokens are compared ASCII-case-insensitively, and the
161+
// attribute is a space-separated fallback list — only the first
162+
// supported token is honored as the effective role.
163+
const firstToken = (roleAttr.value.chars || '').trim().toLowerCase().split(/\s+/u)[0];
164+
if (!firstToken) {
165+
return;
166+
}
167+
roleValue = firstToken;
153168
} else {
154169
// Skip dynamic role values
155170
return;
@@ -168,13 +183,13 @@ module.exports = {
168183
// <select role="combobox"> is only redundant when <select>'s implicit
169184
// role actually is "combobox" (no `multiple`, and `size` absent or <= 1).
170185
// Otherwise the implicit role is "listbox", so the explicit "combobox"
171-
// is not redundant and this rule should not flag it.
172-
if (
173-
node.tag === 'select' &&
174-
roleValue === 'combobox' &&
175-
!selectHasComboboxImplicitRole(node)
176-
) {
177-
return;
186+
// is not redundant and this rule should not flag it. When `size` is
187+
// dynamic we bail ('unknown') rather than risk a false positive.
188+
if (node.tag === 'select' && roleValue === 'combobox') {
189+
const implicit = getSelectImplicitRole(node);
190+
if (implicit !== 'combobox') {
191+
return;
192+
}
178193
}
179194

180195
const isRedundant =

0 commit comments

Comments
 (0)