Skip to content
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

Merged
merged 6 commits into from
Nov 2, 2020

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Oct 28, 2020

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)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

@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 <dialog /> and is failing silently?

@straker
Copy link
Contributor

straker commented Oct 28, 2020

Thanks for tackling the pr. So the dialog elements without roles shouldn't be included in the tests and should be ignored as they don't use ARIA roles.

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

Thanks for tackling the pr. So the dialog elements without roles shouldn't be included in the tests and should be ignored as they don't use ARIA roles.

Why though? Is this you saying that <dialog /> shouldn't be used? As far as I can tell <dialog /> is a [role="dialog"] at least by spec: https://www.w3.org/TR/html-aam-1.0/#details-id-30

As far as I can tell the testdriver simply doesn't support <dialog> since it can't find them.

@straker
Copy link
Contributor

straker commented Oct 28, 2020

The reason it can't find them is because the selector property of the rule metadata file is a CSS selector, so it only finds elements with [role=dialog] and [role=alertdialog]. However, the rule focuses only elements with the role property specifically, so even though dialogs implicit role is dialog, we handle that in a different rule

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

we handle that in a different rule

Could you explain why you have different rules for <dialog /> and [role="dialog"]?

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

Also: <dialog role="alertdialog" /> elements do not pass or fail and display a "element not found" in the browser. Are you sure this is about implicit roles and not about <dialog> not being supported?

@straker
Copy link
Contributor

straker commented Oct 28, 2020

Sure. A lot of the time it's so we can apply different checks. For example, the button-name rule looks only at button elements, while the aria-command-name looks at elements with the role=button (as well as some others). The requirements for how you name a button and role=button are slightly different, so we have different rules for them.

In the case of the dialog element, I don't think we have a rule that looks at it specifically, but if we do it would be it's own rule.

@straker
Copy link
Contributor

straker commented Oct 28, 2020

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 dialog elements. However, I doubt IE11 does so it might fail it there.

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

The requirements for how you name a button and role=button are slightly different, so we have different rules for them.

I understand if that's going too much off-topic but while I have you here: What are differences in naming between <button /> and [role="button"]? I guess there are some host-level semantics that come into play. At least that's the only reason I could imagine looking through accname.

@straker
Copy link
Contributor

straker commented Oct 28, 2020

The difference is pretty subtle. First, we need to ignore any buttons that have a presentational role (presentation or none), which doesn't apply to an element with a role=button. The other difference isn't so much how it's named but the seriousness of missing the naming method. Not having text content for a button is marked as critical while missing text content on a role=button is minor (even though they use the same code underneath to determine if the element has text content).

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

First, we need to ignore any buttons that have a presentational role (presentation or none), which doesn't apply to an element with a role=button.

I was assuming that a <button role="presentation" /> wouldn't fall under [role="button"] but the rest makes sense, thanks!

@straker
Copy link
Contributor

straker commented Oct 28, 2020

There's also this lovely bug which we had to ignore recently #2577

@straker
Copy link
Contributor

straker commented Oct 28, 2020

Alright, I found the "bug" in dialog. The issue is that Chromes default stylesheet is hiding the element by default. We don't run any checks on hidden elements. So ya, probably best just to not include any dialog elements in the test.

dialog:not([open]) {
    display: none;
}

@eps1lon
Copy link
Contributor Author

eps1lon commented Oct 28, 2020

The issue is that Chromes default stylesheet is hiding the element by default. We don't run any checks on hidden elements. So ya, probably best just to not include any dialog elements in the test.

Makes perfect sense 👍 I just rendered all of them as open. Might make sense to include <dialog role="alertdialog" open /> simply to document the styling caveat for testing even though <dialog role="alertdialog" /> is not treated any different in the rule than <div role="alertdialog" />.

@eps1lon eps1lon marked this pull request as ready for review October 28, 2020 23:12
@eps1lon eps1lon requested a review from a team as a code owner October 28, 2020 23:12
@straker straker added the hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/ label Oct 29, 2020
"id": "aria-dialog-name",
"selector": "[role=\"dialog\"], [role=\"alertdialog\"]",
"matches": "no-naming-method-matches",
"tags": ["cat.aria", "wcag2a", "wcag412"],
Copy link
Contributor

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

Suggested change
"tags": ["cat.aria", "wcag2a", "wcag412"],
"tags": ["cat.aria", "best-practice"],

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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)

@WilcoFiers
Copy link
Contributor

Thank you for the contribution @eps1lon! If you're interested at some point to contribute more, please reach out.

@WilcoFiers WilcoFiers merged commit b0e14b0 into dequelabs:develop Nov 2, 2020
@eps1lon
Copy link
Contributor Author

eps1lon commented Nov 5, 2020

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. role="tree".

@eps1lon eps1lon deleted the feat/aria-dialog-name branch November 5, 2020 14:58
@WilcoFiers
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants