Skip to content

Conversation

@ChristopherChudzicki
Copy link
Contributor

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5828

Description (What does it do?)

Makes navigation menu keyboard openable / closeable

Screenshots (if appropriate):

The close button moved:
Screenshot 2024-10-22 at 12 32 26 PM

How can this be tested?

  1. Open and close the navigation menu with your mouse, with your keyboard.
  2. Check that "Escape" closes the nav menu
  3. Check that the nav menu traps focus (cycling through all the links+button should get you back to the beginning)
  4. Check that clicking outside the nav menu closes it
    • even if you click on the initial trigger (the hamburger / explore MIT button)

@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Oct 22, 2024
Comment on lines -12 to +16
height: "60px",
paddingTop: theme.custom.dimensions.headerHeightSm,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nav menu had extra top padding at mobile widths... I think the height: 60px was supposed to be paddingTop: 60px all along. Changed to theme constants.

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review October 22, 2024 16:53
@gumaerc gumaerc self-assigned this Oct 23, 2024
Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

There's an issue with the drawer on mobile:

<UserView />
</StyledToolbar>
</Bar>
<ClickAwayListener
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the ClickAwayListener here broke the drawer on mobile:

Screen.Recording.2024-10-23.at.11.08.51.AM.mov

I realize we created a close button in the drawer, but IMO clicking the menu button again should still close the drawer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Should be resolved now.

Apparently ClickAwayListener attaches listeners to detect both click (mouse or touch) away and touchend (touch only).

Disabling the touch-only events fixes the issue, sort of. Click events also fire on touchscreens, but apparently

  • mousedown -> move lots -> mouse up = click event
  • touchstart -> move lots -> touchend NOT trigger a click. (Movement must be fairly small.)

So maybe disabling the touch-only trigger would feel weird, so went with a different solution.

Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

The latest changes work well on my Android phone (Samsung Galaxy S22) and on iOS, but still exhibit the "open close open" or "close open close" behavior in the Firefox mobile simulator. I'm okay with approving it because it's just the mobile simulator in FF that doesn't work, and it's possible that it's just a bug with the FF simulator itself and the way it simulates touch events. Otherwise this all works fine.

@ChristopherChudzicki
Copy link
Contributor Author

@gumaerc Tested this a lot more on my iphone this morning and various emulators on my mac. It seems to be working well. Very occasionally my iphone iOS safari seems to drop a click event when spamming taps very fast. This has been (a) rare, and (b) only under very spammy conditions, so I'm going to accept it for now.

@ChristopherChudzicki ChristopherChudzicki merged commit a066cc5 into main Oct 24, 2024
12 checks passed
@rhysyngsun rhysyngsun deleted the cc/nav-a11y branch February 7, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants