fix(popover): correct positioning and sizing when html zoom is applied - #31047
fix(popover): correct positioning and sizing when html zoom is applied#31047KanhaiyaPandey wants to merge 1 commit into
Conversation
|
@KanhaiyaPandey is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
|
Actually, Firefox now supports CSS |
thetaPC
left a comment
There was a problem hiding this comment.
Thanks for working on this! This is an important fix. I've researched how other positioning libraries handle CSS zoom and I have some questions about the current approach.
Current Approach - Good Start ✅
You're correctly:
- Reading the
zoomCSS property viagetComputedStyle() - Including a fallback for older browsers
- Applying the zoom factor to positioning
Questions/Gaps to Address
1. What if zoom is applied at different DOM levels?
Your implementation checks document.documentElement.zoom. But what if a developer applies zoom at a parent level instead? How does the fix handle accumulated zoom across multiple ancestors? Have you tested zoom at different levels in the ancestor chain?
2. Where are you reading the zoom from?
Are you getting zoom from documentElement, or from the popover element's own context? These could be different. Which one is correct for positioning the popover?
3. Does size="cover" work?
The issue specifically mentions size="cover" breaks with zoom. Did you verify that the sizing calculations (not just positioning) account for zoom? How does the width/height calculation change with zoom applied?
4. Are pointer coordinates handled?
If the popover uses pointer events (touch/mouse), are those coordinate adjustments also accounted for? Or only DOMRect positioning?
5. Arrow positioning
Does the popover have a separate arrow element? If so, is its positioning also adjusted for zoom?
Edge Cases to Test
Before marking this ready, please verify:
- Zoom at documentElement level
- Zoom at intermediate parent level
- Zoom at multiple levels (accumulated)
- Popover with
size="cover"+ zoom - Arrow alignment with zoom
- Pointer-based interactions with zoom
Research Reference
I'd suggest looking at how Floating UI solved this (PR #3492) for comparison. They handle zoom differently in some key ways that might be relevant.
|
Hi — I'm the reporter of #30919, thanks for picking this up. I've been testing this area against The offscreen adjustment isn't zoom-awareIn I think the premise behind it is the comment in Repro: MD mode, Pixel 5 viewport, What makes this easy to miss: alignment-to-trigger and For transparency: the code path above is from this PR's head, but I confirmed the behaviour on an equivalent local implementation rather than by running this branch. On the review feedbackWhile testing I ended up with working code for three of @thetaPC's points:
Plus unit tests for the zoom detection and an E2E case covering the offscreen behaviour above. @KanhaiyaPandey happy to hand any of that over if it's useful — a PR against your branch, or just the diff in a comment, whichever you prefer. Not trying to duplicate your work, I'd just like to see this fixed. |
|
@caspinos thanks for digging into this. The offscreen clamp finding is a good catch, and the items you listed (cumulative zoom, pointer normalization for @KanhaiyaPandey hasn't been active here in a while, so rather than wait longer: please open a new PR with your work and we'll review it there. Two asks for the new PR:
If a PR isn't up by September 8, I'll open one on our side so this doesn't sit any longer. We'll close this one out once a replacement is open. |
🐛 Issue #30919
When CSS
zoomis applied on thehtmlelement (e.g.zoom: 1.5), the popover is rendered in an incorrect position.Additionally,
size="cover"results in incorrect sizing.✅ Expected Behavior
Popover should be correctly positioned and sized regardless of the document zoom level.
🔧 Fix
DOMRectvalues and pointer coordinates based on the document zoom factor.Files updated
core/src/components/popover/utils.tsmd.enter.tsios.enter.ts🧪 Tests
size="cover"behavior underhtml { zoom: 1.5 }core/src/components/popover/test/zoom/popover.e2e.tszoomis not supported there.