Repository navigation
feat(Slider): every pointer drags a thumb by its travel, as UISlider does - #8190
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
8620b4f to
6347e88
Compare
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Slider · Storybook | 🔴 81.7 kB (+37.3 kB) · 9.4 kB CSS (+489 B) | 2,326 (+1,270) | ✓ · ✓ · ✓ |
| AppShell | 65.9 kB (+62 B) · 12.1 kB CSS (0) | — | — · — · — |
| TreeList | 37.9 kB (+47 B) · 5.0 kB CSS (0) | — | — · — · — |
3 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Collapsible | 22.5 kB (+46 B) · 4.5 kB CSS (0) | — | — · — · — |
| SideNav | 63.4 kB (+44 B) · 10.9 kB CSS (0) | — | — · — · — |
| PowerSearch | 180.9 kB (+37 B) · 20.5 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 5.85 MB packed (+35.4 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 2 rows, core 23 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision: under a coarse pointer the Slider ignores track presses and fires onChangeEnd only after a coast, which changes component:Slider FR5, and the adaptive-menu threshold and the draft spec:AST-094 requirements have no current spec yet, so FR5 needs updating for the touch model and the threshold and the draft's requirements need approving or replacing with a governing spec. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision, please fire onChangeEnd from the handler that ends the touch interaction instead of the layout effect in packages/core/src/Slider/Slider.tsx, and shrink the 27,841-line stack by landing its dependency pull requests first. [Automated review]
| useIsomorphicLayoutEffect(() => { | ||
| if (touchSettle.count === 0) { | ||
| return; | ||
| } | ||
| touchController.handBack(); | ||
| if (touchSettle.report) { | ||
| fireChangeEnd(); | ||
| } | ||
| }, [touchSettle, touchController, fireChangeEnd]); |
There was a problem hiding this comment.
onChangeEnd fires here from a layout effect reacting to touchSettle, which architecture:react-component-runtime INV2 rules out, and it fires again if fireChangeEnd changes identity (a Slider switched between single and range values). Rendering the last value synchronously in the settle callback the controller calls when the interaction ends, and calling fireChangeEnd() there, would leave only handBack() in this effect.
There was a problem hiding this comment.
onChangeEnd has to come from the touch that ends the interaction, exactly once. Fixed in 889f550: the controller now ends an interaction from the handler that ends it (the lift, a cancel, the coast's last frame). flushSync renders the last value, the parts go back to the stylesheet, and onChangeEnd fires there with the rendered value (FR5). The layout effect is gone. A new test switches a single slider to a range after a touch: it fired twice through the old effect, and fires once now.
6347e88 to
889f550
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the touch model: under a coarse pointer the Slider ignores track presses and reports onChangeEnd only after a coast, which changes component:Slider FR5, and the adaptive-menu threshold and the draft spec:AST-094 requirements have no current spec yet. An engineering owner approves it by recording the Slider and menu-threshold changes in their component records and commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner; otherwise the touch path keeps FR5's track press and its end at release. Independently of that decision, please:
- keep a way to set a value without dragging under touch, since a drag is now the only pointer path (WCAG 2.2 SC 2.5.7);
- end a touch drag that is interrupted by disabling or unmounting the Slider without calling
flushSyncandonChangeEndfrom the effect cleanup inuseSliderTouch.ts; - land the dependency pull requests first so this one carries only the Slider commit.
[Automated review]
| } | ||
| // A finger or a pen under the touch presentation belongs to the thumbs' | ||
| // touch targets; the track ignores it, as UISlider's does. | ||
| if (isTouchPresentation && e.pointerType !== 'mouse') { |
There was a problem hiding this comment.
With this early return a finger or pen can change the value only by dragging the thumb: a tap on the track or a mark does nothing and a tap on the thumb changes nothing. WCAG 2.2 SC 2.5.7 (Dragging Movements, AA) asks for a single-pointer way that does not drag. Letting a tap that never passes the 10 px pan threshold set the value from the track, while a swipe still scrolls, would keep both.
| if (!isEnabled) { | ||
| return undefined; | ||
| } | ||
| return controller.attach(); |
There was a problem hiding this comment.
When this cleanup runs mid-drag (the Slider is disabled or unmounted), detaching the pan cancels it, and the cancel reaches finish: commit calls flushSync during React's commit (React logs "flushSync was called from inside a lifecycle method") and onChangeEnd fires from the cleanup, although dispose() says unmount stops without reporting. Ending live interactions silently before the recognizers detach, and naming the gesture host this Effect synchronizes, would keep the end report in the touch handlers.
889f550 to
79cc0ab
Compare
…fari) Recordings of the iOS 26.2 simulator (iPhone 17 Pro) in Mobile Safari, cropped to the page; the gray dot is the finger, drawn by the test page. main against each pull request: meta#8151 coincident range thumbs, meta#8168 translate travel, meta#8190 the touch presentation and its held-thumb fix.
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the touch model: under a coarse pointer the Slider ignores track presses and reports onChangeEnd only after a coast, which changes component:Slider FR5 and ORD2, while the new adaptive-menu query and the draft spec:AST-094 requirements have no current spec yet. An engineering owner settles it by recording the Slider and menu-query changes in those component records and commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner; otherwise the touch path keeps FR5's track press and its end at release. Independently of that decision, please:
- keep a single-pointer way to set a value without dragging under touch (WCAG 2.2 SC 2.5.7);
- end a touch drag interrupted by disabling or unmounting the Slider without calling
flushSyncandonChangeEndfrom the effect cleanup inuseSliderTouch.ts; - land the dependency pull requests first so this one carries only the Slider commit.
[Automated review]
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the touch model: under a coarse pointer the Slider ignores track presses and reports onChangeEnd only after a coast, which changes component:Slider FR5, while the new adaptive-menu query and the draft spec:AST-094 requirements have no current spec yet. An engineering owner settles it by recording the Slider and menu-query changes in those component records and commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner; otherwise the touch path keeps FR5's track press and its end at release. Independently of that decision, please:
- keep a single-pointer way to set a value without dragging under touch (WCAG 2.2 SC 2.5.7);
- end a touch drag interrupted by disabling or unmounting the Slider without calling
flushSyncandonChangeEndfrom the effect cleanup inuseSliderTouch.ts; - add a pen case to the touch tests, since the track now ignores pens and the thumbs take them;
- land the dependency pull requests first so this one carries only the Slider commits.
[Automated review]
| direction: this.model.isHorizontal ? 'horizontal' : 'vertical', | ||
| maximumNumberOfTouches: 1, | ||
| cancelsTouchesInView: false, | ||
| allowedPointerTypes: ['touch', 'pen'], |
There was a problem hiding this comment.
The thumbs now take pens and the track ignores them (Slider.tsx 888), but neither useSliderTouch.test.tsx nor the interaction spec drives a pen. architecture:interaction-modality asks a change adding pointer-dependent behaviour to verify mouse, touch and pen; one pen case (track ignored, thumb drag tracked) would cover it.
962ba39 to
fbd2d03
Compare
There was a problem hiding this comment.
Approval is on hold for an engineering owner's decision on the touch model: on iOS the Slider ignores track presses and reports onChangeEnd only after a coast, which changes component:Slider FR5, while the adaptive-menu threshold, the theme motion roles and platform condition, the module experimental subpaths and the draft spec:AST-094 requirements (whose FR4 says touch motion is not themable) have no current spec yet. An engineering owner settles it by recording these changes in their records and commenting /approve-spec <full head SHA> on this pull request, which the author may do as an engineering owner. Independently of that decision, please:
- keep a single-pointer way to set a value without dragging under touch on iOS (WCAG 2.2 SC 2.5.7);
- end a touch drag interrupted by disabling or unmounting the Slider without calling
flushSyncandonChangeEndfrom the effect cleanup inuseSliderTouch.ts; - add a pen case to the touch tests, since the track ignores pens and the thumbs take them;
- land the dependency pull requests first so this one carries only the Slider commits.
[Automated review]
| } | ||
| // A finger or a pen under the touch presentation belongs to the thumbs' | ||
| // touch targets; the track ignores it, as UISlider's does. | ||
| if (isTouchPresentation && e.pointerType !== 'mouse') { |
There was a problem hiding this comment.
With this early return a finger or pen on iOS can change the value only by dragging the thumb: a tap on the track or a mark does nothing, and a tap on the thumb changes nothing. WCAG 2.2 SC 2.5.7 (Dragging Movements, AA) asks for a single-pointer way that does not drag. Letting a touch on the track that ends before the pan begins set the value, while a swipe still scrolls, would keep both.
| useEffect(() => { | ||
| if (!isEnabled) { | ||
| return undefined; | ||
| } | ||
| return controller.attach(); | ||
| }, [controller, isEnabled, thumbCount, isHorizontal]); |
There was a problem hiding this comment.
When this cleanup runs mid-drag (the Slider is disabled or unmounted), detaching the pan cancels it and the cancel reaches finish: commit calls flushSync inside React's commit (React logs "flushSync was called from inside a lifecycle method") and onChangeEnd fires from the cleanup, although dispose() says unmount stops without reporting. Ending live interactions silently before the recognizers detach, and naming the gesture host this Effect synchronizes, would keep the end report in the touch handlers.
| direction: this.model.isHorizontal ? 'horizontal' : 'vertical', | ||
| maximumNumberOfTouches: 1, | ||
| cancelsTouchesInView: false, | ||
| allowedPointerTypes: ['touch', 'pen'], |
There was a problem hiding this comment.
The thumbs take pens here and the track ignores them (Slider.tsx 905), but neither useSliderTouch.test.tsx nor the interaction spec drives a pen. One pen case (track ignored, thumb drag tracked) would cover the mouse, touch and pen coupling for pointer-dependent behaviour.
| measures (0.55 for a scroll view and a sheet), and a release MUST pick its | ||
| rest point by projecting its velocity the way its UIKit counterpart does: | ||
| with a deceleration rate, or with a measured projection time. | ||
| - **FR4 — Physics are not themable in touch presentations.** A touch |
There was a problem hiding this comment.
FR4 says touch physics are not themable, and DEC-1 rejects tuning touch motion per theme, but this head lets defineTheme({motion}) retune every touch spring, duration and curve by role and resolves slider.releaseSpring under the thumb's theme. One of the two needs to change before an owner can approve the record.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not in the repository at this head or on main, so the departure FR7 and FR8 propose cannot be checked against it.
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
A platform condition widens the adaptation condition vocabulary, which the current theme authoring contract keeps fixed; it needs that contract amended (or an owner's decision) alongside the motion roles.
…eta#8190 24030a1) iOS 26.2 simulator, Mobile Safari. meta#8190's clips are re-recorded on the build whose lifted thumb grows around its center (the earlier clips showed it 5.5 px off the track); the catch is now side by side with the same flick alone (77 against 63); the past-the-end clip runs through the lift; the tap is held 0.3 s. meta#8151's tooltip clip hides the row label so the tooltips read clearly, and its coincident clip names the off-center press precisely.
24030a1 to
605287f
Compare
vjeux
changed the title
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested. Slider's new tracking matches component:Slider FR7 as it now stands on main, so no spec decision is pending, but a few implementation points need fixing before approval.
- Split the change (about 6,700 authored lines): the interaction-harness pacing and cast-step work and the UIKit
slidertable can land on their own. - Play the lift and drop springs through motion roles named in
component:Slider, as the release coast already does (spec:AST-094FR4). - Build the 48 px touch target without a touch-only element per thumb, for example as the thumb's
::before(spec:AST-017FR60), and add before-and-after element counts for the new wrappers. - Focus the thumb on a finger or pen press as the base did, which FR6 relies on for coincident thumbs, or get FR6 amended for touch in this pull request.
- Remove the two layout Effects that only mirror values into refs, play the end haptic only for touch and record its iOS gap in
component:Slider(spec:AST-094FR23), and drop the changeset's wheel clause, since a wheel never changed the value.
[Automated review]
| } | ||
| animator.animate( | ||
| {scale: UIKIT_SLIDER.liftScale}, | ||
| {spring: UIKIT_SLIDER.liftSpring}, |
There was a problem hiding this comment.
This spring (and dropSpring at 1081) reaches the animator as a literal: neither is a registered motion role, while spec:AST-094 FR4 says a component "MUST NOT play a literal". Naming them in component:Slider FR7, adding them to SPRING_ROLES and resolving them like the coast's springs.sliderRelease keeps one gesture on one tempo when a theme sets its motion knobs.
There was a problem hiding this comment.
Done in fcc9bcc. The lift and the drop play the springs.sliderLift and springs.sliderDrop roles, which component:Slider FR7 now names. The generated role table lists both.
| undefined, | ||
| positionStyle, | ||
| )}> | ||
| {hasTouchTargets && ( |
There was a problem hiding this comment.
This span exists only under touch, so a touch Slider renders one more element per thumb than a mouse one; spec:AST-017 FR60 keeps controls at their fine-pointer count, at rest and pressed. A ::before on the thumb with the insets as custom properties (the shape utils/hitArea.ts's HIT_INSET_PROPERTIES uses) gives the same 48 px target, and the recognizers can then attach to the thumbs in both presentations.
There was a problem hiding this comment.
Done in fcc9bcc. Each thumb's ::before is its touch target, drawn from four inset custom properties on the thumb, and the span is gone. A single slider renders 14 elements and a range 19, at rest and pressed, in every presentation (main: 11 and 16; the previous head added a span per thumb under a finger). The three added elements are the travel box for container units, the value anchor for the tooltip, and the fill's clip.
| }); | ||
| // Tells the pointer handlers, which run outside React's render, whether | ||
| // the touch layer takes touches. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
useSliderTouch returns its isEnabled input unchanged, and handlePointerDown already returns on isDisabled (line 1016), so touchLiveRef.current is always true where line 1029 reads it. This Effect names no external system (architecture:react-component-runtime INV3); deleting it and the ref leaves the behaviour unchanged.
There was a problem hiding this comment.
Removed in fcc9bcc. The press handler no longer reads it: any press on a thumb focuses the thumb and returns, and a finger or a pen on the track returns before the mouse path.
| const getPartsRef = useRef(getParts); | ||
| // Keeps the parts getter current for the controller, which reads it from | ||
| // gesture callbacks outside React's render. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
getParts reads only refs (Slider.tsx 1288-1305), so the first getter stays correct and this per-render Effect synchronizes nothing (INV3). Handing getParts to the controller once in the useState initializer is enough.
There was a problem hiding this comment.
Removed in fcc9bcc. Slider's parts getter reads only refs, so the controller keeps the first one. The hook returns nothing now.
| } | ||
| const end = this.endOf(value); | ||
| if (end != null && end !== interaction.atEnd) { | ||
| playHaptic(UIKIT_SLIDER.endHaptic); |
There was a problem hiding this comment.
record() runs for mouse drags too, so a mouse drag reaching an end vibrates wherever navigator.vibrate exists. spec:AST-094 FR23 ties a haptic to the moments a touch presentation's UIKit counterpart plays one, and asks the component record to state that iOS Safari plays nothing at these mid-drag and coast moments. Gating this on a touch interaction and adding that gap to component:Slider would settle both.
There was a problem hiding this comment.
Done in fcc9bcc. The end haptic plays only for a finger or a pen, and FR7 says iOS Safari has no haptic for that moment.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Slider: every pointer drags a thumb by its travel, as UISlider does. A press on a thumb no longer jumps the value to the pointer: once the pointer has moved 10 px, the value follows its further travel along the track. A finger or a pen claims the drag only along the track, so a swipe across the slider scrolls the page; a mouse claims in any direction. On a touch screen each thumb takes a finger or a pen through a 48 px target, capped at half the gap to a range's other thumb, and the held thumb lifts. After a finger's flick the thumb coasts on UISlider's measured spring for about 0.6 s, `onChange` firing through the coast, and `onChangeEnd` reports the value once it settles; a slow drag, a mouse drag and a pen drag end at the release. A tap on the track sets the value when the finger lifts, and a mouse click where it lands; a right click, a macOS ctrl-click, and a wheel or trackpad scroll change nothing. Under reduced motion the thumb does not lift and a tap's value applies at once. |
There was a problem hiding this comment.
The base Slider has no wheel handling, so "a wheel or trackpad scroll change nothing" already held; in a [fix] entry it reads as a change. Dropping that clause, or saying "as before", keeps the release note to what this pull request changes.
…y proof reads a still run against the cast's own step Near a coast's rest, WebKit's 2 px cast shows a new position about every step / pace frames, so a frame delivered early can leave a two-frame plateau right after two changes in a row. The rule "a run more than one frame longer than the one before it is a stall" read that as a stall (2 of 20 runs of meta#8190's Slider coast proof at four CPUs). Where the pace around the run is under the cast's own step, a proof with settledWithinSteps now allows a run of up to ceil(step / pace) frames plus one early frame; a longer run is still a stall. The pace around the run is the smaller of the motion over up to four frames before it and after the change that ends it, never across it, and the step is read from the cast, never the motion. These still fail: a three-frame freeze at 1.2 px a frame that resumes from where it held, a 40 ms freeze in a WebKit or a Chromium tail, a one-frame hold at two steps a frame or more, and a three-frame hold at 2 px a frame on a 1 px cast. A proof with settledWithin judges as before.
…does (slider A, end A) A press on a thumb changes no value; once the pointer has moved 10 px the value follows its travel, so the thumb never jumps to the pointer. A finger or a pen claims the drag only along the track, so a swipe across it scrolls the page; a mouse claims in any direction. Under a coarse pointer each thumb takes a finger or a pen through a 48 px target capped at half the gap to a range's other thumb, the held thumb lifts, a release coasts on UISlider's measured spring, and a tap on the track sets the value on lift. A mouse press on the track sets the value where it lands; a secondary click and a wheel or trackpad scroll change nothing. onChangeEnd fires once a finger's or pen's coast settles. UISlider's measured values live in one table.
…o rest It waited up to 4 s for the list's deceleration to finish, then went on whether or not the list had stopped. It now polls until two reads 100 ms apart agree and fails if they never do, so the horizontal drag that follows never runs on a moving list.
…nal capture names The table and its test named the lab's capture and case labels, which mean nothing outside the lab. They now cite the measurement the way the other UIKit tables do: the UIKit lab, the iOS 26.2 simulator, and what was read.
…ey measure is on screen The hit-area proof checks that both thumbs and both touch targets have boxes, and that where the thumbs have room each target is the full 48 px square around its thumb, before measuring the narrowed targets. The list proof checks its thumbs are visible before a swipe that must leave them still, the no-layout proof checks that its drag moved the value, and the desktop-style proof checks the thumb has a box before reading what it lacks.
… own frame The proof waited in the page for the coast, then asked for the block in a second round trip from the test. On a busy machine that trip outlasted the coast's motion and the block landed after the thumb had settled (a judged window of -17 to +8 ms in 10 of 10 runs on two pinned cores). The block now starts from the frame in which the coast passes 48 ms: the page watches the fill, which only a coast animates, from before the release, as blockMainThread asks.
…l, from a paced flick, to two cast steps from rest The busy-main-thread proof judges the coast against a compositor-only control recorded in the same screencast. Its flick, 900 px/s over 60 ms, keeps the script's spacing in WebKit, whose page reads the release velocity from when the moves arrive, so a stalled dispatch no longer bunches the moves behind it into a far faster throw whose coast reaches the end before the block. It judges until two of the cast's own position steps from rest, so WebKit's 2 px steps read as the cast's resolution, not a stall. The proof and the release filter's stillness reset now run in WebKit too.
…e, with no coast A finger's release coasts; a pen's, like a mouse's, ends where it lets go (component:Slider FR7). The case lifts a pen at 375 px/s, where a finger would coast, and fails if the release coasts.
…oaded runner cannot let the coast settle first The coast runs on the compositor's clock and reaches its target about 0.4 s after the lift. The test read the value during the throw, then the thumb's box, then played the catching tap on its own, and on a loaded runner those round trips sometimes outlasted the coast: the finger landed on a thumb that had settled at the target (1 of 40 runs at four workers on two cores). The lift value now comes from the script, and a second finger lands 48 ms after the lift, about where the coast has carried the thumb, in the same paced script.
3b02462 to
fcc9bcc
Compare
|
The rest of the review is done in fcc9bcc. A press on a thumb focuses it for every pointer, as before; a unit case covers a mouse, a finger and a pen, and fails on the previous head for the finger and the pen. The harness work lives in #9545, and this pull request runs on |
|
/approve-spec fcc9bcc |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: Slider's new tracking matches component:Slider FR7, and the two FR7 clauses this pull request adds are approved by an engineering owner at this head, but three things need fixing first.
- Split the change (about 6,100 authored lines); the
translatepositioning rewrite and its geometry spec can land on their own. - Call
observeResizefromutils/sharedResizeObserver.tsinstead of creating aResizeObserverper slider insliderTouchController.ts. - Play the end haptic only after a pan: a finger's tap on the track that sets an end vibrates today, which UISlider never does and FR7 does not ask for.
[Automated review]
| // paint, so nothing jumps at the hand-back. | ||
| const Observer = parts.control.ownerDocument.defaultView?.ResizeObserver; | ||
| if (Observer != null) { | ||
| const observer = new Observer(() => this.followTrackLength()); |
There was a problem hiding this comment.
Each slider creates its own ResizeObserver here, for every pointer, while it is enabled. Core already owns this: observeResize in utils/sharedResizeObserver.ts (react-component-runtime INV16, INV7). Suggest detachers.push(observeResize(parts.control, () => this.followTrackLength())); its first synchronous call is a no-op while no interaction is live.
There was a problem hiding this comment.
Done in a610788. attach() calls observeResize(parts.control, () => this.followTrackLength()), and its first synchronous call returns early with no live interaction. A unit case renders two sliders and counts one shared observer; the per-slider code made two.
| const end = this.endOf(value); | ||
| // UISlider's end haptic is a touch moment: a finger or a pen plays it, | ||
| // and a mouse does not. | ||
| if (end != null && end !== interaction.atEnd && !interaction.isMouse) { |
There was a problem hiding this comment.
A finger's track tap builds its interaction with isMouse: false, so a tap that sets an end vibrates, a moment UISlider does not have (spec:AST-094 FR23; the proposed FR7 limits it to a drag). Suggest adding && interaction.startValue != null, which only a begun pan sets.
There was a problem hiding this comment.
Done in a610788. The haptic also needs interaction.startValue != null, which only a begun pan sets. A unit case taps the track's end, which played the haptic before and plays none now, then drags into the minimum, which plays it once.
| // handlers below. | ||
| const isTouchPresentation = useTouchPresentation().isTouch; | ||
| const hasTouchTargets = isTouchPresentation && !isDisabled; | ||
| // Whether the slider's gestures are attached (useSliderTouch): the pointer |
There was a problem hiding this comment.
Non-blocking: this comment describes a flag that no longer exists and has no code under it. Delete it.
| }, | ||
| change: (index, next, from) => | ||
| updateValueRef.current(index, next, true, from), | ||
| // A finger paints the thumb pressed and opens its value tooltip. It does |
There was a problem hiding this comment.
Non-blocking: "It does not focus it" contradicts line 1029-1030 and 1243, where every press on a thumb focuses it. Suggest: "A finger paints the thumb pressed and opens its value tooltip. handlePointerDown already focused it."
…is drawn, and a coast that follows the track's live length, on main's harness - The lift and the drop play the `springs.sliderLift` and `springs.sliderDrop` motion roles, which `component:Slider` FR7 now names, as the release coast does with `springs.sliderRelease`. - Each thumb's 48 px touch target is its own `::before`, so a touch presentation renders no more elements than a mouse one, and the recognizers attach to the thumbs for every pointer. - A press on a thumb focuses it for every pointer, as the base did and FR6 relies on for coincident thumbs; a unit case fails on the previous Slider for a finger and a pen. - The two Effects that only mirrored values into refs are gone, and the changeset drops the wheel clause. The end haptic plays only in a finger's or a pen's drag and its coast: a mouse and a track tap play none, as on UISlider (FR7 records iOS Safari's gap). - A finger stops a coast where the browser draws the thumb: the stop reads the thumb's computed translate, not the animator's model of it, which a busy main thread reads past the frame the finger saw. - A coast and a drag follow the track's live length: core's shared ResizeObserver (`observeResize`) watches the track, and a length change re-plays a running coast scaled to the new travel or re-places a drag before the next paint. A value label that takes a digit mid-gesture no longer makes the thumb jump when the parts hand back. - Source comments keep only their reasons; `component:Slider`'s system specs and `spec:AST-094`'s path list carry the links the comments cited. - The proofs run on main's interaction harness. The busy-main-thread proof runs in Chromium; its WebKit form moves to a follow-up on the harness's paced touch play, since WebKit's injected touches bunch behind a stalled dispatch and its JPEG cast moves in 2 px steps. - The catch test sends its finger in its own play once the throw's lift has gone out, so a stalled dispatch cannot bunch the catch behind the lift. It reads the lift's value and the track's length at the lift, as the slider computes its coast from them. - Disabling a slider mid-drag cancels the drag: `onChangeEnd` fires once, with the value the owner rendered, after the Effect's cleanup. An unmount stays silent. - A collapsed track maps no travel, so a drag on it keeps the value. - The docs say what a pen does. - Comments use short sentences, one topic each, and file headers stay short.
cixzhang
added
the
ci:run
fcc9bcc to
a610788
Compare
|
On the split: this is a batch pull request, |
|
/approve-spec a610788 |
There was a problem hiding this comment.
This looks ready to merge: every behavior change matches component:Slider FR5–FR7 and DEC-4, and the FR7 clauses this revision adds carry an engineering owner's approval for it. The shared resize observer and the drag-only end haptic resolve the two points from the previous revision, and the unit, Chromium and WebKit proofs pass. At about 6,200 authored lines the size is fine as an owner-directed batch under the cap.
[Automated review]
Batch: Slider · finger tracking
Implements
component:SliderFR7 (#9407): every pointer drags a thumb by its travel, as UISlider does. It supersedes #8189, #8669 and #8168, which close when it merges. It runs onmain's harness.Evidence
iOS 26.2 simulator, Android 15 emulator; the gray dot is a finger.
User need
A Slider jumped its thumb to the pointer and trapped vertical swipes. UISlider moves the thumb by the travel and coasts after a flick.
Change
::beforeis a 48 px target. It lifts and drops on thesliderLiftandsliderDroproles, and a drag into an end plays UISlider's haptic where the browser vibrates. A flick coasts onsprings.sliderRelease, withonChangethrough it andonChangeEndwhen it settles. A finger on the coast stops it where it is drawn, and the coast follows the track's live length.onChangeEnd.Public delta and caller burden
No API change. A press no longer sets a value, a mouse drag starts after 10 px, and
onChangeEndfollows a flick's coast.Size
About +37 KB gzip, shipping as is; the live-length re-map reuses core's shared ResizeObserver. A follow-up loads the touch layer after hydration or on the first pointer. A single slider renders 14 elements (11 on
main), a range 19 (16): a travel box for container units, a value anchor for the tooltip, and the fill's clip. The touch target adds none.Design and alternatives
Keeping jump-to-pointer for a mouse was rejected: FR7 covers every pointer. #4050's resize mid-drag is handled: a drag and a coast follow the track's length. #7516's off-grid max is not addressed: with max 100 and step 3, a drag reaches 100 only past the end.
Current specification
component:SliderFR5–FR7, ORD2, DEC-4;spec:AST-094FR4, FR9, FR13, FR19, FR23, FR42, FR43;spec:AST-017FR60;design:reduced-motionDR2.Scope
Testing
Local gates at a610788: build, Storybook build, typecheck, strict lint (no warnings in changed files), check:knowledge, Slider's interaction specs in Chromium (23 passed) and WebKit (18 passed, 5 behind engine-limit guards), and the catch test ×80 at 4 workers on an exclusive 2-core pair (80 passed, no runner faults). Core and cli unit tests: 26,099 passed; 5 cli integration tests timed out at 30 s under machine load, and pass alone (211 of 211 in their three files), which this pull request does not touch. The provenance baseline check fails only on
main's own four stale entries (ChatComposer, Item, layerSurfacePress, Token), which this pull request does not carry; its own entries match.Red first: the focus case fails on the previous head for a finger and a pen; the disable-mid-drag case fails without the cancel; the zero-width case fails without the guard; a track tap that sets an end played the haptic before the pan check; two sliders made two observers before the shared one. On the previous stack, the catch's stop check failed 16 of 20 runs without the drawn-translate stop, and its visual check 20 of 20 without the live-length re-map.
The WebKit form of the busy-main-thread proof moves out of this pull request to a follow-up stacked on #9545, "Slider: the WebKit busy-main-thread proof on the paced touch play". On
main's harness WebKit's injected touches bunch behind a stalled dispatch, and its JPEG cast moves in 2 px steps, so the proof cannot judge the coast there. Here the proof runs in Chromium, and its WebKit run carries the harness's engine-limit guard until the follow-up replaces it.Writing rubric results
R1–R6 pass for every record edit: FR7's sentences, Slider's system specs, verification row and removed gap, AST-094's path list, the role table. No research, history or open questions.