8000 Improve accessibility by marking hidden elements appropriately by galfert · Pull Request #87 · remotestorage/remotestorage-widget · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 1 commit into from
Mar 1, 2020

Conversation

galfert
Copy link
Member
@galfert galfert commented May 25, 2018

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

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.
@ndarilek
Copy link

Cool.

So one challenge you'll run into with aria-hidden vs. display: none; is that I can write about what my screen reader detects, but that may not be in sync with what you see. So we essentially have two separate display models, not one, and it may be hard to reason about them, or to determine why certain modes see things and certain ones don't..

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. aria-hidden is a powerful tool, but it's kind of a last resort. And I don't tend to use tab/shift-tab often when I'm exploring an interface, since it skips non-focusable markup.

Any reason to not just use display: none; instead of opacity? IIRC, the former essentially removes the element from the DOM entirely, so there are no inconsistencies between caret and tab navigations, or between accessibility and non-accessibility views.

@raucao
Copy link
Member
raucao commented May 25, 2018

Yes, there are many reasons for that. And the solution is not to not use aria-hidden, but to add the other appropriate ARIA attributes that will explain that you can click the "connect your storage" part to open the connect menu.

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');
Copy link
Member
@raucao raucao May 25, 2018

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?

@ndarilek
Copy link
ndarilek commented May 25, 2018 via email

@raucao
Copy link
Member
raucao commented May 25, 2018

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.

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.

@raucao
Copy link
Member
raucao commented May 25, 2018

Appologies if these are things you know. I'm used to working with developers who don't.

No problem. This is useful knowledge, and the issue will also serve as documentation for people who might not know these things.

@ndarilek
Copy link
ndarilek commented May 25, 2018 via email

@raucao
Copy link
Member
raucao commented May 25, 2018

Thanks! That will be very helpful.

@ndarilek
Copy link

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.

@raucao
Copy link
Member
raucao commented Mar 1, 2020

@ndarilek Sorry for having lost track of this for so long! I'm following your suggestion and merging it now.

@raucao raucao merged commit b5623c5 into master Mar 1, 2020
@raucao raucao deleted the feature/86-improve_accessibility branch March 1, 2020 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0