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

Basic keyboard accessibility CSS tweaks #6441

Merged
merged 6 commits into from
Feb 6, 2013
Merged

Basic keyboard accessibility CSS tweaks #6441

merged 6 commits into from
Feb 6, 2013

Conversation

patrickhlauke
Copy link
Member

Mostly doubling-up :hover styles to also cover :focus, as a first step
to making the framework more keyboard-friendly.
Additionally, fixed two small markup issues in the docs/examples to
make the "Learn more" large primary button-styled links
keyboard-focusable (as without href they're treated as non-tabable
anchors).

Mostly doubling-up :hover styles to also cover :focus, as a first step
to making the framework more keyboard-friendly.
Additionally, fixed two small markup issues in the docs/examples to
make the "Learn more" large primary button-styled links
keyboard-focusable (as without href they're treated as non-tabable
anchors).
@patrickhlauke
Copy link
Member Author

Still unresolved issues with keyboard access and dropdown menus (as featured in the documentation, with tabindex=-1, as well as dropdowns with submenus). Hoping to address these in next update as well...

@mdo
Copy link
Member

mdo commented Jan 12, 2013

Your diff is quite dirty for some reason—perhaps because of how you're compiling the CSS? Run make in Terminal to compile the CSS before committing.

If you clear that up soon, we can look at implementing this before 2.3 is released in a week or so. Thanks!

Used recess rather than less
@patrickhlauke
Copy link
Member Author

ok, worked out that using recess rather than just less gives something much closer to the desired result :)

Removed unnecessary (?) and harmful (from a keyboard access point of
view) tabindex=-1 in the documentation examples.
@patrickhlauke
Copy link
Member Author

remaining issue with dropdown submenus, but that will require some additional JS...leaving that for a separate pull request in the future.

@mdo
Copy link
Member

mdo commented Jan 12, 2013

We're dropping submenus in 3.0, so no need to worry about those. Do you have any background reading or other links to share that would better illuminate these changes? They're rather far reaching.

Related, I don't remember exactly why, but the tabindex="-1" was added for a reason. Perhaps @fat remembers?

@patrickhlauke
Copy link
Member Author

re background reading...do you mean about doubling up :hover and :focus? it's one of the oldest and simplest keyboard accessibility techniques, so not sure if there's a canonical reference for this. i've written about some of it here http://24ways.org/2009/dont-lose-your-focus/ but you can basically test this yourself by using TAB/SHIFT+TAB to move between focusable elements on a page. keyboard navigation moves the focus, but it doesn't trigger :hover styles, only :focus styles.

see for instance also http://www.w3.org/TR/UNDERSTANDING-WCAG20/navigation-mechanisms-focus-visible.html and the tech note http://www.w3.org/TR/2012/NOTE-WCAG20-TECHS-20120103/C15

the majority of my changes deal with this...where there was just :hover, also include a matching :focus.

as for why i added href="#" and removed tabindex="-1": in both cases, that makes the elements non-focusable by default, so they're excluded from the normal tab cycle.

@patrickhlauke
Copy link
Member Author

my gut feeling about why tabindex was added would be that perhaps an early version of your dropdown CSS involved moving them off-screen, rather than display:none, which would have made the menu items still focusable for keyboard users? shrug

@mpnkhan
Copy link

mpnkhan commented Jan 15, 2013

Hi @patrickhlauke . Please don't remove tabIndex -1 .. This is needed for keyboard navigable Javascript widgets. https://developer.mozilla.org/en-US/docs/Accessibility/Keyboard-navigable_JavaScript_widgets

http://www.w3.org/WAI/GL/wiki/Using_ARIA_menus
On How the Keyboard interaction should be for Menu : http://www.w3.org/TR/wai-aria-practices/#menu

@patrickhlauke
Copy link
Member Author

@mpnkhan in this case, tabindex is not needed, as it was applied to actual, already focusable, link elements - rather than generic elements like a span or a div. in fact, try navigating those two cases in the documentation via keyboard as they currently are...because of the tabindex=-1, they can't receive focus and get skipped. with my change of removing them, they now CAN be focused and navigated.

additionally, having that tabindex=-1 in the documentation is even more misleading because in the example pages (e.g. the hero one) the dropdown does NOT contain tabindex, and again because of that it's already navigable just fine with TAB/SHIFT+TAB out of the box.

so, i stand by the proposed change i'm making here.

@patrickhlauke
Copy link
Member Author

and to specifically address the two links you included:

https://developer.mozilla.org/en-US/docs/Accessibility/Keyboard-navigable_JavaScript_widgets "Negative (i.e. tabindex="-1") - Tab navigable: No; author must focus the element with focus() in response to arrow or other key presses." as the dropdowns are already made up of links, you DON'T want to suppress tab navigability and handle it with JS/focus(). there's no need, as the links are already tabable, so the most appropriate approach is not to use any tabindex and to let it "follow the platform conventions of the element", which in the case of link elements (as used in the dropdowns) is to allow keyboard navigation already out-of-the-box.

http://www.w3.org/WAI/GL/wiki/Using_ARIA_menus#Tabindex "For menu items to receive keyboard focus the tabindex attribute must be set of the element if it is not a form control or link element" - as in this case it IS a link element, tabindex should not be used (and certainly not tabindex=-1, which suppresses focusability)

@mpnkhan
Copy link

mpnkhan commented Jan 15, 2013

Anchor shouldn't receive focus in a drop down menu. There should be only one tab stop. That is the Menu container. All the interaction should be down arrow based. Please see the http://www.w3.org/TR/wai-aria-practices/#menu
This is a agreed upon standard and this is how YUI and Jquery are coded..
http://jqueryui.com/menu/
http://developer.yahoo.com/yui/examples/button/button-ariaplugin.html
http://yuilibrary.com/yui/docs/node-focusmanager/node-focusmanager-button.html

@patrickhlauke
Copy link
Member Author

@mpnkhan sorry, but i disagree. the way dropdowns are currently implemented here in bootstrap isn't fully ARIA-fied at this point, so the whole "using arrows keys and managing focus programmatically" part is not present. for this reason, the most sensible approach at this point is to let links be links, let keyboard users simply TAB/SHIFT-TAB through them. the dropdown itself is already only providing a single tab stop - it needs to be activated for the dropdown menu to actually appear. by virtue of the menu being display:none when inactive, those links are not in the tab cycle until the user opened the submenu.

i'd be all for leaving tabindex=-1 in there (note that it's in the documentation, but NOT in the examples at this point) but only if all the other relevant javascript which then does handle the programmatic focus and key handling is also included...which currently, it isn't.

@patrickhlauke
Copy link
Member Author

and additionally, i seriously doubt the discoverability of the fully ARIA compliant menu item for a regular keyboard user who only knows how to use TAB/SHIFT+TAB...but that's an issue to take up with ARIA, i guess.

and to be clear: my patch here only removes tabindex=-1 from the documentation example. leaving it there would mean people copy that part of the code, but without having the other necessary JS to then handle the dropdown, which would mean that out-of-the-box they'd be building completely keyboard-inaccessible dropdowns unless they based their code on the examples (which do NOT use tabindex=-1)

@patrickhlauke
Copy link
Member Author

additionally, fully following ARIA would mean that any navigation bar (which is where those dropdown menus are often found) would also have to be treated as a menu, so any links would also have to be navigated not with TAB/SHIFT+TAB, but with arrows...which is even less intuitive for regular keyboard users.

@mpnkhan
Copy link

mpnkhan commented Jan 15, 2013

The JS is there to do Keyboard navigation http://twitter.github.com/bootstrap/javascript.html#dropdowns .
And Aria roles have been added to 2.2.3
2c0ed07
#6539

This is the AOL keyboard interaction guide as well : http://dev.aol.com/dhtml_style_guide#menu .. Bottom it has a list of contributors to the guidelines.

@patrickhlauke
Copy link
Member Author

hmm, interresting. testing it here though http://twitter.github.com/bootstrap/components.html#dropdowns with the actual dropdown trigger missing makes it inaccessbile in the documentation. i still disagree that it shouldn't listen to TAB/SHIFT+TAB as that is what regular keyboard users will expect.

anyway, if the tabindex part of this patch is contentious, remove it. surprised in that case that ARIA itself was implemented, but something as simple as doubling up :focus/:hover isn't. at least push those out for now.

It seems that, as misguided as I personally think it is, ARIA does
indeed want dropdown menus not to behave as regular keyboard users
expect them, killing TAB/SHIFT+TAB in favor of cursor keys. Fair
enough, the issue I have is then with ARIA, not with bootstrap. I would
sugges thought that if you really do want to follow ARIA consistently,
*any* navigation bar should also become an ARIA menu, further making it
impossible for keyboard users to use TAB/SHIFT+TAB to navigate through
it, and that these changes should be reflected in the bootstrap
examples as well...
@patrickhlauke
Copy link
Member Author

as the tabindex thing seems a contentious issue (even among some people in the accessibility community itself, as arguably ARIA's approach is geared more towards app menus rather than short-ish navigation bars), i've removed it from this pull request (and apologies for the triple-whammy commits, had some issues with git and couldn't work out how to rebase both locally and here on github :S )

@StommePoes
Copy link

Why can't tabindex="0"? Javascript now has access, without keyboard users getting kicked in the head (especially since keyboard users without any special accessibility technology don't get any ARIA anything). Additionally, because JS can now move .focus() to the element, if it later needs to set its tabindex to -1, it can.

Though tabindex="0" was originally for non-natively-focusable elements...

color: #999999;
}

.dropdown-menu > .disabled > a:hover {
.dropdown-menu > .disabled > a:hover .dropdown-menu > .disabled > a:focus {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a comma between these two.

@fat fat merged commit c553290 into twbs:2.3.0-wip Feb 6, 2013
@mdo mdo mentioned this pull request Feb 6, 2013
@mdo
Copy link
Member

mdo commented Feb 6, 2013

Merged this into 2.3.0-wip and then into 3.0.0-wip (ugh, the conflicts). Thanks so much!

@patrickhlauke patrickhlauke deleted the 2.3.0-wip branch April 10, 2013 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants