-
Notifications
You must be signed in to change notification settings - Fork 4.2k
User menu a11y changes to allow use of spacebar, escape key and arrows to navigate #1560
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
Conversation
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.
added roles for a11y
|
@caesar2164, thanks for the work you've put together. In general all of the new roles, attributes, and UI behavior changes look good. As we chatted about over HipChat, let's try syncing up tabbing in the expanded dropdown menu so that it matches the scoped navigation you've set up with the arrow up/down events. So to recap, once focused and selected (through enter, click, or spacebar), the dropdown menu should lock a user's actions to that menu until that locked focus is escaped (escape button, click, etc.) We can have TPG confirm that this consistency is the right way to go once they review to confirm fixes. |
|
@talbs & @adampalay - here's the Tab and Shift + Tab changes |
lms/static/js/my_courses_dropdown.js
Outdated
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.
the thing about children().children('a'): I think it makes sense to make this more generalized: follow the lead here and make a focusableElementsString for dropdowns. Or just do find('a') (I can't think of other dropdown elements we'd want to focus on). It may be overkill for this instance, but it's going to be helpful if a) we alter the html slightly, and b) if other people want to make their dropdowns accessible, but don't have the exact ul>li>a structure we have. I know @jmclaus was asking about this PR earlier today.
|
@caesar2164 regarding the acceptance test failure, on commit 55a48ec. We have refactored and improved a lot of the helper methods for the tests over the past couple weeks. Please rebase so they will run more reliably. |
|
@jzoldak - thanks, just rebased and started a manual test so hopefully this can get wrapped up before @adampalay and @talbs go home today. @adampalay - I think your issue is dealt with now that I only catch the tab and shift-tab key presses when the menu is open. |
lms/static/js/my_courses_dropdown.js
Outdated
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.
it'll be much cleaner if you replaced line 18 with
if ( event.which == 32) { dropdownMenuToggle.click(); event.preventDefault(); }
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.
I guess I could, but that code would have to stay inside the catchKeyPress function anyway as I want to catch spacebar key presses inside the open menu as well...
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.
The only keypress you're worried about when dropdownMenuToggle is in focus is the spacebar. Since focus is restricted to the dropdown once it is opened, when the dropdown is open, focus will never be on the dropdownMenuToggle, so we don't need to define any special rules for it
… keys or tab and shift+tab to navigate
|
👍 |
|
@caesar2164, things look good to me as well. Thanks for the fixes. 👍 |
User menu a11y changes to allow use of spacebar, escape key and arrows to navigate
|
Giulio! Dude!!! Merging is a two-step process - click the merge button, wait a few seconds, then click the Delete branch button! Thanks :) |
|
JEEZ! I clicked merge, then I had to go talk to someone!!! @sarina - you're gonna burst an artery being so strung out! ;) |
|
always i got my 👀 on you... |
* Revert "Fix initialize end-date for self-paced openedx#1544 (openedx#1546)" This reverts commit 8d47dc9cf42c09857efb813e682d1c2f780abd91. * Fix end-date display openedx#1547
@talbs, @adampalay - Here's another A11Y PR for you guys to check.
In short, this makes the user menu easier to use with the keyboard: