-
Notifications
You must be signed in to change notification settings - Fork 860
fix(aria-allowed-attr): restrict br and wbr elements to aria-hidden only #4974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
nami8824
wants to merge
2
commits into
dequelabs:develop
Choose a base branch
from
nami8824:fix/restrict-br-wbr-aria-attrs
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+80
−4
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ | |
| ["#fail5"], | ||
| ["#fail6"], | ||
| ["#fail7"], | ||
| ["#fail8"] | ||
| ["#fail8"], | ||
| ["#fail9"], | ||
| ["#fail10"] | ||
| ] | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,6 +104,8 @@ | |
| ["#pass99"], | ||
| ["#pass100"], | ||
| ["#pass101"], | ||
| ["#pass102"] | ||
| ["#pass102"], | ||
| ["#pass103"], | ||
| ["#pass104"] | ||
| ] | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit reluctant about doing it this way for two reasons.
I think this is mixing node constraints with role constraints. I don't think this function should give both the allowed attributes based on roles, and the allowed attrs based on node names. I think we should probably have a separate method for something like this.
I think we should have role restrictions like this available on the standards object, so that it is configurable and findable, instead of having an inline exception for this case.
The way you've implemented this will ignore the role. I don't think that's the right thing to do here. A
<br>with role=heading should allow all ARIA properties allowed on headings. Axe has a separate rule that reports using role=heading on br's as an issue. That way we keep the issues more clearly separated.I appreciate this is going to make this work a little more complicated. @straker would you mind weighing in here too? I think it might be good to maybe propose a structure for the standards object before asking for more changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback.
Regarding my original approach: the issue I was addressing was specifically about restricting ARIA attributes on certain node types (
br/wbr), rather than on roles. Because of that, I initially thought it was necessary to pass the node information intoallowedAttrin order to enforce node-level constraints.About the standards object: when you mention moving these restrictions there, are you referring to
lib/standards/html-elms.js? If so, would supporting this require changing the data structure in a way that affects other elements as well, or is it expected to be handled only forbr/wbr?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we could have the aria restrictions in the
htmlElmsstandards, but that logic becomes a bit tricky inaria-allowed-attr. The way I see it working would be in theallowed-attrfunction if the element doesn't have a role, we check if thehtmlElmsstandard has any restrictions on attrs and then remove them from the array before we return it. But I don't think that's the correct way to go as it makes the final failure message of not allowing it muddy since the element does not have a role (and in fact has nothing to do with the role).We could probably move the logic to the
aria-prohibited-attrcheck, similar to ouraria-labelandaria-labelledbychecks on nodes with no role, but again that is a little difficult since we need to list all attributes as prohibited exceptaria-hidden. @dbjorge what do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of listing all attributes as prohibited, I'd rather keep the standards object closer to the actual standard (the wording of the relevant table header in ARIA in HTML is "ARIA role, state and property allowances"). I agree that it'd be good to keep the failure message clear about whether the issue is coming from a role-based or element-based restriction. I think based on the rule naming and our existing rule docs, it'd make the most sense for this new type of violation to appear under
aria-allowed-attrand notaria-prohibited-attr.I'd lean towards something like:
html-elmsshape to add a new property forallowedAriaAttrs- array(optional). If specified, restricts which ARIA attributes may be used with this element (in addition to any role-based restrictions).`aria-allowed-attrrule, but split thearia-allowed-attrcheck intoaria-allowed-attr-roleandaria-allowed-attr-elm(both in the rule'sallchecks)allowedAriaAttrsis set for the elm type, and if so, verifies against that list. It uses a different failure message that clarifies that the source of the failure is a restriction from ARIA in HTML's allowances for that element type.We might consider whether we should deprecate
noAriaAttrsin favor ofallowedAriaAttrs: []. That makes conceptual sense to me, but would make this a much broader change; it doesn't look like we actually usenoAriaAttrsanywhere in axe-core right now, so that would start enforcing restrictions on quite a few cases that we don't currently enforce.