feat: add hasFixedDirection to Slider for non-mirrored media controls - #10537
Open
dylanpulver wants to merge 2 commits into
Open
feat: add hasFixedDirection to Slider for non-mirrored media controls#10537dylanpulver wants to merge 2 commits into
dylanpulver wants to merge 2 commits into
Conversation
pointerDown installs mouseup/touchend/pointerup listeners on window. The two stacked-thumb tests never released the pointer, so on React 16 and 17 the listeners outlived the unmounted component and fired during a later test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10112
Adds
hasFixedDirectionto Slider,useSlider, anduseSliderThumb: an opt-in that keeps a horizontal slider left-to-right in RTL locales, for media controls like a playback progress bar.Direction is read in four places, and they mirror by two different mechanisms.
useSlider(track click, track drag) anduseSliderThumb(arrow keys, drag, thumbleft) invert in JS, butSliderFillmirrors in CSS viainsetInlineStart. Changing only the JS is the obvious fix and it pulls those apart: the thumb moves to the LTR position while the fill stays anchored to the right edge. A deliberately naive hooks-only patch failed exactly one test — theSliderFillone — which is why the fill switches to a physicalleftunder this flag. Vertical is untouched, and a mutant that short-circuited the whole inversion was caught by the vertical case.The flag lives on
SliderStaterather than being threaded through both hooks, followingorientation: that is the existing channel for a layout concern shared by the hooks andSliderFill, and it means S2 picks it up throughSliderBaseProps.On naming, I took
is | allows | haswith a default offalsefrom your comment, which is what rules outmatchLocaleDirection(it would have to default true). Happy to rename. I deliberately left the prose docs alone — thePropTableis generated from the JSDoc, so a rename won't strand written docs.Disclosure: this PR was written with AI assistance, pointed at
CLAUDE.mdandAGENTS.mdper your AI contribution guidance.✅ Pull Request Checklist:
📝 Test Instructions:
Storybook: React Aria Components / Slider / Slider Fixed Direction renders two sliders in an
ar-AE/dir="rtl"context. The first mirrors with the locale; the second setshasFixedDirectionand stays left-to-right, fill and thumb on the same side.14 tests added across
useSlider,useSliderThumb, and RACSlider, covering track click, arrow keys, drag, thumb position, and fill positioning — each with an RTL default control and an LTR no-op control, using value 25 so mirrored and non-mirrored results are distinguishable. Reverting the source fails 5 of them, one per behavior site.Tested: keyboard and mouse, LTR and RTL, horizontal and vertical, disabled, single and multi-thumb. Green across slider/color/S2 under
STRICT_MODE=1 VIRT_ON=1on React 19 (428 tests) and on React 16 and 17 viainstall-16/install-17(194).yarn formatandoxlintclean;check-typesreports nothing in the changed files (the only errors here are pre-existing unbuilt@spectrum-icons/workflowartifacts).Not tested: screen readers, high-contrast, and real-browser rendering. The fill/thumb agreement is asserted through inline styles in jsdom, not visually — worth a look on your side, since
insetInlineStartvsleftis ultimately a rendering concern.Second commit:
pointerDowninstallsmouseup/touchend/pointeruplisteners onwindow, and the two stacked-thumb track tests never released the pointer. On React 16 and 17 those listeners outlived the unmounted component and fired during a later test, which is how the new RTL track-click test found it — it was the first test in the file to dispatch a baremouseUpafterwards. Two lines, and removing them reproduces the failure on both versions.Note: #10498 touches five of the same files. The changes are orthogonal — cherry-picked onto its head, the three conflicts are adjacency only (a destructured prop, an appended
describe, an import list) and the combined tree is green. Happy to rebase behind it.🧢 Your Project:
Personal open-source contribution.