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.

Sign up for GitHub

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

Jump to bottom

fix(430): ESC intermittently not working in Safari #431

Merged
merged 1 commit into from
Jul 29, 2023
Merged

fix(430): ESC intermittently not working in Safari #431

merged 1 commit into from
Jul 29, 2023

Conversation

rpearce
Copy link
Owner

@rpearce rpearce commented Jul 28, 2023

Description

Fixes #430. cc @steven-tey

There's a weird issue, probably related to Safari's :focus-visible behavior or their <dialog> implementation, where the <dialog> element's button doesn't receive focus when it's opened. It could also be a race condition because of the animation.

Regardless, let's try listening for Escape at the document level when we zoom an image. We need to make sure we provide the useCapture option and get to the event first so we can intercept it and don't trigger the default <dialog> Escape behavior so we can animate zooming out.

Testing

In your project

  1. Temporarily install react-medium-image-zoom@5.1.7-rc.0
  2. Run your code
  3. See if there are any issues using Escape when the images are zoomed!

Using a clone of this project

  1. Clone this project and go to the project directory
  2. Run npm i && npm start
  3. Go to http://localhost:6006 in macOS Safari
  4. See if there are any issues using Escape when the images are zoomed!

@rpearce rpearce self-assigned this Jul 28, 2023
@rpearce rpearce mentioned this pull request Jul 28, 2023
Closed
@rpearce
Copy link
Owner Author

rpearce commented Jul 28, 2023 •
edited
Loading

I also noticed (playing in Firefox) that if someone removes the unzoom button (as is done on https://nextjs.org/docs/app/building-your-application/rendering via visibility: hidden;), there's no focus to latch on to, and the <body> is focused. This is probably why this isn't working, and it's not a super accessible result, so I'm going to play with that as an alternative in this PR. In short: if there's no button that gets focused, focus the <dialog>, itself.

Edit: this may be a tree not worth barking up...

@rpearce
fix(430): ESC intermittently not working in Safari
ee5a4f5 
There's a weird issue, probably related to Safari's `:focus-visible`
behavior or their `<dialog>` implementation, where the `<dialog>`
element's button doesn't receive focus when it's opened. It could also
be a race condition because of the animation.

Regardless, let's try listening for `Escape` at the `document` level
when we zoom an image. We need to make sure we provide the `useCapture`
option and get to the event first so we can intercept it and don't
trigger the default `<dialog>` `Escape` behavior so we can animate
zooming out.
@rpearce rpearce force-pushed the fix/esc-in-safari branch 2 times, most recently from 0fd5f24 to ee5a4f5 Compare July 29, 2023 03:20
@rpearce rpearce added the bug label Jul 29, 2023
@rpearce rpearce merged commit 1df4826 into main Jul 29, 2023
2 checks passed
@rpearce rpearce deleted the fix/esc-in-safari branch July 29, 2023 03:22
@rpearce rpearce mentioned this pull request Jul 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rpearce rpearce

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Esc to exit zoom doesn't work on Safari
1 participant
@rpearce