Skip to content

fix(aria-allowed-attr): restrict br and wbr elements to aria-hidden only#4974

Merged
straker merged 3 commits intodequelabs:developfrom
nami8824:fix/restrict-br-wbr-aria-attrs
Apr 7, 2026
Merged

fix(aria-allowed-attr): restrict br and wbr elements to aria-hidden only#4974
straker merged 3 commits intodequelabs:developfrom
nami8824:fix/restrict-br-wbr-aria-attrs

Conversation

@nami8824
Copy link
Copy Markdown
Contributor

@nami8824 nami8824 commented Dec 30, 2025

Closes: #3177

@nami8824 nami8824 requested a review from a team as a code owner December 30, 2025 16:57
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 30, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@nami8824 nami8824 force-pushed the fix/restrict-br-wbr-aria-attrs branch from b181510 to 76c9db8 Compare December 30, 2025 17:07
WilcoFiers
WilcoFiers previously requested changes Jan 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nami8824 Thank you very much for this contribution! It is greatly appreciated.

Comment thread lib/checks/aria/aria-allowed-attr-evaluate.js Outdated
@nami8824 nami8824 force-pushed the fix/restrict-br-wbr-aria-attrs branch from e5af01d to 43733a2 Compare February 11, 2026 10:25
@nami8824 nami8824 requested a review from dbjorge February 14, 2026 05:32
straker
straker requested changes Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. Sorry it took me so long to get to the I didn't notice you pushed changes and the pr still said changes requested. Overall it's looking, just a few things we'll need to fix before we can merge.

Comment thread lib/checks/aria/aria-allowed-attr-elm-evaluate.js
Comment thread lib/checks/aria/aria-allowed-attr-role.json Outdated
Comment thread lib/checks/aria/aria-allowed-attr-elm.json Outdated
@straker straker requested a review from WilcoFiers March 25, 2026 22:38
@nami8824 nami8824 force-pushed the fix/restrict-br-wbr-aria-attrs branch from 43733a2 to c4af071 Compare April 1, 2026 04:31
@nami8824
Copy link
Copy Markdown
Contributor Author

nami8824 commented Apr 1, 2026

I've addressed the review feedback. Could you take another look?

@nami8824 nami8824 requested a review from straker April 1, 2026 04:45
straker
straker previously requested changes Apr 1, 2026
View reviewed changes
Comment thread lib/checks/aria/aria-allowed-attr-elm-evaluate.js
straker
straker reviewed Apr 1, 2026
View reviewed changes
Comment thread lib/checks/aria/aria-allowed-attr-elm-evaluate.js
@straker
Copy link
Copy Markdown
Contributor

straker commented Apr 1, 2026

Wait... I see what you mean by the nodeName. Yes because the nodeName attribute is there you can't just set data(values) and that's why you are doing the values and message key yourself. Ignore everything I've said :D

@straker straker dismissed their stale review April 1, 2026 14:46

Resolved

straker
straker approved these changes Apr 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work you put into this!

straker
straker requested changes Apr 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@straker straker left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the back and forth review. I found one minor thing that we should change. When the element has both global and non-global ARIA attributes, the resulting fix message is:

<br aria-busy="true" id="fail9" aria-expanded=false />

"Fix all of the following:
  ARIA attribute is not allowed: aria-expanded=\"false\"
  ARIA attributes are not allowed on br elements: aria-busy=\"true\", aria-expanded=\"false\""

This is because the aria-allowed-attr check fails for the non-global ARIA attribute aria-expanded, and then the new aria-allowed-attr-elm check also sees 2 attributes not allowed. I think the fix is to ignore non-global ARIA attributes in the new aria-allowed-elm-check that way the message doesn't have duplication.

"Fix all of the following:
  ARIA attribute is not allowed: aria-expanded=\"false\"
  ARIA attribute is not allowed on br elements: aria-busy=\"true\"

You can use the function get-global-aria-attrs to figure out which ones are global or not.

nami8824 added 3 commits April 2, 2026 10:32
@nami8824
fix(aria-allowed-attr): restrict ARIA attributes on br and wbr elements
7367285 
Split aria-allowed-attr check into role-based (aria-allowed-attr-role)
and element-based (aria-allowed-attr-elm) checks. Add allowedAriaAttrs
property to html-elms standard to define per-element ARIA attribute
restrictions. br and wbr now only allow aria-hidden.
@nami8824
fix(aria-allowed-attr): revert check rename and remove deprecated impact
3d89c5c
@nami8824
fix(aria-allowed-attr): ignore non-global attrs in aria-allowed-attr-…
f4eda3d 
…elm check
@nami8824 nami8824 force-pushed the fix/restrict-br-wbr-aria-attrs branch from c4af071 to f4eda3d Compare April 2, 2026 01:32
nami8824
nami8824 commented Apr 2, 2026
View reviewed changes
Comment thread lib/checks/aria/aria-allowed-attr-elm-evaluate.js Outdated
const invalid = [];

for (const attrName of virtualNode.attrNames) {
if (validateAttr(attrName) && !allowedAriaAttrs.includes(attrName)) {
Copy link
Copy Markdown
Contributor Author

@nami8824 nami8824 Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed validateAttr() because any attribute returned by getGlobalAriaAttrs() is already guaranteed to exist in standards.ariaAttrs.

@nami8824
Copy link
Copy Markdown
Contributor Author

nami8824 commented Apr 2, 2026

Thanks for the review. I've updated aria-allowed-attr-elm-evaluate.js.

@nami8824 nami8824 requested a review from straker April 2, 2026 01:40
straker
straker approved these changes Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@straker straker left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work. Thanks!

Reviewed for security.

@straker straker merged commit c6245e7 into dequelabs:develop Apr 7, 2026
22 of 23 checks passed
straker pushed a commit that referenced this pull request Apr 13, 2026
@nami8824 @straker
fix(aria-allowed-attr): restrict br and wbr elements to aria-hidden o…
1d80163 
…nly (#4974)

Closes: #3177
@straker straker mentioned this pull request Apr 13, 2026
Merged
WilcoFiers added a commit that referenced this pull request Apr 13, 2026
@WilcoFiers
chore(release): v4.11.3 (#5070)
c71e3dd 
### Bug Fixes

- **aria-allowed-attr:** restrict br and wbr elements to aria-hidden
only ([#4974](#4974))
([1d80163](1d80163))
- **target-size:** ignore position: fixed elements that are offscreen
when page is scrolled
([#5066](#5066))
([5906273](5906273)),
closes [#5065](#5065)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@straker straker straker approved these changes

@dbjorge dbjorge Awaiting requested review from dbjorge

@WilcoFiers WilcoFiers Awaiting requested review from WilcoFiers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ARIA in HTML allowances changes for br and wbr

5 participants

@nami8824 @CLAassistant @straker @dbjorge @WilcoFiers