-
Notifications
You must be signed in to change notification settings - Fork 784
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
feat(list/definition-list): only allow required owned roles #3707
Conversation
return false; | ||
} | ||
|
||
const role = getExplicitRole(vChild); |
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.
We should we use getRole
instead to account for role conflict. Something like this would fail even though each is treated as a listitem (this currently does not fail today)
<ul>
<li role="none" aria-label="hello"></li>
<li role="none" tabindex="0">Hello</li>
</ul>
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 think people should rely on fallback roles to pass. We're failing this after all. The one thing I do think is worth considering is to allow role=none on empty elements. We don't allow that currently, so I think that's a separate PR.
One thing I'd like your input on is in how this rule is becoming stricter. Something like the following was passing before. I don't think it should have been, but I'm not sure if we need to more explicitly call it out in the PR title, or even create a separate PR so its separated in the changelog. WDYT?
<!-- This passes today, because it's an li and we then don't look at the role -->
<ul><li role="button">Button!</li></ul>
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 think changing the pr title to something along the lines of feat(list/definition-list): only allow required owned roles
would be sufficient for that. That would indicate a new failing case, and the clarification of the remediation message would then be a by-product that doesn't need to be called out. This should also be able to cover the now stricter syntax of other roles not being allowed when there was at least one li
Improve on messages reported by the
list
anddefinition-list
checks. Since they had so much in common I unified them into a newinvalid-children-evaluate
method. That got me 95% of the way to running their rules on virtual nodes, so I then also updated the matches method to run on virtual.This rule gets rid of the (minor) exception on
list
, where if you haveli
elements, where some have a role and others do not, the rule will pass. I don't recall why we put that in, but it doesn't make too much sense to me.The
only-dlitems-evaluate
andonly-listitems-evaluate
methods need to be deprecated, but this belongs in a separate PR.Closes issue: #3559