Skip to content

Freeze overlay positioning when pinch zooming #5612

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
Merged
merged 11 commits into from
Feb 5, 2024
Merged

Freeze overlay positioning when pinch zooming #5612

merged 11 commits into from
Feb 5, 2024

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Dec 20, 2023 •
edited
Loading

Closes #5757 #2700

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Go to the combobox storybook stories and open them outside the iframe in new window. Test pinch zoom when the dropdown is open

🧢 Your Project:

RSP

@rspbot
Copy link

rspbot commented Dec 21, 2023

Build successful! 🎉

@snowystinger
Copy link
Member

The overarching question we need to answer (so we don't forget what it is after the break) is:
Do we think that the overlaid element should maintain its size and position from before pinch zooming happens?
Or should it try to fit into the new viewport?

I personally feel it's better to maintain, that way a user can actually zoom on an overlay. If it tries to re-size to fit, then it may unintentionally move the item of interest in an unexpected way for the user.

@snowystinger
Copy link
Member

linking #5205

@yihuiliao
Copy link
Member

yihuiliao commented Jan 2, 2024 •
edited
Loading

+1 to rob's comment, i think the popover should maintain its position and not resize to fit

@rspbot
Copy link

rspbot commented Jan 8, 2024

Build successful! 🎉

@LFDanLu LFDanLu changed the base branch from main to submenu_flip January 9, 2024 22:48
@LFDanLu LFDanLu force-pushed the check_positioning_fix branch from 33bcf3c to cb25686 Compare January 9, 2024 22:48
@rspbot
Copy link

rspbot commented Jan 9, 2024

Build successful! 🎉

LFDanLu
LFDanLu commented Jan 9, 2024
View reviewed changes
@rspbot
Copy link

rspbot commented Jan 22, 2024

Build successful! 🎉

LFDanLu
LFDanLu commented Jan 22, 2024
View reviewed changes
packages/@react-spectrum/overlays/src/Modal.tsx Outdated
Comment on lines 87 to 91
// TODO: try getting rid of this reliance on useViewport size in favor of height: "100svh" and see if that makes the
// modal. Maybe to do later
let style: any = {
// TODO: look up why we have this and if we can use svh instead so we don't run into a modal with combobox in it + pinch zoom breaking the combobox dropdown placement
// not sure if that even happens though
Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior of the modal is the same before and after this change (aka it adjusts itself to fit the visual viewport), will need to revisit to see if we want its size to remain static as the user pinch zooms. For now we'll keep the same behavior

@LFDanLu LFDanLu changed the title (WIP) Rough fix for pinch zooming Freeze overlay positioning when pinch zooming Jan 22, 2024
@LFDanLu LFDanLu marked this pull request as ready for review January 22, 2024 21:31
@rspbot
Copy link

rspbot commented Jan 23, 2024

Build successful! 🎉

1 similar comment
@rspbot
Copy link

rspbot commented Jan 23, 2024

Build successful! 🎉

Base automatically changed from submenu_flip to main January 23, 2024 21:43
@LFDanLu LFDanLu force-pushed the check_positioning_fix branch from b5db74f to 293d4f2 Compare January 23, 2024 21:57
@LFDanLu LFDanLu force-pushed the check_positioning_fix branch from 3a48a1f to b5db74f Compare January 23, 2024 21:59
@LFDanLu LFDanLu changed the base branch from main to submenu_flip January 23, 2024 22:00
@LFDanLu LFDanLu force-pushed the check_positioning_fix branch from b5db74f to 3a48a1f Compare January 23, 2024 22:00
@LFDanLu LFDanLu changed the base branch from submenu_flip to main January 23, 2024 22:01
@rspbot
Copy link

rspbot commented Jan 23, 2024

Build successful! 🎉

@reidbarber reidbarber mentioned this pull request Jan 26, 2024
Closed
@LFDanLu LFDanLu linked an issue Jan 30, 2024 that may be closed by this pull request
Pinch-zoom on Mac OS with Popover opened #2700
Closed
@rspbot
Copy link

rspbot commented Feb 2, 2024

Build successful! 🎉

reidbarber
reidbarber approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

@rspbot
Copy link

rspbot commented Feb 5, 2024

Build successful! 🎉

@rspbot
Copy link

rspbot commented Feb 5, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@LFDanLu LFDanLu merged commit bcb5cc0 into main Feb 5, 2024
@LFDanLu LFDanLu deleted the check_positioning_fix branch February 5, 2024 20:17
This was referenced Feb 14, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktabors ktabors ktabors approved these changes

@reidbarber reidbarber reidbarber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

usePopover positioning issue when zooming in Pinch-zoom on Mac OS with Popover opened
6 participants
@LFDanLu @rspbot @snowystinger @yihuiliao @ktabors @reidbarber