fix(popover): account for css zoom in positioning - #31426
Open
caspinos wants to merge 1 commit into
Open
Conversation
When a CSS `zoom` other than 1 applies to the popover, geometry APIs like `getBoundingClientRect()` and pointer `clientX`/`clientY` report values in the zoomed coordinate space, while the inline `top`/`left`/`--width` styles the popover sets are interpreted in the unzoomed layout space and re-scaled by the browser. Applying the zoom factor twice placed the popover in the wrong location and, with `size="cover"`, gave it the wrong width. Read the effective zoom from the popover's own context via `currentCSSZoom`, so a zoom applied anywhere above it is picked up and accumulated zoom across ancestors is handled, falling back to the ratio between the bounding rect and `offsetWidth` where that property is unavailable. Normalize every rect-derived measurement by it: the trigger and content rects, the arrow dimensions, the `size="cover"` width, and the pointer coordinates used by `reference="event"`. `innerWidth`/`innerHeight` are not affected by CSS `zoom`, so scale them into the same space as well. Otherwise the offscreen adjustment clamps against a viewport larger than the space actually available and the popover can render past the edge of the screen. closes ionic-team#30919 Co-authored-by: KanhaiyaPandey <kanhaiyapandey2232@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014em3LPxMPQRPufMRz5i7so
|
@claude is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The zoom normalization approach is consistently applied and is backed by targeted unit/E2E coverage; only a minor naming typo remains.
Pull request overview
This PR fixes ion-popover positioning/sizing when CSS zoom is applied by normalizing geometry/pointer measurements into the unzoomed (layout) coordinate space, preventing the zoom factor from being effectively applied twice.
Changes:
- Add
getElementCSSZoom(usingcurrentCSSZoomwith anoffsetWidthfallback) and thread azoomfactor through popover positioning/sizing helpers. - Normalize trigger/content/arrow rect measurements,
reference="event"pointer coordinates, and viewport bounds (innerWidth/innerHeight) by the detected zoom factor in both MD and iOS enter animations. - Add E2E + unit coverage for zoomed scenarios (including accumulated zoom across ancestors,
size="cover",reference="event", and iOS arrow positioning).
File summaries
| File | Description |
|---|---|
| core/src/components/popover/utils.ts | Introduces zoom detection and normalizes rect-derived measurements/pointer coordinates in popover helpers. |
| core/src/components/popover/animations/md.enter.ts | Applies zoom normalization for MD positioning and viewport clamping. |
| core/src/components/popover/animations/ios.enter.ts | Applies zoom normalization for iOS positioning, arrow sizing, and viewport clamping. |
| core/src/components/popover/test/zoom/popover.e2e.ts | Adds functional E2E assertions validating popover geometry under various zoom setups. |
| core/src/components/popover/test/zoom/index.html | Adds a zoomed test fixture page with multiple trigger/popover configurations. |
| core/src/components/popover/test/util.spec.ts | Adds unit tests for zoom detection and zoom-normalized dimension helpers. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
183
to
+185
| const contentDimentions = contentEl.getBoundingClientRect(); | ||
| const contentHeight = contentDimentions.height; | ||
| let contentWidth = contentDimentions.width; | ||
| const contentHeight = contentDimentions.height / zoom; | ||
| let contentWidth = contentDimentions.width / zoom; |
Contributor
|
Thanks @caspinos for taking this forward and for the detailed investigation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: resolves #30919
Supersedes #31047, which this builds on. @KanhaiyaPandey is credited as co-author on the commit.
What is the current behavior?
When a CSS
zoomother than1applies to the popover,ion-popoverrenders incorrectly: it is positioned away from its trigger, and withsize="cover"it is given the wrong width. This affects a documented workflow — adjusting thehtmlzoom is the approach Ionic's documentation recommends for dynamic font scaling on Chrome for Android.The zoom factor is effectively applied twice. Geometry APIs (
getBoundingClientRect()on the trigger, content and arrow, plusclientX/clientYforreference="event") report values in the zoomed coordinate space. Those values are written straight into the inlinetop/left/--widthstyles on.popover-content, which are interpreted in the unzoomed layout space and then re-scaled by the browser.What is the new behavior?
currentCSSZoom, not fromdocument.documentElement. This picks up a zoom applied anywhere above the popover and accounts for zoom accumulated across several ancestors. Where the property is unavailable, it falls back to the ratio between the element's bounding rect and itsoffsetWidth; differences below a small tolerance are treated as no zoom, sinceoffsetWidthis integer-rounded and would otherwise report a phantom zoom.size="cover"width, and the pointer coordinates used byreference="event".innerWidth/innerHeightare scaled into the same space. They are not affected by CSSzoom, so leaving them alone made the offscreen adjustment clamp against a viewport larger than the space actually available, letting the popover render past the edge of the screen.1and every normalization is a division by1.This mirrors how Floating UI addressed the same problem in floating-ui/floating-ui#3492 —
Element.currentCSSZoomas both the value and the feature detector, with a default of1on engines that lack it. Their fix also had to scale the overflow bounds insidedetectOverflow(), which is the same class of issue as theinnerWidth/innerHeightpoint above.Does this introduce a breaking change?
The new
zoomparameters on the popover positioning helpers are optional and default to1. Those helpers are internal to the component and are not part of the public API.Other information
Tests
Eight E2E tests in
core/src/components/popover/test/zoom/, covering the review points raised on #31047:body; accumulated zoom (html1.2 ×body1.25)documentElementsize="cover"width matches the triggerreference="event"anchors to the pointer0.8) and zoomed in (1.5)All eight fail against
mainand pass with this change, so each one covers the regression rather than merely passing.These are functional assertions rather than screenshots: what is being verified is the popover's geometry relative to its trigger, and both boxes are read in the same coordinate space, so the relationship holds at any zoom level. No screenshot baselines are added.
Unit tests in
core/src/components/popover/test/util.spec.tscover the zoom detection itself — thecurrentCSSZoompath, theoffsetWidthfallback, the rounding tolerance — and the normalization of content, trigger and arrow measurements.Verification
The spec suite passes in full: 82 files, 714 tests, no failures.
The zoom tests are not skipped for any browser and pass on all three browser projects — Chromium, Firefox and WebKit — in both
iosandmdmodes. Assertions use a 2px tolerance to absorb sub-pixel differences between engines.