[direction provider] Fix RTL component behavior#4840
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,318.74 ms -261.79 ms(-16.6%) | Renders: 50 (+0) | Paint: 2,001.25 ms -390.52 ms(-16.3%)
7 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
5b7b979 to
9747e92
Compare
9747e92 to
78ccb2a
Compare
78ccb2a to
0a9ab67
Compare
Code Review (GPT-5.5)Approve ✅ The RTL interaction fixes are clean, and the remaining Scroll Area wheel edge/clamp coverage gap is now addressed. 1. Bugs / Issues (None)No branch-relevant actionable issues remain. Root Cause & Patch AssessmentThe PR now covers the direction-sensitive paths that caused the RTL regressions: Combobox chip/input arrow navigation, Combobox grid list navigation, vertical Navigation Menu open keys, Popover logical side callback data, and Scroll Area horizontal RTL wheel behavior. Test Coverage AssessmentClaude's useful follow-up was the optional Scroll Area wheel edge/clamp coverage gap. That is now covered by additional
Validated locally with:
|
flaviendelangle
left a comment
There was a problem hiding this comment.
Only missing tests, feel free to skip those:
PR #4840 Review Summary — [direction provider] Fix RTL component behavior
PR: #4840
Author: atomiks
Branch: codex/fix-rtl-behavior → master
Stats: +429 / -34 across 12 files
Critical Issues (3 found)
- pr-test-analyzer: Missing LTR equivalent of the "registers after scrollbar becomes visible" test ScrollAreaScrollbar.test.tsx — the actual production bug fix only has RTL coverage; an LTR regression would slip through CI.
- pr-test-analyzer:
directiondep on wheeluseEffectis untested ScrollAreaScrollbar.tsx:114 — no test verifies that togglingDirectionProvider directionre-attaches the listener with the new sign convention. - pr-test-analyzer: RTL grid navigation only covers horizontal axis ComboboxRoot.test.tsx:2563 — the LTR companion exercises
ArrowDown/ArrowUpcross-row navigation, but the RTL test only asserts horizontal. Cross-orientation RTL handling in useListNavigation.ts is unprotected.
Important Issues (3 found)
- pr-test-analyzer: Flaky-prone chromium test for "registers after scrollbar becomes visible" — relies on
viewport.scrollLeft === -50after a real layout pass without faking dims like the sibling test. Tighten withwaitFor/toBeLessThan(0)or useObject.definePropertylike the others. - pr-test-analyzer: RTL clamping test misses the partial-movement case ScrollAreaScrollbar.test.tsx — all RTL assertions hit exact edges (
0or-800). Add e.g.scrollLeft = -100,deltaX: 50→ expect-50to actually exercise the newMath.min/Math.maxarithmetic in RTL. - comment-analyzer: Missing "why" comment at ScrollAreaScrollbar.tsx:99-100 — the
minScroll = -maxScroll/maxScrollValue = 0swap encodes the non-obvious "RTL browsers report negative scrollLeft" convention. The pre-existingonPointerDownblock at line 175 already has a comment about this; the new wheel handler is now the inconsistent one.
Suggestions (8 found)
- pr-test-analyzer: NavigationMenuTrigger RTL test should also verify horizontal RTL still opens with
ArrowDownand thatArrowRightno longer opens it in vertical RTL NavigationMenuTrigger.test.tsx. - pr-test-analyzer: ComboboxChip RTL test should assert ArrowRight at non-zero
selectionStartis a no-op (guards against theselectionStart === 0check being dropped in the RTL branch). - pr-test-analyzer: Wrap
trigger.focus()inawait act(...)in the NavigationMenu RTL test to match the sibling pattern at line 119. - pr-test-analyzer / code-reviewer: Heavy duplication — the "allows horizontal scrolling away from the RTL start edge" test inlines ~40 lines instead of calling
renderWheelTest({ direction: 'rtl' }). - code-reviewer: Use
TextDirectiontype from@base-ui/react/direction-providerforrenderWheelTest'sdirectionparam instead of inline'ltr' | 'rtl'. - pr-test-analyzer: Add a nested-provider override test to DirectionProvider.test.tsx (inner provider overrides outer).
- pr-test-analyzer: Add an
inline-endsymmetric assertion to the popover RTL test. - silent-failure-hunter: Asymmetric RTL convention detection —
onPointerDownat ScrollAreaScrollbar.tsx:175 detects the negative-scrollLeftconvention at runtime, while the new wheel handler assumes it unconditionally. Consider sharing a helper (low priority; modern Blink follows the negative convention).
Strengths
- No critical correctness issues, no silent failures, no removed error paths. The math refactor (
Math.min/Math.maxclamping) is actually more robust than the prior strict-equality edge checks (sub-pixel tolerant). - Bug fix is well-implemented: hoisting
shouldRender+ adding it to theuseEffectdeps correctly fixes the missing-listener-after-deferred-measurement bug. React commits refs before effects run, so there's no listener-missing window. useDirection()contract is sound — defaults to'ltr', neverundefined. The new test pins this. Nodirection === 'rtl'site can silently coerce to the wrong default.ArrowDownis correctly NOT mirrored in NavigationMenuTrigger's horizontal-menubar path and ScrollArea's vertical wheel — direction only affects horizontal axis.rtl: direction === 'rtl'in AriaCombobox correctly uses the documented public option onuseListNavigationat useListNavigation.ts:187.- Test patterns follow project conventions:
it.skipIf(isJSDOM)for layout-dependent tests,Object.definePropertyfor fake dims (matches existing pattern in the same file),DirectionProvider.spec.tsxusesexpectType+@ts-expect-error. - The variable naming (
previousChipKey,nextChipKey,verticalOpenKey) is self-documenting — comment-analyzer flagged no junk comments and only one missing "why" (in the wheel handler).
ARIA spec check — ArrowRight → ArrowLeft mirror in vertical RTL menu
The WAI-ARIA APG Menu/Menubar pattern describes keyboard interactions assuming LTR only and contains zero references to RTL — an acknowledged documentation gap (the APG explicitly calls out RTL for sliders, spin buttons, and toolbars).
The de-facto convention across jQuery UI, Angular CDK, React Aria, and Radix UI is to mirror horizontal arrow keys for menus in RTL. The PR's change at NavigationMenuTrigger.tsx:773 matches that convention. ✅
Recommended Action
- Critical first: add LTR coverage of the listener re-registration bug fix (C1), add a
direction-switch wheel test (C2), and extend the RTL grid test with vertical/cross-row assertions (C3). - Then important: tighten the flaky chromium test (I1), add the partial-movement RTL clamping assertion (I3), and add the one-line "negative scrollLeft in RTL" comment in the wheel handler.
- Optional polish: dedupe
renderWheelTest, addawait actaroundtrigger.focus(), add nested-provider test. - No blockers for merge from a correctness standpoint — all three code-focused agents (code-reviewer, silent-failure-hunter, comment-analyzer) found zero critical issues. All Critical items are test-coverage gaps, not code defects.
Several direction-sensitive paths still treated left/right or wheel movement as LTR, so RTL keyboard and scroll interactions could stall or move the wrong way.
This also uncovered a bug with Scroll Area where the
wheellistener was not registered depending on the render timing.Root cause
Combobox, Scroll Area, Navigation Menu, and logical positioning coverage did not consistently pass or assert
DirectionProviderstate.Changes
scrollLeftrange.