fix(aria-prohibited-attr): allow many elements to be named and disallow label and body from being named - #5259
fix(aria-prohibited-attr): allow many elements to be named and disallow label and body from being named#5259straker wants to merge 9 commits into
Conversation
WilcoFiers
left a comment
WilcoFiers
left a comment
There was a problem hiding this comment.
Nice cleanup — replacing the "no role at all means no name" inference with explicit spec data is clearly the right direction, and it fixes a batch of long-standing false positives. I built both the merge-base and this branch and diffed rule output across ~60 markup cases to see exactly what moves. Two things need a decision before merge.
Note: the failing test_node (18) and test_node (20) checks are an unrelated undici/jsdom incompatibility, not caused by this PR.
1. del loses naming prohibition while ins keeps it (blocking)
lib/standards/html-elms.js L215 (compare ins at L566)
del didn't get namingProhibited: true, but ins did:
<del aria-label="hello"></del>
before: violation — aria-label attribute cannot be used on a del with no valid role attribute.
after: PASS
<ins aria-label="hello"></ins>
before: violation
after: violation
Naming is prohibited on both — del maps to deletion and ins to insertion, and both carry prohibitedAttrs in aria-roles.js. Since neither element appears in implicitHtmlRoles, the role spec is no longer consulted, so del silently drops its prohibition.
Suggestion: add namingProhibited: true to del, and add a test case pairing del with ins so the two can't drift apart again.
2. body and dd report a role whose spec allows naming (blocking)
lib/checks/aria/aria-prohibited-attr-evaluate.js L76, with body at html-elms.js L135 and dd at L211
These are the only two elements given namingProhibited: true that also have a non-null implicit role, and two problems surface there.
The outcome is right but the reported role is wrong. <body aria-label> should fail — but axe maps body to document and dd to definition, and neither role has prohibitedAttrs, so the message blames a role that permits naming:
<body aria-label="hello"> before: PASS
after: incomplete — aria-label attribute is not well supported with role "document".
<dd aria-label="hello"> before: PASS
after: violation — aria-label attribute cannot be used with role "definition".
I confirmed aria-allowed-attr still passes both, so the two rules now disagree with each other. The real fix is in the mapping: body should be generic and dd should have no corresponding role. Worth noting generic doesn't exist in lib/standards/aria-roles.js at all today, so that correction is a prerequisite rather than a one-liner.
role === implicitRole isn't a safe proxy for "no explicit role." An author can write <dd role="definition"> or <p role="paragraph"> and land in the HTML-spec branch even though an explicit role was set — which is the case the ARIA role spec should govern.
Suggestion: branch on getExplicitRole() rather than comparing the resolved role to the implicit role, and fix the body/dd mappings (adding generic to aria-roles.js) so the prohibition and its message come from the same place. If the mapping fix is too large for this PR, splitting it out would at least keep body and dd from shipping with contradictory messaging in the meantime.
3. Four more elements correctly stop prohibiting names (note)
Beyond del, four elements flip from violation to pass, all correctly:
<address aria-label="hello"> before: violation after: PASS
<blockquote aria-label="hello"> before: violation after: PASS
<hgroup aria-label="hello"> before: violation after: PASS
<ruby aria-label="hello"> before: violation after: PASS
These were false positives from the old catch-all — implicitHtmlRoles only lists elements whose role axe actually needs, so every known HTML element in that gap fell through to return ['aria-label', 'aria-labelledby']. This is a fix, not a regression.
Suggestion: call these out in the PR description so they land in the release notes, and add test cases pinning the new behavior so we don't regress them in the future.
4. doc/check-options.md documents a stale default (non-blocking)
doc/check-options.md L98
The PR changes the elementsAllowedAriaLabel default to ["applet"], but the docs still advertise a twelve-element list including label — precisely what this PR reverses. The table was already stale, but this makes it actively contradict the fix in the PR title. Two-line change while you're in there.
5. noAriaAttrs branch reuses the noRole messages (non-blocking, separate issue)
lib/checks/aria/aria-prohibited-attr-evaluate.js L88
<col aria-invalid="true">
violation — aria-invalid attribute cannot be used on a col with no valid role attribute.
The wording implies a valid role would fix it, but col permits no ARIA attributes at all regardless of role. Same message now reaches map, picture, base, head, html, link, meta, noscript, param, script, slot, source, style, template, title, and track. A dedicated message key would read better — worth its own issue rather than expanding this PR.
straker
changed the title
straker
changed the title
…e-core into aria-prohibited-fix
2 participants
After looking over the HTML in ARIA spec, I realized that we could better handle how we determine when an attribute is prohibited. Basically if the element's role resolves to it's implicit role then we use the
htmlElmspec to determine prohibited attrs and naming, otherwise we use theariaRolesspec to determine that. This should allow us to eliminate theelementsAllowedAriaLabeloption that was introduced to fix which elements allowed naming and then modified when we added Chromium roles to fix conflict name resolution. However the option still has theappletelement which isn't in our html spec so I'm leaving it for now.Elements which allow naming now include:
section,blockquote,address,hgroup,ruby.This also adds enforcement for elements that are not allowed any aria-* attribute (which has never been used in our code), but if we don't want that I can take it out.
Closes: #5185
Closes: #3410