Skip to content

[tabs] Fix activationDirection not updating on programmatic value changes#4347

Open
LukasTy wants to merge 14 commits intomui:masterfrom
LukasTy:claude/fix-tabs-activation-direction
Open

[tabs] Fix activationDirection not updating on programmatic value changes#4347
LukasTy wants to merge 14 commits intomui:masterfrom
LukasTy:claude/fix-tabs-activation-direction

Conversation

@LukasTy
Copy link
Member

@LukasTy LukasTy commented Mar 16, 2026

Fixes #3873

Alternative solution with one detection path, but the DOM as the source of truth to #3883.

Had to be very precise and explicit with tests to ensure this result.

Summary

Previously, activationDirection was computed inside TabsList's click handler (useActivationDirectionDetector), so programmatic value changes in controlled mode never updated data-activation-direction.

Approach

Direction detection is moved from TabsList to TabsRoot, where it can observe value changes regardless of origin. Two complementary mechanisms ensure correctness:

  1. Render-time detection — when value changes, direction is computed and set via setState during render, so Tabs.Panel and other children receive the correct tabActivationDirection on their very first render. previousValueRef is read-only during render to stay compatible with React 18 strict mode; it is synced after commit via useValueChanged.

  2. onValueChange handler — computes and sets direction together with setValue, populating eventDetails.activationDirection for the public callback.

Changes

TabsRoot.tsx

  • Added previousValueRef, computeActivationDirection (compares getBoundingClientRect of old/new tab elements), and the render-time detection block
  • Added useValueChanged(value, ...) to sync previousValueRef after commit
  • onValueChange now computes direction and sets tabActivationDirection directly
  • Moved getTabElementBySelectedValue earlier (before first usage)

TabsList.tsx

  • Removed useActivationDirectionDetector hook, getInset helper, and direction computation from onTabActivation
  • onTabActivation now delegates to onValueChange without intercepting direction

TabsRoot.test.tsx

  • Existing click-based tests now also assert Tabs.Panel receives the correct direction on first render via panelRenderMock
  • Added 2 new tests for programmatic value changes (horizontal + vertical) with first-render assertions

@LukasTy LukasTy self-assigned this Mar 16, 2026
@LukasTy LukasTy added component: tabs Changes related to the tabs component. type: bug It doesn't behave as expected. labels Mar 16, 2026
@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 16, 2026

commit: 85f5b91

@mui-bot
Copy link

mui-bot commented Mar 16, 2026

Bundle size report

Bundle Parsed size Gzip size
@base-ui/react 🔺+36B(+0.01%) 🔺+26B(+0.02%)

Details of bundle changes


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

@netlify
Copy link

netlify bot commented Mar 16, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 85f5b91
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/69bd3a8ef1343900085671ab
😎 Deploy Preview https://deploy-preview-4347--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@LukasTy
Copy link
Member Author

LukasTy commented Mar 16, 2026

Copilot (Opus) self-review:

Review Summary

The PR correctly solves the issue. No blockers found.

What the PR does

Moves activation direction logic from TabsList into TabsRoot and adds a render-time detection path for programmatic value changes. Two code paths now compute the direction:

  1. User click path (onValueChange): Direction is computed inline via computeActivationDirection(value, newValue) before calling the user callback and updating state — so eventDetails.activationDirection is populated correctly.
  2. Programmatic change path (render-time check): When the controlled value prop changes externally, a render-time if block detects previousValueRef.current !== value and computes the direction immediately, ensuring children see the correct direction on their very first render (no stale 'none').

The old useActivationDirectionDetector hook (~90 lines) is removed from TabsList — a nice simplification. It stored pixel-edge positions in state and needed a layout effect to keep them in sync. The new approach directly compares getBoundingClientRect() of old vs. new tab on demand.

Correctness analysis

  • Render-time setState pattern — Calling setTabActivationDirection inside the render-time if block is a documented React pattern. React synchronously re-renders before committing, so children see the correct direction.
  • No double-fire on clicks — When a user clicks a tab, onValueChange already calls setTabActivationDirection. On the subsequent render, the render-time if block computes the same direction and the activationDirection !== tabActivationDirection guard prevents a redundant state update.
  • previousValueRef update timing — The ref is updated after commit via useValueChanged (which uses useIsoLayoutEffect). During render, the ref still holds the previous value — exactly what's needed for the comparison.
  • React 18 Strict Mode — Double-renders re-execute the render-time block, but setTabActivationDirection bails out on same-value updates, so no issues arise.
  • RTL handling — Direction is positional (left/right), not logical. Matches the old behavior.

One thing worth noting

getBoundingClientRect() during render — The render-time if block calls computeActivationDirection, which reads DOM layout. This is technically impure, but:

  • It's a read-only operation — no layout mutations.
  • It's necessary to ensure children see the correct direction on their first render (the whole point of the fix).
  • The alternative (computing in an effect) would cause children to see 'none' first — defeating the purpose.
  • The comment in code clearly explains the rationale, linking back to the issue.

Pragmatic and justifiable tradeoff.

Tests

New programmatic-change tests cover both horizontal and vertical orientations. Existing click-based tests are strengthened with panelRenderMock assertions to verify children see the correct direction immediately.

@atomiks
Copy link
Contributor

atomiks commented Mar 17, 2026

Codex Review

Overview

This patch moves activation-direction detection from TabsList into TabsRoot so controlled, programmatic tab changes can populate the same direction state that click-based changes already use. The latest revision also adds good coverage for the previously flagged canceled-click and ignored-click cases, but the render-time path still assumes both tabs already exist in the committed DOM map.

Findings

1. 🟡 [Non-blocking] Adding and selecting a new tab in one controlled update still yields none

Impact: Consumers that append a tab and select it in the same render still get data-activation-direction="none" on the first panel render, so direction-aware animations stay incorrect for a common dynamic-tabs pattern even after this fix.

Evidence: The new render-time branch computes direction from the existing tabMap before the newly mounted tab has been registered (packages/react/src/tabs/root/TabsRoot.tsx:88-96). computeActivationDirection() then falls back to 'none' whenever either tab is missing from that map (packages/react/src/tabs/root/TabsRoot.tsx:281-333, especially :310-311). Replaying the reverted “add tab + select it” scenario against the current head in Chromium still fails: the first Tabs.Panel render receives tabActivationDirection: 'none' instead of 'right'.

Recommendation: Base the render-time calculation on render order (or another source that already includes tabs being mounted in the current render) instead of the previous commit’s tabMap, and add a regression test for “mount new tab + select it in the same update.”

No blocking issues found in this patch.

Confidence: 4/5

High confidence. I did a full branch pass, checked the updated control flow and test deltas, and verified the dynamic add-and-select scenario with a focused Chromium run.

Notes

  • The new on programmatic change after a canceled click and controlled parent ignores click tests cover the earlier directionJustComputedRef concern, so I would treat that one as resolved on the current head.

@LukasTy
Copy link
Member Author

LukasTy commented Mar 17, 2026

Final pass cleanup result:

  1. Eliminate double getBoundingClientRect on the click path — Added directionJustComputedRef that onValueChange sets before triggering state updates. The render-time detection block checks this flag and skips recomputation when it's already been done. The flag resets in a layout effect after commit.

  2. Extract computeActivationDirection to module level — The function is no longer recreated on every render. It now takes orientation and tabMap as explicit parameters.

  3. Single-pass tab element lookupcomputeActivationDirection now finds both old and new tab elements in one tabMap iteration with an early break, replacing two separate full-map scans via getTabElementBySelectedValue.

  4. Remove unused direction from context — Removed the useDirection() call and direction from the useMemo context value and its dependency array. No child component consumed it, and it wasn't even part of the TabsRootContext interface — it only caused unnecessary context invalidation when text direction changed.

  5. Replace useValueChanged with inline layout effect — A single useIsoLayoutEffect now syncs previousValueRef and resets the direction flag, eliminating the redundant internal ref that useValueChanged allocated to track the same value.

@LukasTy LukasTy requested a review from atomiks March 17, 2026 19:59
@atomiks
Copy link
Contributor

atomiks commented Mar 17, 2026

Minor issue in new review, above:

  1. 🟡 [Non-blocking] A canceled or ignored click can permanently disable the new programmatic path

@LukasTy
Copy link
Member Author

LukasTy commented Mar 18, 2026

Minor issue in new review, above:

  1. 🟡 [Non-blocking] A canceled or ignored click can permanently disable the new programmatic path

Fixed it.

@LukasTy LukasTy changed the title [fix] Fix activationDirection not updating on programmatic value changes [tabs] Fix activationDirection not updating on programmatic value changes Mar 18, 2026
@atomiks
Copy link
Contributor

atomiks commented Mar 19, 2026

New:

  1. 🟡 [Non-blocking] Adding and selecting a new tab in one controlled update still yields none
    Impact: Consumers that append a tab and select it in the same render still get data-activation-direction="none" on the first panel render, so direction-aware animations stay incorrect for a common dynamic-tabs pattern even after this fix.

@LukasTy
Copy link
Member Author

LukasTy commented Mar 19, 2026

New:

  1. 🟡 [Non-blocking] Adding and selecting a new tab in one controlled update still yields none
    Impact: Consumers that append a tab and select it in the same render still get data-activation-direction="none" on the first panel render, so direction-aware animations stay incorrect for a common dynamic-tabs pattern even after this fix.

Is it new?
Looks like the same issue that we explored and decided to keep "unfixed", since covering this edge case is really inelegant. 🙈

@atomiks
Copy link
Contributor

atomiks commented Mar 19, 2026

Maybe they have the same cause, but given Chrome Tabs have this UX (pressing to add a new tab selects it immediately) it seems like it would be common to hit (as it mentions)

@LukasTy
Copy link
Member Author

LukasTy commented Mar 20, 2026

Maybe they have the same cause, but given Chrome Tabs have this UX (pressing to add a new tab selects it immediately) it seems like it would be common to hit (as it mentions)

Good point and very precise example. 👍
I've pushed a fix with the following Claude summary of the situation and changes.

The solution provides an eventual correctness (the last render will have the correct activationDirection), it would be enough for certain use cases, but might not be correct in the particular use case outlined in the issue (+ controlled addition and selection).
The correctness will depend on the value used for the Tabs.
The optimistic first render fallback calculation uses JS-level string or number comparison.
This edge case behavior is outlined in the 2nd test I added here: 8fc4073


Now I understand the full picture. The issue is that when a new tab is added and selected in the same update, computeActivationDirection can't find the new tab element (not registered in tabMap yet) and returns 'none'. My previous fix deferred the ref sync so a retry happens after tabMap updates, but the first panel render still sees 'none'.

The root cause: computeActivationDirection relies on getBoundingClientRect() of DOM elements, but the new tab's DOM element doesn't exist yet during that render.

The cleanest fix: add a value-based fallback in computeActivationDirection when DOM elements aren't available. For comparable values (numbers, strings), comparing values gives the correct direction in LTR — the most common case. The directionComputationIncompleteRef retry mechanism still corrects edge cases (e.g. RTL) on the next render before paint.


The fix has two parts working together:

1. Value-based fallback in computeActivationDirection — When a tab element isn't in tabMap yet (but at least one is found, oldTab !== newTab), infer direction by comparing the values directly. Restricted to comparable types (numbers and strings) with a same-type guard. This gives the correct direction on the first panel render for the common case.

2. Deferred ref sync via directionComputationIncompleteRef — When the tab element is missing from tabMap, mark the computation as incomplete and keep previousValueRef stale. This ensures a re-computation from actual DOM positions happens once tabMap updates (before browser paint), correcting any edge cases where value ordering doesn't match DOM ordering (e.g., RTL layouts).

@LukasTy
Copy link
Member Author

LukasTy commented Mar 20, 2026

Claude Opus 4.6 follow-up review

Merge Confidence: 88/100

Summary

All 183 tabs tests pass (both Chromium and JSDOM). The PR cleanly fixes #3873 and the reported comment-thread concerns have been addressed (dynamic tab reverts applied per atomiks/LukasTy agreement).

What's solid

  • Core bug is fixed: Programmatic value changes now correctly update data-activation-direction. Previously, direction was only computed inside TabsList's click handler, so controlled changes were invisible.
  • Dual-path detection is correct: Render-time detection (for programmatic changes) + event handler detection (for clicks) with directionJustComputedRef preventing double-computation.
  • Clean removal of ~100 lines from TabsList — the old useActivationDirectionDetector, getInset, and related state are gone. The unused direction (RTL) field was also removed from context.
  • 8 well-written activation direction tests cover: horizontal/vertical clicks with first-render assertions, programmatic changes, canceled clicks, ignored controlled clicks, and dynamic tab addition.
  • Edge case handling: The previousValueRef is read-only during render and synced post-commit via layout effect — compatible with React 18 strict mode.

Minor concerns (not blockers)

  1. getBoundingClientRect() during render (TabsRoot.tsx): The render-time block calls computeActivationDirection which reads DOM layout. This is a side effect in render. It's safe in practice (read-only, no mutations, positions are stable) and was explicitly discussed in the PR comments — the team accepted it.

  2. Incomplete-computation retry mechanism (TabsRoot.tsx): directionComputationIncompleteRef keeps previousValueRef stale when a dynamically-added tab isn't yet in tabMap. This adds complexity but is tested. The string-value fallback ('Account' > 'Projects' via lexicographic comparison) can produce wrong intermediate direction, which the TabsRoot.test.tsx explicitly acknowledges — the first panel render gets 'left' but the final state is 'right'.

  3. No RTL-aware direction mapping: Neither the old code nor the new code map 'left'/'right' to logical directions based on text direction. getBoundingClientRect() returns viewport-relative coordinates, so in an RTL layout a visually "leftward" tab switch still returns 'left'. This isn't a regression — it was the same before — but worth noting for future consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: tabs Changes related to the tabs component. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[tabs] data-activation-direction not updated during programmatic value changes

3 participants