-
Notifications
You must be signed in to change notification settings - Fork 791
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: Add aria-dialog-name #2609
Conversation
@WilcoFiers Seems like axe is not picking up that role. I have no idea how it does that or how to debug it. I didn't find help in CONTRIBUTING.md and simply followed #2548. But I don't see where I diverged since I must be missing something fundamental. Does the environment not support |
Thanks for tackling the pr. So the |
Why though? Is this you saying that As far as I can tell the testdriver simply doesn't support |
The reason it can't find them is because the |
Could you explain why you have different rules for |
Also: |
Sure. A lot of the time it's so we can apply different checks. For example, the button-name rule looks only at In the case of the |
I can take a look the next time it fails, but we use chrome headless for the tests and I'm pretty sure it supports |
I understand if that's going too much off-topic but while I have you here: What are differences in naming between |
The difference is pretty subtle. First, we need to ignore any buttons that have a presentational role ( |
I was assuming that a |
There's also this lovely bug which we had to ignore recently #2577 |
Alright, I found the "bug" in dialog:not([open]) {
display: none;
} |
Makes perfect sense 👍 I just rendered all of them as open. Might make sense to include |
lib/rules/aria-dialog-name.json
Outdated
"id": "aria-dialog-name", | ||
"selector": "[role=\"dialog\"], [role=\"alertdialog\"]", | ||
"matches": "no-naming-method-matches", | ||
"tags": ["cat.aria", "wcag2a", "wcag412"], |
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.
Looking good, glad all the tests are passing now. Just one more thing before we can merge. The rule should be a best practice rule and not linked to wcag success criterion
"tags": ["cat.aria", "wcag2a", "wcag412"], | |
"tags": ["cat.aria", "best-practice"], |
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.
Right, I've been meaning to ask how this is decided. @WilcoFiers added tags in the umbrella issue but some of the newly implemented rules don't match those tags (or just partially):
- aria-command-name has "cat.aria", "wcag2a", "wcag412" instead of just "wcag2a"
- aria-progressbar-name has "cat.aria", "wcag2a", "wcag111" instead of just "sc111"
- aria-tooltip-name has "cat.aria", "wcag2a", "wcag412" instead of "best-practice"
Specifically here: Why is not naming a dialog not a 4.12 failure and "just" best practice?
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.
This is based on our interpretation of WCAG 2. Every rule needs a category tag. That's the "cat.*" stuff. Additionally, a rule is either a WCAG success criterion, which are those "wcag123" type tags or a "best-practice". If it's a wcag tag, it'll also include a level, which is just the level of that success criterion (SC) in WCAG. So SC 1.1.1 has the tag "cat.aria", "wcag2a", "wcag111". This best practice would get "cat.aria", "best-practice".
As for why I made tooltip a WCAG rule and think dialog should be a best practice, that is based on some discussions I've had internally at Deque. A tooltip doesn't make much sense when it is empty, but more importantly a tooltip is a piece of a user interface that users will understand to be a single "thing". Success criterion 4.1.2 applies only to "user interface components". It's a little more difficult to argue that a dialog is a user interface component, since it is just a region of the screen with other content in it. User interface components are the smallest things users will understand as its own control. For example a select is a user interface component, but a list of radio buttons is not, each radio button would be though.
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 can see how this makes sense.
Slightly going off-topic here: why does a tree
not fall under "user interface components"? It was was scratched due to aria-structure-name
being scratched but I would consider this more of a widget and not just some region. Happy to continue this in #2421 (comment)
Thank you for the contribution @eps1lon! If you're interested at some point to contribute more, please reach out. |
@WilcoFiers I did in #2421 (comment) but haven't heard back from you. Specifically, I'd like to understand why some roles have name required by spec but you don't want to see rules for it e.g. |
@eps1lon TBH I don't fully know about tree. I think you're right that generally, trees need an accessible name. I don't think WCAG requires it, but for a best practice, probably. What makes me hesitant is that I don't know if this is always true. If you follow ARIA, trees are dynamic, collapsable structures. But what if they're not? And what if the treeitems are explanatory? It's a little like radiogroups. Most of the time they need a name, but if the purpose of the group is obvious from the name, the name is redundant. Not saying you're wrong, but I'm concerned we'll get false positives from testing trees for accessible names. It might help if we had a couple of examples to show different uses of trees, and how they work in screen readers. If you want, lets open an issue specifically for this topic. |
Not sure how to debug the tester so I'm checking if CI gives more insights.
Part of #2421
Reviewer checks
Required fields, to be filled out by PR reviewer(s)