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

generalize dropdowns / drop role="menu" #16571

Merged
merged 1 commit into from
Jun 2, 2015
Merged

generalize dropdowns / drop role="menu" #16571

merged 1 commit into from
Jun 2, 2015

Conversation

patrickhlauke
Copy link
Member

as role="menu" is a very specific (and strict) ARIA pattern for
desktop-like application menus, and our dropdowns are often used
as pure navigation dropdowns, this change abandons ARIA menus for
a more open-ended and light-weight approach
(see http://heydonworks.com/practical_aria_examples/#submenus and
http://www.w3.org/WAI/tutorials/menus/flyout/#improve-screen-reader-support-using-wai-aria)
note that in dropdown.js, switched to now target .dropdown-menu
instead of role["menu"] - this also prevents bootstrap scripts
from "bleeding" into non-bootstrap components on the same page.
also removed the role=["listbox"] part, which appears to be
vestigial/unused (only place in bootstrap that uses that
role are carousels, and their key handling is done separately)

for further context, see also discussion on #16179

as role="menu" is a very specific (and strict) ARIA pattern for
desktop-like application menus, and our dropdowns are often used
as pure navigation dropdowns, this change abandons ARIA menus for
a more open-ended and light-weight approach
(see http://heydonworks.com/practical_aria_examples/#submenus and
http://www.w3.org/WAI/tutorials/menus/flyout/#improve-screen-reader-support-using-wai-aria)
note that in dropdown.js, switched to now target ``.dropdown-menu``
instead of ``role["menu"]`` - this also prevents bootstrap scripts
from "bleeding" into non-bootstrap components on the same page.
also removed the ``role=["listbox"]`` part, which appears to be
vestigial/unused (only place in bootstrap that uses that
role are carousels, and their key handling is done separately)
@twbs-savage
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 5fd7bc1
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/65057753

(Please note that this is a fully automated comment.)

@patrickhlauke patrickhlauke added this to the v3.3.5 milestone Jun 2, 2015
patrickhlauke added a commit that referenced this pull request Jun 2, 2015
generalize dropdowns / drop role="menu"
@patrickhlauke patrickhlauke merged commit caca5d4 into twbs:master Jun 2, 2015
@cvrebert cvrebert mentioned this pull request Jun 2, 2015
@patrickhlauke
Copy link
Member Author

to be clear, the dropdown.js script will still work if a dev manually wants to add all the role="menu" stuff to their markup (because they ARE actually making a desktop-like web application where the menu contains actions, rather than links/navigation). this change only generalizes the script (so it looks for .dropdown-menu rather than role"menu"). however, for clarity in the documentation examples, it assumes the more common use case of developers using dropdowns for navigation.

@yatil
Copy link

yatil commented Jun 2, 2015

BTW: Some description on the challenges of using application menus are described on this W3C Menus tutorial page.

@mdo
Copy link
Member

mdo commented Jun 2, 2015

Nice! Love the simpler markup without all those attributes. Also, don't forget to at this to the ship list @patrickhlauke <3.

@patrickhlauke
Copy link
Member Author

@mdo already added to shiplist after i merged it :)

@mdo
Copy link
Member

mdo commented Jun 2, 2015

Oh shit what, totally missed that reference <3. Sorry!

@patrickhlauke
Copy link
Member Author

👍 :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants