-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
Conversation
…lements => fix keyboard navigation
# Conflicts: # js/src/dropdown.js
Update unit test with new markup. |
@ALL can you help me with this one http://stackoverflow.com/questions/41645884/accessibility-for-select-fields-in-ie11 |
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? |
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. |
It's a very good work thank you @vanduynslagerp 👍 IMO we shouldn't remove dependency to |
@Johann-S the PR remove the dependency to |
js/src/dropdown.js
Outdated
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}`) |
There was a problem hiding this comment.
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 ?
Sorry but your branch is out-of-date so can you update ? |
Again but I arrived too late and your branch isn't up to date and #21802 too |
Can you notice me when you update your branch ? Or can you allow me to update your branch ? Thank you |
I gave you permission |
There was a problem hiding this 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
Per migration documentation in 4d8d8bd:
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:
role="menu"
orrole="listbox"
and rely on the classdropdown-menu
insteada
element for the dropdown-item and rely on the classdropdown-item
insteadli
elementsWith this modification the javascript now matches the markup in the documentation.
The keyboard navigation works again now in dropdowns.
Also fixes #21941 (see comment)