-
Notifications
You must be signed in to change notification settings - Fork 16
Improve accessibility by marking hidden elements appropriately #87
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
Conversation
Elements are hidden by setting the opacity to 0, which does not actually hide them from screen readers. Now they also get the `aria-hidden` attribute.
Cool. So one challenge you'll run into with In this case, when I use Firefox's caret navigation, I can't find anything other than the text "Connect your storage to sync data with your account." When I tab/shift-tab, I do find a bunch of controls. But they don't appear in caret navigation. Any reason to not just use |
Yes, there are many reasons for that. And the solution is not to not use With this change, the hidden role actually maps 1:1 to what you see (or rather do not see). It's just not enough, because when you don't see anything, you also need to know which elements will take you to the next screen/view. |
@@ -345,6 +347,11 @@ Widget.prototype = { | |||
this.closed = false; | |||
this.rsWidget.classList.remove('rs-closed'); | |||
this.shouldCloseWhenSyncDone = false; // prevent auto-closing when user opened the widget | |||
|
|||
let selected = document.querySelector('.rs-box.rs-selected'); |
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 find this name to be very misleading. Selected means having selected something from various options. But this seems to not be the case for this element. I think this is about which one is visible or not, isn't it?
Ah, so "Connect your storage" is clickable. Got it. Can that be a
`<button/>`? The rabbit hole there is that, if it isn't, you'll have to
add key-handling to behave like buttons, and that gets platform-specific.
I'm not saying "never use ARIA." I'm saying that web developers who
aren't familiar with accessibility think "ah, I can slap on an ARIA
attribute to make this non-button behave like a button," and then "Ah,
I'll just add a keyup/tabindex" not realizing that different platforms
use different keys for button-presses. It's like the old regexp
quote--people think they can solve a problem with ARIA, then they have
two problems. :)
Appologies if these are things you know. I'm used to working with
developers who *don't*.
|
Yes, we could probably change it. But if there's a well-supported way of defining it via attributes plus title or something, then that would be preferable in this case, because the styling would be much harder with a button. If nothing else is good enough (which you then have to tell us from testing whatever we think might be enough), then we'll have to turn it into a button as a last resort. |
No problem. This is useful knowledge, and the issue will also serve as documentation for people who might not know these things. |
Fair enough.
IIRC, the clickable text is an `<h3/>`. I'd leave the `<h3/>` intact,
but put the text itself in a span like so:
`<span aria-role="button" tabindex="0">Connect your storage</span>`
Then you'll need a keyup on that element, for Space and Enter, that
triggers a click. I think some platforms don't trigger clicks on one or
the other of these, but we'll just ignore the inconsistency for now.
|
Thanks! That will be very helpful. |
I'm not sure where this was left, but any chance it might be merged in for continuing development? From reading the comments, it looks like things were hidden a bit better, but we may be able to add some additional attributes to improve the experience. Even if this isn't a complete fix, I think it represents an improvement over the way things are now and should probably be merged to create a more accessible baseline. Thanks. |
@ndarilek Sorry for having lost track of this for so long! I'm following your suggestion and merging it now. |
Right now elements are only hidden by setting the opacity to 0, which does not actually
hide them from screen readers.
This PR also sets the the
aria-hidden
attribute.A demo of this version of the widget can be found at https://rs-widget-demo.5apps.com/.