fix(popover): account for css zoom in positioning - #31426
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. |
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.
|
Thanks @caspinos for taking this forward and for the detailed investigation. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
||
| /** | ||
| * `currentCSSZoom` exposes the exact effective zoom of an element | ||
| * (Chromium 126+). When available we use it directly. |
There was a problem hiding this comment.
currentCSSZoom landed in Chrome/Edge 128, not 126 as shown on MDN.
| * (Chromium 126+). When available we use it directly. | |
| * (Chromium 128+). When available we use it directly. |
| /** | ||
| * `offsetWidth` is rounded to an integer while the bounding rect is not, | ||
| * so the ratio is rarely exactly 1 even when no zoom is applied. Treat | ||
| * sub-pixel differences as "no zoom" so that unzoomed popovers are not | ||
| * shifted by the rounding error. A real zoom deviates far more than this. | ||
| */ |
There was a problem hiding this comment.
The ratio is approximate rather than exact, and the comment should say so. offsetWidth is integer rounded, so the detected factor keeps a small error once a real zoom is present: on WebKit I measured 1.5033 against an actual 1.5 on a 56px popover.
It is harmless at realistic widths and well inside the 2px tolerance the e2e tests use. Worth writing down because the tolerance only snaps the unzoomed case to exactly 1, so nothing bounds the error in the zoomed case, and a later change that tightens an assertion against the detected zoom would be surprised by it.
| /** | |
| * `offsetWidth` is rounded to an integer while the bounding rect is not, | |
| * so the ratio is rarely exactly 1 even when no zoom is applied. Treat | |
| * sub-pixel differences as "no zoom" so that unzoomed popovers are not | |
| * shifted by the rounding error. A real zoom deviates far more than this. | |
| */ | |
| /** | |
| * `offsetWidth` is rounded to an integer while the bounding rect is not, | |
| * so the ratio is rarely exactly 1 even when no zoom is applied. Treat | |
| * sub-pixel differences as "no zoom" so that unzoomed popovers are not | |
| * shifted by the rounding error. A real zoom deviates far more, though | |
| * the same rounding leaves the detected factor approximate. | |
| */ |
| * `document.documentElement`, otherwise a zoom applied lower in the tree is | ||
| * missed entirely. | ||
| */ | ||
| test.describe('zoom applied at other levels of the tree', () => { |
There was a problem hiding this comment.
Please add a case where the zoom wraps only the trigger, on top of what is here. The existing cases all put it on html or body, an ancestor of both elements, so the popover and the trigger never end up under different zooms. That split is what popoverController.create() produces by default, since the overlay is appended to ion-app.
<style>
.panel { zoom: 1.5; }
</style>
<div class="panel">
<button id="trigger">Trigger</button>
</div>
<ion-popover trigger="trigger">
<ion-content class="ion-padding">Content</ion-content>
</ion-popover>zoomedPage() only takes the styles, so this needs its own markup. It already passes on your branch, so it guards a regression rather than fixing anything: reading the factor from contentEl instead of document.documentElement is what makes it hold.
| * `currentCSSZoom` exposes the exact effective zoom of an element | ||
| * (Chromium 126+). When available we use it directly. | ||
| */ | ||
| const currentCSSZoom = (el as unknown as { currentCSSZoom?: number }).currentCSSZoom; |
There was a problem hiding this comment.
currentCSSZoom is already declared on Element in lib.dom.d.ts, both in the TypeScript 6.0.3 the repo is on and in the copy Stencil 4.44.2 bundles, so the cast is not needed. Lints and typechecks clean without it.
| const currentCSSZoom = (el as unknown as { currentCSSZoom?: number }).currentCSSZoom; | |
| const currentCSSZoom = el.currentCSSZoom; |
Keep the typeof guard below either way. The lib declares it as a required number, but WebKit has not shipped it and returns undefined at runtime, so that check is load-bearing despite what the type says.
| */ | ||
| configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { | ||
| test.describe(title('popover: zoom'), () => { | ||
| test('should centre the arrow on the trigger when a zoom is applied', async ({ page }) => { |
There was a problem hiding this comment.
Please extend this to the other sides. calculateArrowPosition branches per side and each branch now runs on zoom-normalized dimensions, but only the default bottom is covered. /popover/test/arrow/ has a trigger per side to model it on, and these should stay functional rather than screenshots.
The assertion here only holds for top and bottom though. The side positions centre the arrow vertically instead, so they need a different expectation.
| */ | ||
| configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { | ||
| test.describe(title('popover: zoom'), () => { | ||
| test('should centre the arrow on the trigger when a zoom is applied', async ({ page }) => { |
There was a problem hiding this comment.
| test('should centre the arrow on the trigger when a zoom is applied', async ({ page }) => { | |
| test('should center the arrow on the trigger when a zoom is applied', async ({ page }) => { |
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.