-
Notifications
You must be signed in to change notification settings - Fork 779
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
fix(required-children): consider overriding descendant role(s) when validating required child role(s) #2131
Conversation
Do we need to fail on this test case? <div role="list" aria-owns="list"></div>
<div id="list">
<div role="tabpanel">
<div role="listitem"></div>
</div>
</div> |
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.
Both of the suggestions are optional, you can make the changes if you so feel.
} else { | ||
return false; | ||
const items = isOwns | ||
? Array.from(el.children).reduce( |
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.
What exactly is this reduce doing? It looks like it's just pushing the children onto an array with the parent element. If that's the case it would be cleaner to just use concat.
const items = (isOwns ? [el] : []).concat(Array.from(el.children));
: !!axe.utils.querySelectorAll(virtualTree, selector)[0]; | ||
const ownedElements = idrefs(node, 'aria-owns'); | ||
const descendantRole = getDescendantRole(node, ownedElements); | ||
const missing = missingRequiredChildren( |
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.
Instead of upping the eslint max-params
limit you could pass all these as a single object and destructure it.
missingRequiredChildren({
node,
childRoles,
all,
role,
ownedElements,
descendantRole
});
function missingRequiredChildren({
node,
childRoles,
all,
role,
ownedElements,
descendantRole
}) {
// ...
}
@jeeyyy hey, question on this change. I'm not 100% sure, but this change seems to result in axe-core to flagging issues where Example that seems like it should be OK, but fails (Example pulled from: The MDN Example #2 on using group role) <div role="menu">
<ul role="group">
<li role="menuitem">Inbox</li>
<li role="menuitem">Archive</li>
<li role="menuitem">Trash</li>
</ul>
<ul role="group">
<li role="menuitem">Custom Folder 1</li>
<li role="menuitem">Custom Folder 2</li>
<li role="menuitem">Custom Folder 3</li>
</ul>
<ul role="group">
<li role="menuitem">New Folder</li>
</ul>
</div> |
Closes issue:
aria-required-children
#2076Reviewer checks
Required fields, to be filled out by PR reviewer(s)