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

Strategy "fixed" + pinch-to-zoom generates incorrect popper position in Safari #1121

Closed
s-bilous opened this issue Jun 4, 2020 · 12 comments · Fixed by #1576
Closed

Strategy "fixed" + pinch-to-zoom generates incorrect popper position in Safari #1121

s-bilous opened this issue Jun 4, 2020 · 12 comments · Fixed by #1576
Labels
browser bug This bug is related to an upstream software (usually the browser) and can't usually get fixed by us. PRIORITY: medium TARGETS: browser compat This issue affects only specific browsers.

Comments

@s-bilous
Copy link

s-bilous commented Jun 4, 2020

CodeSandbox demo

https://codesandbox.io/s/strange-hooks-nukkq?file=/src/index.js

Steps to reproduce the problem

  1. Open the demo in Safari (iOS or desktop)
  2. Open the sandbox embedded browser in a new window
  3. Zoom-in browser window using the pinch-to-zoom gesture
  4. Scroll the page

What is the expected behavior?

Popper should stay on the bottom side of the reference element when scrolling.

What went wrong?

Popper goes up as far as the scroll position changes.

Any other comments?

@s-bilous s-bilous added bug Something is not working. NEEDS: triage labels Jun 4, 2020
@FezVrasta
Copy link
Member

What version of Safari are you on? With 13.1 everything looks good to me.

@s-bilous
Copy link
Author

s-bilous commented Jun 4, 2020

The version is 13.1.1. It's important to use the pinch-to-zoom gesture on the trackpad, not command+plus. Or use a mobile browser.

IMG-9008

@FezVrasta
Copy link
Member

Oh ok thanks, I missed that step, yes I can reproduce it.

@FezVrasta FezVrasta added PRIORITY: medium TARGETS: browser compat This issue affects only specific browsers. and removed NEEDS: triage labels Jun 4, 2020
@danieldelcore
Copy link

Hey, we're running into this issue too!

Looking at the example, it seems like some logic kicks-in whenever it's pinch zoomed. The logic will manually reposition popper (see attached) on scroll thereafter.

I'm completely new to this project so sorry if I'm stating the obvious here but i hope it helps anyway - ha 😜 I'll have poke around today and see if i can find anything else.

Kapture 2020-06-15 at 13 38 35

@danieldelcore
Copy link

After some more digging i've found that the issue is related to this line:

https://github.com/popperjs/popper-core/blob/38914aae7a2e91715c6eb2b563517082a40cfa64/src/modifiers/computeStyles.js#L87

The result of this calculation x -= offsetParent.clientWidth - popperRect.width; changes when we use cmd + and cmd - but if you use pinch to zoom, the calculation remains the same, as if there was no zoom at all.

For example:

  • No zoom: x -= offsetParent.clientWidth - popperRect.width; = 366
  • Zoom: x -= offsetParent.clientWidth - popperRect.width; = 313
  • Pinch zoom: x -= offsetParent.clientWidth - popperRect.width; = 366

Right now i'm thinking that the offsetParent.clientWidth isn't being updated in Safari like we would otherwise expect.

@danieldelcore
Copy link

hmmm. appears that this is the culprit: https://stackoverflow.com/questions/6163174/detect-page-zoom-change-with-jquery-in-safari

Safari wont include the zoom in clientWidth & clientHeight but it will update the window.innerWidth / Height...

@danieldelcore
Copy link

Hi all, looks like this issue is already fixed in v2.4.1

See this PR

@atomiks we can probably close this issue for now, thanks 😄

@atomiks
Copy link
Collaborator

atomiks commented Jun 24, 2020

@danieldelcore I can still reproduce it updating to 2.4.1 & 2.4.2 in the CodeSandbox

@danieldelcore
Copy link

Sorry about that, yea can still confirm that this is an issue

@AndrewOCC
Copy link

I took some time to investigate this issue today, and it looks like like the issue is caused at least in part by a Webkit bug with calculating getBoundingClientRect() when using pinch-to-zoom. Popper.js wraps this function and uses it in a bunch of places to calculate positioning. I assume the same bug causes the inconsistencies with offsetParent that Dan mentioned above.

The bug is tracked in the Webkit issue tracker here, including a simpler popper example as a demonstration: https://bugs.webkit.org/show_bug.cgi?id=207089

I'll look into whether there's an alternate way we can calculate the reference's bounding box and position, and will update in a few hours

@atomiks
Copy link
Collaborator

atomiks commented Jul 3, 2020

Oh right I made that bug report 🤔

@FezVrasta FezVrasta changed the title Strategy "fixed" generates incorrect popper position in Safari Strategy "fixed" + pinch-to-zoom generates incorrect popper position in Safari Feb 20, 2021
@FezVrasta FezVrasta added browser bug This bug is related to an upstream software (usually the browser) and can't usually get fixed by us. and removed bug Something is not working. labels Feb 23, 2021
@daneah
Copy link

daneah commented Oct 2, 2021

We see an issue with misalignment depending on whether the Safari browser chrome (top navbar, bottom toolbar) is showing or not—is this potentially a flavor of this same issue since it's about calculating bounding boxes and may depend on offsets or clientHeight/innerHeight? Happy to file a separate issue if it seems different!

@FezVrasta FezVrasta added the Popper 2 This issue only applies to @popperjs/core@2.x label Dec 8, 2021
@atomiks atomiks removed the Popper 2 This issue only applies to @popperjs/core@2.x label Jan 15, 2022
ptu14 pushed a commit to ptu14/popper-core that referenced this issue Aug 6, 2022
ptu14 pushed a commit to ptu14/popper-core that referenced this issue Aug 6, 2022
ptu14 pushed a commit to ptu14/popper-core that referenced this issue Aug 6, 2022
ptu14 pushed a commit to ptu14/popper-core that referenced this issue Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser bug This bug is related to an upstream software (usually the browser) and can't usually get fixed by us. PRIORITY: medium TARGETS: browser compat This issue affects only specific browsers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants