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

fix: accessibility for closing hamburger menu with keyboard #8674

Conversation

TanyaPina
Copy link
Contributor

@TanyaPina TanyaPina commented Dec 28, 2023

Closes #7917

Issue: The user hamburger menu being unable to be closed through keyboard use, resulting in a menu that remains open even when focus is outside of it.

This PR fixes this issue by enabling the user to exit header dropdown menus with the press of the Escape key. Additionally, it traps focus in the hamburger menu list by cycling through the list items until the user chooses to exit the menu with the escape key.

Technical

While this PR applies escape key functionality to header dropdown menus, it currently fixes the focus issue within the hamburger dropdown menu specifically. A limitation with this fix would be if the user is in the last menu item and shift+tabs to go backwards, which results in them going to the first menu list item instead of the penultimate.

Testing

  1. Tab to the user hamburger menu button on the far right side of the header
  2. Press the "Enter" or "Space" key to open the dropdown menu
  3. Tabbing all the way to the last list and then tabbing once more should bring the focus back to the first item of the list
  4. Pressing the "Escape" key will exit the menu and bring the focus to the next item.

Screenshot

Loom video to show keyboard behavior:
https://www.loom.com/share/9f35dffdc8dd435f82a991f5be22db2a?sid=2cece70d-9271-494a-b81c-40822c6d5b85

Stakeholders

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2023

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 15.93%. Comparing base (45ed081) to head (025ed94).
Report is 67 commits behind head on master.

Files Patch % Lines
openlibrary/plugins/openlibrary/js/index.js 0.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8674      +/-   ##
==========================================
- Coverage   15.96%   15.93%   -0.03%     
==========================================
  Files          89       89              
  Lines        4710     4718       +8     
  Branches      821      823       +2     
==========================================
  Hits          752      752              
- Misses       3449     3455       +6     
- Partials      509      511       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Nice, thank you @TanyaPina ! One UX/Accessibility change to the approach for focus out 👍

openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
@cdrini cdrini added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing labels Jan 15, 2024
@cdrini cdrini added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Jan 29, 2024
@cdrini
Copy link
Collaborator

cdrini commented Mar 18, 2024

Hey @TanyaPina ! Are you still working on this one? Anything we can do to help knock it over the finish line?

@TanyaPina TanyaPina requested a review from cdrini April 1, 2024 20:07
@cdrini cdrini removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Apr 15, 2024
@cdrini cdrini removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 23, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Lgtm! Tested both on desktop/mobile and works like a charm! One small fix.

openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
Comment on lines 446 to 456
$('.header-dropdown').on('keydown', function (event) {
if (event.key === 'Escape') {
$('.header-dropdown > details[open]').not(this).removeAttr('open');
}
});

$('.dropdown-menu').each(function() {
$(this).find('a').last().on('focusout', function() {
$('.header-dropdown > details[open]').not(this).removeAttr('open');
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see the one's above do this 😅 We also shouldn't be using .not(this) here, since we actually do want to close this!

@cdrini cdrini merged commit 801cf5c into internetarchive:master Apr 27, 2024
3 checks passed
merwhite11 pushed a commit to merwhite11/openlibrary that referenced this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User icon hamburger menu cannot be easily closed with keyboard.
3 participants