search

remotestorage / remotestorage-widget

⬡ Connect widget for remoteStorage.js
https://remotestorage.io
28 stars 16 forks source link

Improve accessibility by marking hidden elements appropriately #87

Closed galfert closed 4 years ago

galfert commented 6 years ago

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

ndarilek commented 6 years ago

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 commented 6 years ago

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.

ndarilek commented 6 years ago

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.

raucao commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

raucao commented 6 years ago

Thanks! That will be very helpful.

ndarilek commented 6 years ago

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 commented 4 years ago

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