Skip to content

[popups] Fix transform origin for start/end alignment - #5015

Merged
atomiks merged 2 commits into
mui:masterfrom
atomiks:claude/sad-hertz-0bbe8c
Aug 26, 2026
Merged

atomiks merged 2 commits into
mui:masterfrom
atomiks:claude/sad-hertz-0bbe8c

Conversation

@atomiks

@atomiks atomiks commented Jun 11, 2026 •

Copy link
Copy Markdown
Contributor

Before: https://base-ui.com/react/components/combobox#input-inside-popup
After: https://deploy-preview-5015--base-ui.netlify.app/react/components/combobox#input-inside-popup

The transformOrigin middleware in useAnchorPositioning always derived the cross-axis origin from the arrow middleware data. Without an arrow element, a zero-size virtual arrow is used, which the arrow middleware centers against the anchor. That's correct for center alignment (the origin tracks the anchor's center even when the popup shifts), but for start/end the popup's edge is aligned with the anchor's edge, so the scale animation emanated from the anchor's center partway into the popup — and for an anchor wider than the popup, from a clamped point near the popup's far edge.

The cross-axis origin is now picked explicitly:

  • Arrow present → arrow's tip (unchanged, regardless of alignment, since the arrow points at the anchor).
  • No arrow, center align → anchor's center via the virtual arrow (unchanged).
  • No arrow, unshifted start/end align → the popup's aligned edge: 0%/100%. Uses the rendered alignment and Floating UI's RTL platform info, so it stays correct after alignment flips and in RTL.
  • No arrow, shifted start/end align → anchor's center via the virtual arrow, since the popup edge no longer lines up with the anchor edge.

The middleware is also simplified: instead of a four-entry side map plus a separate overlap string, it computes one cross-axis and one side-axis coordinate (the anchor-overlap case for context menus is folded into the side coordinate) and joins them in axis order.

Added Chromium tests asserting the exact --transform-origin value for center/start/end alignment, RTL start/end alignment, shifted start alignment, horizontal sides, sideOffset, alignOffset, and the arrow-present case.

@atomiks atomiks added type: bug It doesn't behave as expected. scope: all components Widespread work has an impact on almost all components. labels Jun 11, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jun 11, 2026 •

Copy link
Copy Markdown

commit: de70021

@code-infra-dashboard

code-infra-dashboard Bot commented Jun 11, 2026 •

Copy link
Copy Markdown

Bundle size

Bundle Parsed size Gzip size
@base-ui/react 🔺+74B(+0.02%) 🔺+41B(+0.03%)

Details of bundle changes

Performance

Total duration: 1,230.08 ms +117.86 ms(+10.6%) | Renders: 76 (+0) | Paint: 1,985.47 ms +194.34 ms(+10.8%)

Test Duration Renders
Scroll Area mount (300 instances) 80.49 ms 🔺+16.84 ms(+26.5%) 3 (+0)
Mixed surface mount (app-like density) 77.36 ms 🔺+13.95 ms(+22.0%) 5 (+0)
Tooltip mount (300 contained roots) 62.74 ms 🔺+12.46 ms(+24.8%) 1 (+0)
Select open (500 options) 57.29 ms 🔺+12.12 ms(+26.8%) 14 (+0)
Combobox open — 500 items 36.00 ms 🔺+6.87 ms(+23.6%) 4 (+0)

10 tests within noise — details

Metric alarms

Test Metric Change
Scroll Area mount (300 instances) bench:paint 🔺 +20.70 ms
Tooltip mount (300 contained roots) bench:paint 🔺 +18.41 ms
Select open (500 options) bench:paint 🔺 +30.48 ms
Select open (500 options) bench:paint#select-open 🔺 +30.48 ms
Combobox open — 500 items bench:paint#combobox-open 🔺 +20.31 ms

…and 1 more metric alarm — details


Check out the code infra dashboard for more information about this PR.

@netlify

netlify Bot commented Jun 11, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit de70021
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/6a8e72c2768b9d000820784a
😎 Deploy Preview https://deploy-preview-5015--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@atomiks
atomiks force-pushed the claude/sad-hertz-0bbe8c branch 3 times, most recently from cf30a2a to 01641f2 Compare June 12, 2026 00:39
@atomiks
atomiks marked this pull request as ready for review June 12, 2026 00:58
@oliviertassinari
oliviertassinari requested a review from jjenzz as a code owner June 27, 2026 20:50

@flaviendelangle flaviendelangle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PR review

This fixes the scale-animation transform origin for start/end aligned popups without an arrow: the origin now sits on the popup's aligned edge (0%/100%) instead of a clamped point derived from the virtual arrow. The rewrite is correct and the RTL/shift/overlap branches precisely mirror Floating UI's own positioning logic (verified against @floating-ui/core). Nothing is merge-blocking; test coverage is thorough (center/start/end, RTL, shifted, overlap, arrow-present, sideOffset, horizontal sides).

Bugs (0)

No findings.

Tests (0)

No findings.

Simplifications (0)

No findings.

Docs (0)

No findings.

Notes

ℹ️ 1. platform.isRTL! relies on the default DOM platform being in use

Location: packages/react/src/utils/useAnchorPositioning.ts:388 (new code)

crossOrigin =
  (renderedAlign === 'start') === (isVertical && platform.isRTL!(floating))
    ? '100%'
    : '0%';

platform.isRTL is optional in Floating UI's Platform interface, so the non-null assertion assumes a platform that implements it. This holds today — Base UI never passes a custom platform to useFloating, and the default DOM platform defines isRTL (floating-ui.dom.mjs:492) — and matching platform.isRTL(floating) is exactly right here because that is the same signal Floating UI uses to position the alignment. Just a heads-up that the ! encodes that invariant rather than guarding it; a ?. with a false fallback would degrade to LTR instead of throwing if that ever changes. Reasonable to leave as-is.

Verdict

Approve — correct, well-tested fix; the RTL/shift/overlap handling matches Floating UI's positioning math, and the only note is an optional defensive tweak.


🤖 Review generated with Claude Code

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 16, 2026
@jjenzz jjenzz self-assigned this Aug 20, 2026
@jjenzz

jjenzz commented Aug 25, 2026 •

Copy link
Copy Markdown
Member

i had my lil DX review skill take a look at this and it seems to think alignOffset also breaks the edge-to-anchor relationship, but this branch only checks middlewareData.shift? in the test mentioned below, apparently the popup moves 7px while --transform-origin remains 0%, so the documented anchor coordinate is now 7px away.

should we include the cross-axis value from middlewareData.offset? for example, make LTR start with alignOffset={7} produce calc(0% - 7px) 0px and fall back to the virtual arrow center when middlewareData.arrow.alignmentOffset is present?

perhaps we need to add end/RTL/horizontal and narrow anchor test coverage as well.

DX review

The core fix is sound, but the new edge-origin calculation overlooks other middleware that can move the popup away from its aligned edge. Nothing is blocking, though offset positioning should be handled before merge.

Findings (1)

1. 🟠 Alignment offsets leave the transform origin detached from the anchor

Source: packages/react/src/utils/useAnchorPositioning.ts:391, packages/react/src/popover/positioner/PopoverPositioner.test.tsx:647

Math.abs(isVertical ? middlewareData.shift?.x || 0 : middlewareData.shift?.y || 0) <= 1

The edge-origin branch only checks displacement from shift. alignOffset also moves the popup along this axis, but the added test explicitly preserves 0% after confirming that the popup moved seven pixels. This conflicts with the documented meaning of --transform-origin as the coordinates the positioner is anchored to and with the existing treatment of sideOffset.

The virtual arrow can similarly apply middlewareData.arrow.alignmentOffset for narrow anchors, moving an apparently “unshifted” popup without this condition noticing.

User scenario: A dropdown uses alignOffset={8} to align its content with the trigger text. Its popup opens from its own corner rather than the corresponding point on the trigger, producing a visible lateral drift during scale transitions. Very narrow icon triggers can encounter the same problem through the virtual arrow’s default padding.

Recommendation: Derive the origin from every cross-axis displacement, not only shift. Account for middlewareData.offset.x/y; when the virtual arrow reports alignmentOffset, fall back to its center as already done for collision shifts. Cover LTR, RTL, horizontal placement, and a narrow anchor.

What is already sound

  • The change adds no new public API or consumer mental model.
  • Using the rendered placement correctly preserves flipped alignment and floating-element RTL behavior.
  • The arrow-present, collision-shifted, overlap, side-offset, and horizontal-side cases receive strong browser coverage.

Verdict

Approve after nits — account for non-shift cross-axis displacement so the CSS variable continues to identify the actual anchor coordinate.


🤖 Review generated with Codex

@atomiks
atomiks force-pushed the claude/sad-hertz-0bbe8c branch from 50ce634 to 2b6a738 Compare August 25, 2026 11:12
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Aug 25, 2026
@atomiks
atomiks force-pushed the claude/sad-hertz-0bbe8c branch 2 times, most recently from 25921a4 to d017843 Compare August 25, 2026 11:49
@atomiks
atomiks force-pushed the claude/sad-hertz-0bbe8c branch from d017843 to d68c540 Compare August 25, 2026 12:24
@atomiks

atomiks commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@jjenzz I believe the current PR is correct, based on this recording. Fixing it causes it to slide (second column):

Screen.Recording.2026-08-25.at.10.32.12.pm.mov

@jjenzz

jjenzz commented Aug 25, 2026

Copy link
Copy Markdown
Member

Fixing it causes it to slide (second column):

ironically i think that's exactly what Codex was suggesting the fix was supposed to avoid, so yeah we don't want that 🙈 silly Codex.

@atomiks
atomiks merged commit e464fac into mui:master Aug 26, 2026
24 checks passed
@atomiks
atomiks deleted the claude/sad-hertz-0bbe8c branch August 26, 2026 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: all components Widespread work has an impact on almost all components. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants