Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
flaviendelangle
left a comment
There was a problem hiding this comment.
Code Review: [popups] Ignore pinch-zoom shifting
Rating: 4/5 ⭐⭐⭐⭐
Overview
This PR extends the existing layout-viewport-based shift() behavior — previously scoped only to context menus (shiftCrossAxis) — to all popups when the user is actively pinch-zooming (visualViewport.scale !== 1). This prevents disorienting popup movement during pinch zoom on touch devices.
What's Good
- Minimal, focused diff — net +4/−3 in a single file. The change is easy to reason about.
- Correct use of project utilities — uses
ownerWindow()as required by project conventions, instead of accessing the globalwindowdirectly. - SSR-safe —
win.visualViewport?.scale ?? 1gracefully handles environments wherevisualViewportisundefinedornull. - Backward compatible — the
shiftCrossAxiscondition is preserved in the OR expression, so existing context menu behavior is untouched. - Clean variable naming —
useLayoutViewportclearly communicates intent and replaces a bareshiftCrossAxisthat was harder to read in context.
Concerns
- Tests were added then removed — The first commit introduced well-structured unit tests that validated both the default behavior and the pinch-zoom case. The second commit deletes them entirely. This leaves the new
visualViewport.scalecode path without any test coverage. Could these tests be kept, perhaps adjusted to be less reliant on mocking Floating UI internals? Even a simpler assertion (e.g., verifying theuseLayoutViewportlogic in isolation) would improve confidence against regressions.
Minor Nits
- The dependency array at line 310 doesn't include
visualViewport.scale, but this is correct — the value is read dynamically inside the callback each time Floating UI recalculates positioning, so it doesn't need to be a memoization dep.
Summary
Clean, well-scoped fix that correctly generalizes an existing safeguard. The only real concern is the removal of test coverage in the second commit — I'd love to see at least some test retained for the new behavior.
|
This should be built into Floating UI as a string option due to floating-ui/floating-ui#3387 unless we implement that on our side. It's pretty rare to encounter but it does happen, so I'll try to do a release that handles this |
Shared anchor positioning now switches
shift()to the layout viewport only detection while pinch-zoomed, which keeps Navigation Menu and other popups from moving around when the visual viewport scales during pinch zoom, which is disorienting. It continues to support virtual software keyboard collision detection.Changes
shift()whenvisualViewport.scale !== 1.