search

theKashey / react-remove-scroll

Removes and disables 📜in a "React" way
MIT License
804 stars 31 forks source link

Pinch zoom does not work on MacOS when content is not scrollable or content is at `scrollTop` 0 #75

Closed chesterhow closed 2 months ago

chesterhow commented 2 years ago

Bug

Even with the allowPinchZoom prop toggled to true, there are 2 cases where I am unable to pinch zoom on the content within <RemoveScroll> on MacOS using a trackpad (no issues on iOS and Android).

I've detailed the 2 cases below, but do let me know if I am perhaps misunderstanding the allowPinchZoom prop or using it incorrectly in some way 😅

1. Content is not scrollable

https://codesandbox.io/s/react-remove-scroll-non-scrollable-content-unable-to-pinch-zoom-diiuw3

Steps to repro:

  1. Pinch zoom on the content.
  2. Pinch zoom does not work.

2. Content is scrollable but is at scrollTop 0

https://codesandbox.io/s/react-remove-scroll-scrolltop-0-unable-to-pinch-zoom-ezirwf

Steps to repro:

  1. Pinch zoom on the content while it is not scrolled down.
  2. Pinch zoom does not work.
  3. Scroll content (by any amount)
  4. Pinch zoom on the content.
  5. Pinch zoom works.
theKashey commented 2 years ago

Thank you for such detailed description. PinchZoom was designed/tested mostly for mobile phones (touch events) and probably not really working for trackpads.

theKashey commented 2 years ago

Confirmed - the current implementation cannot detect trackpad-based zoom and thinks that is a wheel event. In order to support one might need to use pointer events

atomiks commented 2 years ago

event.ctrlKey on the wheel event determines if it's pinch-zoom or not, doing a code search in the repo returned 0 results so I assume it's not being checked for yet 😃

theKashey commented 2 years ago

On my machine ctrlKey+wheel is a system wide zoom, ie it works on the display level.

atomiks commented 2 years ago

I'm not sure what you mean, can you clarify? I think apps like Figma check for event.ctrlKey to determine pinch-zooming on a trackpad to zoom in vs. pan, so it should be reliable.

Some info: https://kenneth.io/post/detecting-multi-touch-trackpad-gestures-in-javascript

EDIT: Okay I think I understand what you're saying now. I don't think that has an effect on this particular check though? It would simply not prevent scrolling on your machine while the system zooms in/out, which doesn't seem like it matters.

stale[bot] commented 1 year ago

This issue has been marked as "stale" because there has been no activity for 2 months. If you have any new information or would like to continue the discussion, please feel free to do so. If this issue got buried among other tasks, maybe this message will reignite the conversation. Otherwise, this issue will be closed in 7 days. Thank you for your contributions so far.

theKashey commented 2 months ago

@atomiks, sorry for a very long follow-up, and thank you for the very detailed page linked. I hope this issue will be solved in the next release.

theKashey commented 1 month ago

Released in v2.6.0

owenammann commented 1 month ago

Can we update react focus on as well? :) https://github.com/theKashey/react-focus-on/pull/93