[Tabs] Consider 3D transforms when positioning the indicator#4852
[Tabs] Consider 3D transforms when positioning the indicator#4852michaldudak wants to merge 8 commits into
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,154.97 ms +1.80 ms(+0.2%) | Renders: 50 (+0) | Paint: 1,770.45 ms -10.99 ms(-0.6%)
9 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
michaldudak
changed the title
michaldudak
added
component: tabs
michaldudak
requested review from
LukasTy,
atomiks,
colmtuite and
flaviendelangle
as code owners
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
PR #4852 Review Summary — [Tabs] Consider 3D transforms when positioning the indicator
Branch: tabs-indicator-3d · Author: @michaldudak · Diff: +132 / −46 across 4 files
Critical Issues (1)
- pr-test-analyzer: Tautological test — TabsIndicator.test.tsx:44-66 re-implements
getElementOffset/getRelativeLayoutOffsetbyte-for-byte identical to the production code at TabsIndicator.tsx:23-45. A formula regression would pass. Worse: the previous helper usedgetBoundingClientRect+ scale division, which was an independent oracle — that independence is now lost for every existing case routed throughassertBubblePositionVariables(lines 94, 116, 144, 175, 222, 271).
Important Issues (3)
- silent-failure-hunter:
offsetParent === nullis handled silently — TabsIndicator.tsx:23-34. Forposition: fixedtab lists with non-zero size, the existing width/height guards do not catch it; the function returns(0, 0)and the indicator anchors to viewport origin. The deletedhasNonZeroScaleelse-branch and the oldgetBoundingClientRect()-returns-zero path used to absorb this. Consider returningnulland bailing in the caller, or a dev-mode assertion. - pr-test-analyzer: Prehydration script has zero behavioral coverage — the only test (TabsIndicator.test.tsx:462-473) verifies the
<script>tag exists but never executes it. The duplicated math in prehydrationScript.template.js and.min.tscan drift fromTabsIndicator.tsxundetected. - silent-failure-hunter: Prehydration loop has no type guard — prehydrationScript.template.js:47-56. A non-
HTMLElementoffsetParent(e.g., SVG root) yieldsundefinedarithmetic →NaNCSS variables. The TSX-sideas HTMLElement | nullcast at TabsIndicator.tsx:25 masks rather than validates this.
Suggestions (4)
- pr-test-analyzer: Harden the 3D test (TabsIndicator.test.tsx:222-268) with fixture-derived literal expectations, e.g.
expectedLeft = 2*120 + 2*8 = 256. The 3D ancestor provesgetBoundingClientRect()math would be wrong; offset-walk should still return the literal CSS-box value — that's the assertion that catches drift. - pr-test-analyzer: Add coverage for
orientation="vertical", RTL, an intermediateposition: relativewrapper between tab and tab list, and a tab list that is itself the transformed element (transform makes it its descendants'offsetParent). - silent-failure-hunter: Add a
scaleX(0)regression test — the deletedhasNonZeroScalebranch was reachable. The new offset-walk handles it correctly, but only therotateYcase is exercised. - comment-analyzer: One-line comment justified on
- ancestor.clientLeft/- ancestor.clientTopat TabsIndicator.tsx:42-44 (and mirrors in template + test). The loop adds the ancestor's ownclientLeftonce, so the final subtraction is the non-obvious "remove ancestor's own border" step.
Strengths
- code-reviewer (full trace): Algorithm is mathematically correct across all traced cases — direct child, non-positioned wrapper, positioned wrapper with borders, no-
offsetParentroot. Common-ancestor contributions cancel exactly in the subtraction. - code-reviewer: Dropping
tabList.scrollLeft/scrollTopis correct —offsetLeftis unaffected by ancestor scrolling (unlikegetBoundingClientRect), so the old compensation is no longer needed. - code-reviewer: Removing the
hasNonZeroScalefallback is a net improvement — the old fallback to rawactiveTab.offsetLeftwas buggy for non-direct-child tabs anyway. - comment-analyzer: No stale comments left behind; no superfluous new ones added — consistent with project's default-no-comments stance.
- Bonus: minified prehydration script is smaller.
Recommended Action
- Address the tautology — replace the test's recomputed helper with literal pixel expectations derived from the CSS box model. This is the highest-leverage fix because it strengthens every case in the file, not just the new one.
- Decide on the
offsetParent === nullpolicy — either an early-bail (preferred) or a dev-mode assertion. Quick, low-risk. - Add a
scaleX(0)test alongside the 3D one to close the gap left by removinghasNonZeroScale. - Optional: one-line comment on the ancestor-
clientLeftsubtraction and aninstanceof HTMLElementguard in the prehydration loop.
The core change is sound and fixes a real bug (#4837). The blocking concern is test independence — fix that and the PR is in good shape.
|
Sizing is less ideal: Before: After: https://master--base-ui.netlify.app/experiments/tabs/tabs-overflow |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db87dce5f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0972d5dd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return hasTransform(element) || | ||
| (Math.abs(rectOffset[0] - layoutOffset[0]) <= 1 && | ||
| Math.abs(rectOffset[1] - layoutOffset[1]) <= 1) | ||
| ? rectOffset |
There was a problem hiding this comment.
getIndicatorOffset unconditionally returns rectOffset whenever the active tab has any transform, which reintroduces the projection bug this change is trying to avoid when the tab list is inside a 3D-transformed ancestor. In that mixed case (e.g., a transformed active tab for visual state + parent rotateY(...)), getBoundingClientRect() deltas are skewed, but this branch bypasses layoutOffset even when the mismatch is large, so the indicator can visibly drift from the tab; the same logic is mirrored in the prehydration script, so SSR/hydration can jump too.
Useful? React with 👍 / 👎.
3 participants
Fixes
Tabs.Indicatordesynchronizing when the tab list is inside a 3D-transformed ancestor.The previous implementation derived indicator coordinates from
getBoundingClientRect()deltas and an inferred scale factor. That works for simple translations and axis-aligned scale transforms, but breaks with rotations, skew, perspective, or 3D transforms because bounding rects are projected viewport geometry and cannot always be mapped back to tab-list coordinates with a single scale.This keeps the rect-based path for transforms it can represent accurately, and falls back to layout offsets when the active tab or one of its ancestors has a distorting transform. The fallback walks the
offsetParentchain for both the tab and tab list and subtracts their layout positions, so the indicator stays aligned because it lives in the same transformed subtree as the tab.The same logic is applied to the prehydration script, and browser regression tests cover rotated and 3D-transformed ancestors.
Fixes #4837