mobify / pinny

A mobile-first content fly-in UI plugin
MIT License
23 stars 4 forks source link

Accessibility Issues #29

Closed jeffkamo closed 10 years ago

jeffkamo commented 10 years ago

Keyboard Accessibility

Screen Reader Accessibility

<!-- Before pinny is opened... -->
<div class="t-main" aria-hidden="false"></div>
<section class="pinny" aria-hidden="true"></section>

<!-- After pinny is opened... -->
<div class="t-main" aria-hidden="true"></div>
<section class="pinny pinny--is-open" aria-hidden="false"></section>
<section class="pinny" describedby="pinnyDescription" role="dialogue">
    <div id="pinnyDescription" class="visually-hidden"><!-- description text --></div>
    <!-- .pinny__header -->
    <!-- .pinny__content -->
</section>
<section class="pinny" labelledby="pinnyLabel" role="dialogue">
    <!-- description block -->
    <header class="pinny__header">
        <h1 id="pinnyLabel" class="pinny__title"></h1>
    </header>
    <!-- .pinny__content -->
</section>
<section class="pinny" labelledby="pinnyLabel" role="dialogue">
    <div role="document">
        <!-- description block -->
        <!-- .pinny__header -->
        <!-- .pinny__content -->
    </div>
</section>

W3C Recommendations


sources

jeffkamo commented 10 years ago

@kpeatt @scalvert @ry5n thoughts?

ry5n commented 10 years ago

sweet research. most of this I haven’t looked at myself, but seem broadly solid.

one question: what is the reason we need aria-describedby (and maybe aria-labelledby for the header)? wouldn’t that content be hidden until the user opened the pinny, in which case it should be fulfilled by the pinny header?

jeffkamo commented 10 years ago

From what I gathered by Greg Kraus' article (he doesn't really get into the description part too deeply) is: it's purely s message to help orient screen reader users.

"You're in a modal. This is what it's for and how to use it or how to escape"

ry5n commented 10 years ago

Ah, I see. That makes sense.

kpeatt commented 10 years ago

Interactivity with the page main content must be disabled. This can be achieved by adding aria-hidden="true" to whatever is wrapping the main content (the example pages don't have any identifiers on the main content div!!)

Hm. I don't really like this. We don't have any guarantee anyone will have a content wrapper so we'd have to go off and do that. If something is aria-hidden="false" inside of an aria-hidden="true", what happens?

kpeatt commented 10 years ago

A (perhaps overridable) description div should be included always - it's purpose is to describe the modal and how to interact with it. Implementation on the rendered html would require the description div having a unique id, and that id referred to on the pinny in a aria-describedby attribute

Not sure I understand the need for this.

kpeatt commented 10 years ago

The window behind the modal dialog should not be allowed to be clicked on

There's a difference between a modal in the strict sense and these overlays. Are we saying we don't want to close Pinny on background tap?

ry5n commented 10 years ago

If something is aria-hidden="false" inside of an aria-hidden="true", what happens?

Pretty sure it’s still hidden to screen readers.

aria-hidden=false is not mapped in any browser that supports aria-hidden, thus its use has no meaning or in other words has the same meaning as its absence. See http://www.paciellogroup.com/blog/2012/05/html5-accessibility-chops-hidden-and-aria-hidden/

ry5n commented 10 years ago

Are we saying we don't want to close Pinny on background tap?

No I think we just want to hide the rest of the page to screen readers while pinny is open. It is, strictly speaking, a modal thing.

kpeatt commented 10 years ago

No I think we just want to hide the rest of the page to screen readers while pinny is open. It is, strictly speaking, a modal thing.

Right, so this recommendation "The window behind the modal dialog should not be allowed to be clicked on" is probably not something we want to adhere to.

jeffkamo commented 10 years ago

I actually meant to remove that particular recommendation.

Our implementation is usually that tapping the window behind acts the same as tapping the "close" button.

Although it might be reasonable to implement as an optional feature. We very well may come across a situation where we legitimately wish to prevent users from escaping a modal by accident (or perhaps other developers might want it to behave as such)

jeffkamo commented 10 years ago

A (perhaps overridable) description div should be included always - it's purpose is to describe the modal and how to interact with it. Implementation on the rendered html would require the description div having a unique id, and that id referred to on the pinny in a aria-describedby attribute

Not sure I understand the need for this.

See my response to Ryan above.

jeffkamo commented 10 years ago

Interactivity with the page main content must be disabled. This can be achieved by adding aria-hidden="true" to whatever is wrapping the main content (the example pages don't have any identifiers on the main content div!!)

Hm. I don't really like this. We don't have any guarantee anyone will have a content wrapper so we'd have to go off and do that. If something is aria-hidden="false" inside of an aria-hidden="true", what happens?

What? We always implement a content wrapper of our own.

As far as I can tell, the pinny elements are appended to the end of <body>, so this is still entirely a doable thing.

Of course it would be advisable that developers identify the necessary content wrapper in order for this to work.

jeffkamo commented 10 years ago

If developers want to, or have to, implement a modal system that can't have it's own content wrapper, we could certainly allow that. But that is a broken and inadvisable approach as far as accessibility is concerned, so for us we'll never do that.

In the pinny readme and/or documentation, our recommended markup will advice the use of a content wrapper.

kpeatt commented 10 years ago

What? We always implement a content wrapper of our own.

That's true for our sites but not true for Pinny in general. We can't expect a content wrapper around everything in body. This one is looking like something that could easily be done as an extension to Pinny but not something we do by default.

kpeatt commented 10 years ago

In the pinny readme and/or documentation, our recommended markup will advice the use of a content wrapper.

We can definitely recommend that and maybe show an example for how to do it. I'm still iffy on requiring a content wrapper or requiring one in the configuration by default. Like Pikabu, these things become much less likely to be used the more we ask people to change their core markup. Are we okay with this as an optional thing?

kpeatt commented 10 years ago

See my response to Ryan above.

Is there a good default response we could write up to include for this? This one seems pretty trivial otherwise.

kpeatt commented 10 years ago

This issue is getting messy enough now that we should probably start breaking it off into individual tasks. Let's do that on Monday.

scalvert commented 10 years ago

Unless there's a super compelling reason to do so, we shouldn't add keyboard bindings. The main reason for this is that this is a mobile first plugin. Plenty of desktop implementations exist for this type of thing, but not a ton of mobile. Saving bytes is one of our mandates. Adding these will bloat the code.

I understand that keyboards are added to mobile devices sometimes to aid in navigation, but I'd rather hold on this stuff so we don't add unnecessary code.

In terms of this ticket, I think we should just focus on adding the appropriate aria attributes, and leave it at that for now. Any other accessibility functionality should be captured in tickets for potential future implementation.

kpeatt commented 10 years ago

This one is looking like something that could easily be done as an extension to Pinny but not something we do by default.

As an addendum to this, what I mean is that we can make this the definitive implementation of Pinny for our mobile builds but may not require this for usage of the plugin. We can also recommend it under an accessibility section in the Documentation — which I am all for.

kpeatt commented 10 years ago

Unless there's a super compelling reason to do so, we shouldn't add keyboard bindings.

I'm not opposed to it. Esc binding is pretty simple. For other key trapping, it's important as well. If there's an input inside the modal and someone hits the arrows above the keyboard, they could move focus outside of the Pinny to another form element on the page. This is Bad™.

kpeatt commented 10 years ago

Closed in favour of individual issues for each sub heading.