fix(aria-allowed-role): allow roles on a non-details summary - #5242
fix(aria-allowed-role): allow roles on a non-details summary#5242chutchins25 wants to merge 2 commits into
Conversation
A summary that is not its parent details element's disclosure trigger is exposed as generic and may take any role. A summary that is the details summary continues to allow no explicit role. Per ARIA in HTML (w3c/html-aria#435). The attribute-side of the spec change (disallowing aria-expanded and aria-pressed on a details summary) is tracked separately in #5241. Closes #3911 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // See https://github.com/w3c/html-aria/pull/435 | ||
| summaryForDetails: { | ||
| matches: { | ||
| condition: vNode => { |
There was a problem hiding this comment.
Probably a new matches function called firstOfType which kinda mimics the CSS pseudo selector of the same name :first-of-type. So maybe something like
matches: {
firstOfType: 'summary'
}There was a problem hiding this comment.
Also we have code that does this lookup of if the summary is the first summary of a details parent in https://github.com/dequelabs/axe-core/blob/develop/lib/commons/dom/visibility-methods.js#L248-L256
There was a problem hiding this comment.
Built the firstOfType matcher as suggested, but matches: { firstOfType: 'summary' } also matches a standalone summary (first-of-type under a non-details parent), which the spec treats as "any role" — so it regresses the standalone cases. Keeping it correct needs an additional "parent is details" condition, which the matches object can't express today without the inline function we're removing. Options: (a) extend the matches mechanism with a parent/ancestor matcher, (b) a more specific named matcher (e.g. summaryForDetails), or (c) accept treating any first-of-type summary as the trigger (matches the existing visibility-methods assumption). Which do you prefer? I've reverted the half-built version pending your call.
There was a problem hiding this comment.
Thanks for the pointer — I'll reuse that first-summary-of-details logic. Folding this into the matcher-shape question in the thread above.
| ); | ||
| } | ||
| }, | ||
| allowedRoles: false |
There was a problem hiding this comment.
Might also need to add the allowed attrs:allowedAriaAttrs: ['aria-disabled', 'aria-haspopup']
There was a problem hiding this comment.
The attribute side is handled in the stacked follow-up #5241 via an element-level disallowedAttrs: ['aria-expanded', 'aria-pressed']. allowedAriaAttrs (aria-allowed-attr-elm) only restricts global attrs and defers when there's an explicit role — but aria-expanded/aria-pressed are non-global, so it wouldn't catch them, and all globals are in fact allowed on a details-summary. Mind taking a look at #5241 to see if that shape works for you?
| assert.deepEqual(checkContext._data, ['tab']); | ||
| }); | ||
|
|
||
| it('returns true for a details summary with its implicit button role', () => { |
There was a problem hiding this comment.
summary doesn't seem to have an implicit role of button anymore? I'm not sure when this changed as the way back machine is down at the moment. We should look into when this change was made. At least in https://www.w3.org/TR/2023/REC-html-aria-20230306/ they updated it to say that it had no role but some assistive technologies would announce it as button. This implies we need to test it ourselves and make sure what is announced.
There was a problem hiding this comment.
Good catch. axe maps summary → button implicitly (since #2375, 2019), but current html-aria says summary has "no corresponding role." That's a pre-existing mapping with its own blast radius, so I've dropped the implicit-button assertion here and think changing the mapping belongs in its own issue rather than this PR. Want me to file one?
| ); | ||
| }); | ||
|
|
||
| it('returns false for a details summary with an unallowed role in shadow DOM', () => { |
There was a problem hiding this comment.
This type of shadow dom test isn't needed. Only time this is needed is if we need to cross the shadow dom boundary or not. But ones that just test things inside the shadow DOM aren't needed.
There was a problem hiding this comment.
Removed — no boundary crossing here, so agreed it's not needed.
| assert.deepEqual(getElementSpec(vNode), {}); | ||
| }); | ||
|
|
||
| it('should not allow a role on a summary that is the summary for its details', () => { |
There was a problem hiding this comment.
This file tests more generic style tests than this so aren't needed. The integration tests for allowed-role will cover this.
There was a problem hiding this comment.
Removed — the allowed-role integration tests cover this.
Drop the inside-shadow-DOM aria-allowed-role case (no boundary crossing), the generic get-element-spec summary cases (covered by integration), and the implicit-button-role assertion (summary's implicit role is a separate question being tracked independently). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Updates the roles allowed on a
<summary>element based on whether it is its parent<details>element's disclosure trigger, per the ARIA in HTML update.What & why
Per ARIA in HTML (
#el-summary, w3c/html-aria#435):summarythat is the "summary for its parentdetails" (the firstsummarychild of adetails) allows no explicit role — browsers expose it as the disclosure trigger.summarythat is not the details summary (standalone, or a 2nd+summary) is exposed as generic and allows any role.axe previously modeled
summarywith an unconditionalallowedRoles: false. This converts it to avariantusing aconditionmatcher (the same pattern introduced forfigurein #3443).Scope — role side only
Issue #3911 covers both the allowed roles and the allowed
aria-*attributes forsummary. This PR implements the role side. The attribute side (the spec disallowsaria-expanded/aria-pressedon a details-summary) is intentionally deferred — it requires an element-level disallowed-attribute mechanism axe does not have today, and is tracked in #5241.Behavior change
A standalone / non-details
<summary>now permits any role, where previously only the implicitbuttonrole was allowed. This is a spec-correct loosening; a<details>-summary's role handling is unchanged in effect (still restricted to its implicitbuttonrole).Tests
get-element-specunit — variant resolution for the three cases (details summary, non-first summary, standalone).aria-allowed-rolecheck unit — incl. an open Shadow DOM case.aria-allowed-rolevirtual-rule — details/summary structures built withSerialVirtualNode, incl. the no-parent default.aria-allowed-roleintegration HTML/JSON — first-summary fail, 2nd-summary pass, standalone pass, implicit-button pass.Closes #3911