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

remove dropdown.js reliance on roles and fix keyboard navigation #21535

Merged
merged 24 commits into from
Apr 13, 2017

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Jan 4, 2017

Per migration documentation in 4d8d8bd:

  • Rebuilt dropdown styles and markup to provide easy, built-in support for <a> and <button> based dropdown items.
  • Dropdown items now require .dropdown-item.

In addition fix the the space key behavior on firefox for <button> elements: #21159. Thanks to @RyanThomasMusser for the analysis here. Fixes #21159.

This PR:

  • Remove the dependency to role="menu" or role="listbox" and rely on the class dropdown-menu instead
  • Remove the dependency to the a element for the dropdown-item and rely on the class dropdown-item instead
  • Remove the necessity to wrap dropdown-item in li elements

With this modification the javascript now matches the markup in the documentation.
The keyboard navigation works again now in dropdowns.

Also fixes #21941 (see comment)

@pvdlg pvdlg changed the title Dropdown: remove dependency to role="menu", role="listbox" a and li elements => fix keyboard navigation Dropdown: remove dependency to role="menu", role="listbox", a and li elements => fix keyboard navigation Jan 4, 2017
@mdo mdo requested a review from patrickhlauke January 4, 2017 19:30
@mdo mdo modified the milestones: v4.0.0-alpha.6, v4.0.0-beta Jan 4, 2017
@pvdlg
Copy link
Contributor Author

pvdlg commented Jan 6, 2017

Update unit test with new markup.
Per comment

@jump020305
Copy link

@pvdlg pvdlg changed the title Dropdown: remove dependency to role="menu", role="listbox", a and li elements => fix keyboard navigation Dropdown: new markup and fix keyboard navigation Jan 22, 2017
@mdo mdo requested a review from bardiharborow March 20, 2017 19:48
@mdo
Copy link
Member

mdo commented Mar 20, 2017

This PR is a little wonky right now, but any chance for a review from @Johann-S and @bardiharborow on the core changes at https://github.com/twbs/bootstrap/pull/21535/files#diff-bfe9dc603f82b0c51ba7430c1fe4c558?

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 20, 2017

I merged the last changes from v4-dev and for some reason the diff in the PR show all these changes even though it shouldn't as they are not different from the last v4-dev.
Looking at the diff between v4-dev and my dropdown-keyboard branch show the proper diff: v4-dev...vanduynslagerp:dropdown-keyboard

@Johann-S
Copy link
Member

It's a very good work thank you @vanduynslagerp 👍

IMO we shouldn't remove dependency to role="menu" or role="listbox" for the accessibility but I'm not an expert in this subject we should call the ARIA guy @patrickhlauke 😄

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 20, 2017

@Johann-S the PR remove the dependency to role="menu" and role="listbox" in the JS selectors so it's consistent with other JS module and rely on classes for selector.
Like other components in Bootstrap, the use of role="menu", role="listbox" or aria- for accessibility is recommended in the documentation and examples.

const ARROW_UP_KEYCODE = 38 // KeyboardEvent.which value for up arrow key
const ARROW_DOWN_KEYCODE = 40 // KeyboardEvent.which value for down arrow key
const RIGHT_MOUSE_BUTTON_WHICH = 3 // MouseEvent.which value for the right button (assuming a right-handed mouse)
const REGEXP_KEYDOWN = new RegExp(`${ARROW_UP_KEYCODE}|${ARROW_DOWN_KEYCODE}|${ESCAPE_KEYCODE}|${SPACE_KEYCODE}`)
const REGEXP_KEYDOWN = new RegExp(`${ARROW_UP_KEYCODE}|${ARROW_DOWN_KEYCODE}|${ESCAPE_KEYCODE}`)
Copy link
Member

Choose a reason for hiding this comment

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

Can you change the indent please ?

@Johann-S
Copy link
Member

Sorry but your branch is out-of-date so can you update ?

@Johann-S
Copy link
Member

Again but I arrived too late and your branch isn't up to date and #21802 too

@Johann-S
Copy link
Member

Can you notice me when you update your branch ? Or can you allow me to update your branch ? Thank you

@pvdlg
Copy link
Contributor Author

pvdlg commented Apr 13, 2017

I gave you permission

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

Looks good, and does indeed fix the issue of Firefox/space. Good stuff

@patrickhlauke patrickhlauke merged commit 3b3366e into twbs:v4-dev Apr 13, 2017
@patrickhlauke patrickhlauke changed the title Dropdown: new markup and fix keyboard navigation remove dropdown.js reliance on roles and fix keyboard navigation Apr 13, 2017
@mdo mdo mentioned this pull request Apr 13, 2017
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