Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Needs an engineering owner's decision, then changes.
RefreshControl ships here together with Core, CLI, Storybook and test-harness changes, so the Lab exemption does not apply, and no current spec record governs the component. An engineering owner needs to provide or identify that record (the pull mapping and trigger, the page top-edge claim on touch screens, the mouse and pen pull, the announcements, its motion roles, and the title prop name next to Spinner's label) or decide that it proceeds as Lab without one, and records the decision by commenting /approve-spec 11602123b977c5f3d9b2fa5f814c368301bab900 on this pull request; the author may do so as an engineering owner.
Required changes either way:
- A mouse or pen drag down over list text at the top becomes a pull and clears the text selection (
refreshTouchController.tslines 913-920), which contradictsspec:AST-094FR41: a drag that starts on selectable text keeps its normal selection. - The docs say "A trackpad's or a wheel's scroll at the top pulls too" (
RefreshControl.doc.mjslines 67, 146, 180 and 183), but the component and its tests never pull on mouse-wheel notches, and the Chinesetitletext describes a status line the component does not render. - At 8,241 authored lines with no stack, behavior-module or batch exemption, the change is too large for one review, so land the Core part (#9807) and the harness helpers separately from the component.
[Automated review]
| // A scroll selects nothing and keeps the page's selection; a pen's | ||
| // drag gives up what it selected before the pull took it. | ||
| if (pan.scrollType == null) { | ||
| if (pan.pointerType !== 'touch') { | ||
| clearDragSelection(); | ||
| } | ||
| holdSelection(); | ||
| } |
There was a problem hiding this comment.
A pen or mouse drag that moves down at the top becomes a pull here, and clearDragSelection() drops the text it was selecting. spec:AST-094 FR41 says a drag that starts on selectable text is not claimed and keeps its normal selection. Please start no mouse or pen pull on a drag that starts on selectable text, and drop the clearing.
| }, | ||
| usage: { | ||
| description: | ||
| 'Pull to refresh following UIRefreshControl, with a finger, a pen or a mouse: pulling the content down past the top reveals a spinner; pulling far enough starts a refresh while the pointer is still down; letting go settles the content just below the spinner until the refresh ends, then it slides back up. Under a finger (a coarse primary pointer, on iOS and Android alike) it replaces the browser’s own pull to refresh near the top of the page while mounted. A trackpad’s or a wheel’s scroll at the top pulls too. Pair it with a visible refresh button. useRefreshControl gives the same behavior to a custom indicator.', |
There was a problem hiding this comment.
"A trackpad's or a wheel's scroll at the top pulls too" does not match the component: mouse-wheel notches never pull (the wheel tests and FR43). Suggest: "A trackpad's two-finger scroll at the top pulls too; a mouse wheel's notches never do." The same sentence is at lines 146, 180 and 183.
| // Every pointer pulls with the same mapping, trigger, settle and drawing. A | ||
| // pen's or a mouse's pull clears the text selection that its drag started. A | ||
| // trackpad's two-finger scroll pulls once the content is at its top, and keeps | ||
| // the selection. Only under a coarse primary pointer does the pull replace the | ||
| // browser's own pull to refresh. Under a fine pointer, a programmatic refresh | ||
| // only reports busy and announces itself. |
There was a problem hiding this comment.
Non-blocking: this paragraph repeats useRefreshControl.ts lines 23-27 almost word for word. Keep it in one file, with only the reasons.
1160212 to
4592820
Compare
There was a problem hiding this comment.
Needs an engineering owner's decision, then changes.
RefreshControl ships here together with Core, CLI, Storybook and test-harness changes, so the Lab exemption does not apply, and no current spec record governs the component. An engineering owner needs to provide or identify that record (the pull mapping and trigger, the top-edge overscroll claim on touch screens, the mouse, pen and trackpad pull, the Refreshing and changed-label announcements, the programmatic refresh that shows no indicator under a fine pointer, and its motion roles) or decide that it proceeds as Lab without one, and records the decision by commenting /approve-spec 4592820c910c9cbddcbf00d70f4f106af4ec68bf on this pull request; the author may do so as an engineering owner.
The text-selection drag, the wheel wording in the docs and the label name are fixed since the last review.
Required changes either way:
- The pull re-implements Core's drag-selection hold instead of calling it:
selectionOf(RefreshPullRecognizer.tslines 137-148) andrestoreSelection,holdSelectionandreleaseSelection(refreshTouchController.tslines 327-360) copyselectionRangesOfandlockDragSelectionfrompackages/core/src/utils/selectionLock.tsand already lack its document-wideselectstartrefusal andselectionchangerestore for WebKit, so re-export those two from the internal touch entry, call them, and delete the copies. - At 8,548 authored lines with no stack, behavior-module or batch exemption, the change is too large for one review, so land the Core part (#9807) and the harness helpers (#9872, #9874) separately from the component.
[Automated review]
| /** Puts back the page's selection as a pen's or a mouse's press found it. */ | ||
| const restoreSelection = useCallback((ranges: readonly Range[]) => { | ||
| const selection = contentElement.current?.ownerDocument.getSelection(); | ||
| if (selection == null) { | ||
| return; | ||
| } | ||
| selection.removeAllRanges(); | ||
| for (const range of ranges) { | ||
| selection.addRange(range); | ||
| } | ||
| }, []); | ||
| const holdSelection = useCallback(() => { | ||
| const element = contentElement.current; | ||
| if (element == null || selection.current != null) { | ||
| return; | ||
| } | ||
| selection.current = element.style.getPropertyValue('-webkit-user-select'); | ||
| element.style.setProperty('-webkit-user-select', 'none'); | ||
| element.style.setProperty('user-select', 'none'); | ||
| }, []); | ||
| const releaseSelection = useCallback(() => { | ||
| const element = contentElement.current; | ||
| if (element == null || selection.current == null) { | ||
| return; | ||
| } | ||
| if (selection.current === '') { | ||
| element.style.removeProperty('-webkit-user-select'); | ||
| element.style.removeProperty('user-select'); | ||
| } else { | ||
| element.style.setProperty('-webkit-user-select', selection.current); | ||
| element.style.setProperty('user-select', selection.current); | ||
| } | ||
| selection.current = null; | ||
| }, []); |
There was a problem hiding this comment.
These three callbacks copy Core's lockDragSelection (packages/core/src/utils/selectionLock.ts): they put back the press selection and set user-select: none on the content, but leave out Core's document-wide selectstart refusal and selectionchange restore, which Core keeps because WebKit extends a drag's selection beyond the held elements. Re-export lockDragSelection from @astryxdesign/core/internal/touch, hold the drag with it from the pull's start (line 913) to pullEnd or pullCancel (line 967), and delete these copies.
| /** A copy of the selection in the document a contact landed in. */ | ||
| function selectionOf(touch: GestureTouch): readonly Range[] { | ||
| const target = touch.target; | ||
| const selection = | ||
| target instanceof Node ? target.ownerDocument?.getSelection() : null; | ||
| if (selection == null) { | ||
| return []; | ||
| } | ||
| return Array.from({length: selection.rangeCount}, (_, index) => | ||
| selection.getRangeAt(index).cloneRange(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
selectionOf re-implements Core's selectionRangesOf (packages/core/src/utils/selectionLock.ts), which also drops a collapsed selection and reads ranges inside shadow roots. Re-export it from the internal touch entry and call it here.
| * Under Reduce Motion, the fade is the shared 200 ms opacity fade. It is a | ||
| * plain Web Animation: on a fresh element, the animator's start-up cost a | ||
| * busy main thread's first frame one more interval. The copy goes once its |
There was a problem hiding this comment.
Non-blocking (comments): this says Reduce Motion plays the shared 200 ms fade, but startSpinning skips the cross-fade under Reduce Motion (line 548), as the comment at line 216 says. Suggested: "Under Reduce Motion there is no copy and no fade (startSpinning)."
| /** CSS custom property the spokes read: the alpha step while spinning. */ | ||
| export const REFRESH_SPIN_STEP_VAR = '--astryx-refresh-spin-step'; |
There was a problem hiding this comment.
Non-blocking: nothing reads REFRESH_SPIN_STEP_VAR; useRefreshControl.ts only re-exports it. Delete both.
4592820 to
ade2f5b
Compare
vjeux
changed the title
There was a problem hiding this comment.
Needs an engineering owner's decision, then changes.
No current spec record governs useRefreshControl, and this pull request also changes Core, Storybook and a script test, so the Lab exemption does not apply. An engineering owner needs to provide or identify that record (the pull mapping and trigger, which pointers pull, the container's overscroll-behavior-y claim, when announcement is spoken, a pull while refreshing that calls onRefresh again, and a programmatic refresh whose indicator stays hidden), or decide that it proceeds as Lab, and record the decision with an /approve-spec comment naming the full head commit SHA, which the author may post if they are an engineering owner. These findings need changes either way:
- Miss: the hook sets
overscroll-behavior-y: noneon the nearestoverflow-y: autoancestor even when that container does not overflow (refreshTouchController.ts:294), so a short list nested in a scrolling page blocks wheel and touch scrolling of the page and pulls to refresh mid-page, againstspec:AST-025FR14 and FR18. - Newly live check: the end of a refresh slides over a fixed 300 ms (
refreshTouchController.ts:480), butspec:AST-094FR45 requires motion the program starts to play the theme's duration and easing tokens scaled by tempo. - At 4,489 authored lines with no qualifying exemption, the change is too large for one review, so land the Core internal-entry part on its own first and consider moving the story and browser spec to a follow-up.
[Automated review]
| const near = | ||
| isPullEnabledRef.current && | ||
| current != null && | ||
| scrollTopOf(current) < Math.max(heightOf(current), REFRESH_HEIGHT); |
There was a problem hiding this comment.
This claims overscroll-behavior-y: none on the nearest overflow-y: auto ancestor whether or not it overflows. A short list nested in a scrolling page then blocks wheel and touch scrolling of the page, and a swipe on it pulls to refresh mid-page. spec:AST-025 FR14 keeps containment to axes that overflow, and FR18 picks the nearest effective owner. Resolve the owner by overflow (content taller than its viewport), and claim only while it overflows.
| // Motion, the animator cross-fades the slide. | ||
| const from = content.animator.get('y'); | ||
| const element = contentElement.current; | ||
| const duration = refreshControl.endMs; |
There was a problem hiding this comment.
The program starts the end, when onRefresh settles or isRefreshing turns false, so spec:AST-094 FR45 has it play the theme's duration and easing tokens scaled by tempo, not the measured 300 ms. The browser spec's 300 ms assertion (line 909) changes with it.
| effects: [], | ||
| }; | ||
| case 'pullMove': { | ||
| // GUESS (M-RC3): a pull while refreshing counts from the inset. |
There was a problem hiding this comment.
Non-blocking: the header at lines 19-20 states this re-trigger as UIKit's behavior, but here it is a guess, and M-RC3 is named nowhere else. Suggested header line: "A pull while refreshing triggers again a trigger distance past the inset. No UIKit capture confirms this yet."
| * SYNC: When modified, update: | ||
| * - /packages/lab/src/RefreshControl/refreshTouchController.ts | ||
| * - /packages/lab/src/RefreshControl/useRefreshControl.test.tsx | ||
| * - /packages/lab/src/RefreshControl/useRefreshControl.doc.mjs |
There was a problem hiding this comment.
Non-blocking: useRefreshControl.doc.mjs does not exist, so this SYNC line points nowhere. Remove it.
ade2f5b to
1d3e1ae
Compare
|
/approve-spec 1d3e1ae |
There was a problem hiding this comment.
Needs an engineering owner's decision, then changes.
No current spec record governs useRefreshControl, and this pull request also changes Core, Storybook and a script test, so the Lab exemption does not apply. An engineering owner needs to provide or identify that record (the pull mapping and trigger, which pointers pull, the scroll container's overscroll-behavior-y claim, when the announcement is spoken, a pull while refreshing that calls onRefresh again, a programmatic refresh whose indicator stays hidden, and which theme duration the end plays), or decide that it proceeds as Lab, and records that decision by commenting /approve-spec <full head SHA> on this pull request, which the author may post as an engineering owner. These findings need changes either way:
refreshPull.tscopies Core'sparseCssEasingand duration parsing frompackages/core/src/motion/roles.ts, so please re-export Core's through the internal touch entry and delete the copies.refreshTouchController.tscreates its ownResizeObserverfor each instance, so please register with Core's sharedobserveResizeinstead.- Miss: each enabled instance keeps a capture
scrolllistener ondocumentwhile idle and under every pointer, whichspec:AST-094FR24 rules out, so please listen on the scroll owner instead. - Miss: the
announcementoption names a concept that Core'suseClipboardalready callsannounce(architecture:public-component-apiINV2, DEC-1), so please rename it. - Miss: the
GUESS (M-RC3)re-trigger names a measurement that no list carries, whichspec:AST-094FR2 requires, so please list it or measure it. - At 4,894 authored lines with no qualifying exemption, the change is too large for one review, so please land the Core internal-entry part first and consider moving the story and browser spec to a follow-up.
[Automated review]
| * A CSS easing as cubic-bézier control points. Null for an easing that has | ||
| * none, such as `steps()`. | ||
| */ | ||
| export function parseCssEasing(value: string): CurvePoints | null { |
There was a problem hiding this comment.
This copies Core's parseCssEasing (packages/core/src/motion/roles.ts), and parseCssTimeMs above copies parseMotionRoleValue('duration', …) from the same module. Please re-export those two from @astryxdesign/core/internal/touch, call them in endTiming, and delete the copies, so a fix to Core's token parsing reaches the refresh end too.
| // container resizes, with no scroll event. So the owner is read again on | ||
| // each resize of the content and of its scroll containers. | ||
| const resizes = | ||
| typeof ResizeObserver === 'undefined' ? null : new ResizeObserver(update); |
There was a problem hiding this comment.
Core already has one shared resize observer, observeResize from @astryxdesign/core/utils. Please register the content and each scroll container with it, and call the returned unsubscribe functions in the cleanup, instead of creating a ResizeObserver per hook instance.
| update(); | ||
| } | ||
| }; | ||
| document.addEventListener('scroll', onScroll, { |
There was a problem hiding this comment.
Miss: this keeps a capture scroll listener on document for every enabled instance, idle and under a fine pointer too. spec:AST-094 FR24 allows document listeners only during a press, and its verification fails a per-instance document listener. Please listen for scroll on the resolved scroll owner instead, re-attached when resolveOwner changes it.
| * Announced politely when a refresh starts, for example "Refreshing". The | ||
| * app supplies it in its own language. Omit it to announce nothing. | ||
| */ | ||
| announcement?: string; |
There was a problem hiding this comment.
Miss: Core's useClipboard already names this option announce: a localized message spoken in a polite live region, omitted for none. architecture:public-component-api INV2 and DEC-1 keep one word per concept, so please rename it announce.
| effects: [], | ||
| }; | ||
| case 'pullMove': { | ||
| // GUESS (M-RC3): a pull while refreshing counts from the inset. |
There was a problem hiding this comment.
Miss: spec:AST-094 FR2 has a guess name the measurement that will replace it, and its verification fails a guess without a listed measurement. M-RC3 is listed nowhere. Please list it, with its setup and the value it replaces, or measure it.
| const indicator = useMotionAnimator(); | ||
| const contentElement = useRef<HTMLElement | null>(null); | ||
| const indicatorElement = useRef<HTMLElement | null>(null); | ||
| // Whether the content holds the refreshing inset (only under a finger). |
There was a problem hiding this comment.
Non-blocking: begin() also sets this flag (line 766), so "only under a finger" is not true. Suggested: // Whether the content holds the refreshing inset.
| * @input Touches from the gesture arena (Core's internal touch entry) | ||
| * @output Exports RefreshPullRecognizer, the vertical pan that reads a pull | ||
| * to refresh without taking the touch, and RefreshScrollPullRecognizer | ||
| * @position The gesture of RefreshControl, on the foundation's |
There was a problem hiding this comment.
Non-blocking: no RefreshControl exists any more. Name useRefreshControl here and in useMotionAnimator.ts:10.
|
The Core internal-entry part lands first as #9807. This pull request then rebases onto it, so its own diff is Lab-only and the Lab exemption applies. |
1d3e1ae to
e60e757
Compare
There was a problem hiding this comment.
Changes requested.
The earlier findings are fixed: Core's parsers and observeResize replace the copies, no document listener remains, the option is announce, and the guess is gone. Two items remain:
- The scroll owner's
scrolllistener moves only insideupdate()(refreshTouchController.ts:353-365), so whenresolveOwner()changes the owner at touch-down, or the content mounts after the hook, the container's top edge is claimed only at touch-down and keepsoverscroll-behavior-y: noneafter scrolling away; please move the listener wherever the owner changes, re-run that setup whencontentRefattaches a node, and add a test for content that mounts after the hook. - At 4,817 authored lines with no qualifying exemption the change is too large for one review, so please either declare the existing stack with an identical
Stack: #9807 → #9865line in both pull request bodies or split it further.
[Automated review]
| const update = () => { | ||
| resolveOwner(); | ||
| applyEdge(); | ||
| listen(); | ||
| }; |
There was a problem hiding this comment.
This is the only place the scroll listener moves, but resolveOwner() also changes the owner at touch-down (line 706), at a pan's start (603), in end() (487) and in begin() (761), and content that mounts more than two frames after the hook never reaches update() at all. The container's top edge is then claimed only at touch-down and never released after scrolling away, so its bottom edge keeps overscroll-behavior-y: none (architecture:react-component-runtime INV6). Please move the listener wherever owner.current changes (inside resolveOwner, with the Effect's cleanup removing the last one), re-run this setup when contentRef attaches a node, and add a test for content that mounts after the hook.
| // - A pull triggers once, with the finger down, when its overscroll reaches | ||
| // the trigger distance. Only overscroll that a finger drives triggers, never | ||
| // a momentum overshoot. | ||
| // - A pull while refreshing only stretches: one refresh runs at a time. |
There was a problem hiding this comment.
Non-blocking: the header calls these rules "as UIKit runs them", but no UIKit capture backs this one, and the previous version said UIKit fires valueChanged again. Suggested: keep "One refresh runs at a time: a pull while refreshing only stretches." and drop the UIKit attribution for this rule.
e60e757 to
31ca01f
Compare
There was a problem hiding this comment.
Needs an engineering owner's decision, then one change.
#9807: approved pending CI. Core's internal touch entry re-exports exactly the names Lab's hook imports, inside the internal-subpath exception; CI has not run on this draft, so once it is ready for review a maintainer adds the ci:run label so CI runs.
#9865: needs an engineering owner's decision and a change. No current spec record governs useRefreshControl and the story sits outside packages/lab, so an engineering owner records the decision for this head by commenting /approve-spec <full head SHA> (the author may, as one) or provides the governing record; the earlier approval named the previous head. The scroll-owner listener fix and the stack declaration clear the two earlier findings.
- Miss: the indicator's fade in
startSettle(refreshTouchController.ts:447-472) re-solves the settle spring and differentiates it locally, which Core'smapProgressinpackages/core/src/motion/progress.tsalready does for companion motion, so please re-export it through the internal touch entry and build the fade from the settle'scurve('y')with it.
Stack: the two pull requests fit together, and a pull request lands only when it and every pull request below it are approved.
[Automated review]
| if (to === 'top' && !model.current.isRefreshing && from > 0) { | ||
| // The indicator fades as the content covers it: started in the same | ||
| // task, the two start on the same compositor frame. | ||
| const curve = solveSpring(toSpring(scroll.bounce), {from, to: 0}); |
There was a problem hiding this comment.
Miss: this fade re-solves scroll.bounce with the animator's own 0.05 px threshold and differentiates it locally (derivative), which is what Core's mapProgress (packages/core/src/motion/progress.ts) does for companion motion; MotionHandle.curve says to map the settle's curve with it and play that on the same clock. Please re-export mapProgress through the internal touch entry and build the fade as mapProgress(settle.curve('y'), y => revealProgress(y, dimension) / start) from the content's settle handle, then drop this local companion.
| const refreshToken = useRef(0); | ||
| const isControlled = isRefreshingProp !== undefined; | ||
| const latest = useRef({onRefresh, isControlled, announce, announcer}); | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
Non-blocking: this layout Effect has no note on what it syncs. Suggested: "Syncs the handlers' ref with the newest options."
31ca01f to
680bf0d
Compare
|
/approve-spec 680bf0d |
There was a problem hiding this comment.
Needs an engineering owner's decision, then one change.
#9807: approved pending CI. Core's internal touch entry re-exports exactly the names Lab's hook imports; once the draft is ready for review, a maintainer adds the ci:run label so CI runs.
#9865: needs an engineering owner's decision, then the change below. No current spec record governs useRefreshControl and its story sits outside packages/lab, so an engineering owner records the decision for this head by commenting /approve-spec <full head SHA> (the author may, as an engineering owner) or provides the record that governs the pull, its inputs, the container's top-edge claim, the announcement and the end.
- Miss: a finger that catches the settle back to the top leaves the indicator's opacity fade running, because
stopPlacingandonTouchDowninrefreshTouchController.tsstop onlyy, so the indicator fades out while it is pulled deeper and pops back when the fade ends; inonTouchDown, multiply the reveal by the indicator's presented opacity and callrestoreIndicator(), so the pull and a resumed settle start from what is on screen, and add a test that catches a settle and pulls on.
Stack: the two pull requests fit together, and a pull request lands only when it and every pull request below it are approved.
[Automated review]
| pan.onTouchDown = () => { | ||
| resolveOwner(); | ||
| applyEdge(); | ||
| cancelPendingEnd(); | ||
| stopPlacing(); | ||
| }; |
There was a problem hiding this comment.
Miss: a finger landing on a settle back to the top stops only the y channels here, so the indicator's opacity fade from startSettle keeps running under the next pull. In Chromium on the story, caught at 54 px and pulled on to 84 px, the indicator's visible amount fell from 0.32 to 0; the finished fade then wrote reveal 0 under the still finger, and the next move popped it to 0.67. A tap that catches it restarts the fade at opacity 1 over the old reveal, so it flashes. Suggested: end the fade here and keep what it shows.
const shown = Number(indicatorElement.current?.style.getPropertyValue(REFRESH_REVEAL_VAR) || 0);
setReveal(shown * indicator.animator.get('opacity'));
restoreIndicator();with a test that catches a settle back to the top and pulls on.
680bf0d to
9b255ea
Compare
|
/approve-spec 9b255ea |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
- #9807: approved pending CI. Core's internal touch entry re-exports exactly the names Lab's hook imports, with no change to the public entry.
- #9865: approved pending CI. An engineering owner recorded the decision on
useRefreshControlfor this head, and the fix for a finger catching the settle keeps the indicator in view, with a test. - Stack: the two fit together and merge cleanly onto
main; a pull request lands only when it and every pull request below it are approved, and once the drafts are ready for review, a maintainer adds theci:runlabel so CI runs.
[Automated review]
cixzhang
added
the
ci:run
… reads Core's internal touch entry re-exports, by name, what Lab's useRefreshControl imports: the pan recognizer, its hook and the gesture types; the motion animator; the cubic Bézier, the rubber band and its inverse, and the map of a companion curve; the UIKit scroll table; and the motion-role parsers. Each one is an existing internal, and Core's behavior does not change. A bundle test checks that the public entry exports none of them.
9b255ea to
705f4cb
Compare
There was a problem hiding this comment.
Needs an engineering owner's decision on #9865; #9807 is approved pending CI.
#9807
Approved pending CI: CI has not run on this head yet. Core's internal touch entry re-exports exactly the names Lab's hook reads, with no change to Core's public entry.
#9865
No current spec record governs useRefreshControl, and the last /approve-spec named the previous head, so approval needs an engineering owner to record the decision for this rebased head by commenting /approve-spec <full head SHA> (the author may, as an engineering owner) or to provide the governing record, covering the pull and its trigger, which inputs pull, the scroll container's top-edge claim, the announcement and the end.
- Miss: a release settles from rest whatever the finger's speed (
refreshTouchController.ts:454-463), wherespec:AST-094FR45 asks a gesture's settle to start at the release velocity; the decision should say whether that stands.
Stack
#9807 exports exactly what #9865 imports, and a pull request lands only when it and every pull request below it are approved; both drafts come from a fork, so once they are ready for review, a maintainer adds the ci:run label so CI runs.
[Automated review]
| const touchRelease = pullPointer.current === 'touch'; | ||
| const contentSettle = content.animator.animate( | ||
| {y: target}, | ||
| {spring: scroll.bounce, velocity: {y: 0}}, |
There was a problem hiding this comment.
Miss: every release settles from rest here (velocity: {y: 0}, and again at line 461), whatever the finger's speed. spec:AST-094 FR45 asks a gesture's settle to start at the release velocity, while its Non-goals keep scroll physics native and OQ1 leaves the scroll view's bounce-back open, so the engineering owner's decision for this head should say whether this stands. If it should carry the speed, the pan's release velocity would go here, as Core's other scroll.bounce users hand theirs.
…roll container Lab's useRefreshControl gives an app's own scroll container UIKit's pull to refresh. The pull follows UIKit's measured mapping and triggers with the finger down. A release settles onto the refreshing inset on the scroll view's bounce. The end slides away on the theme's duration and move easing tokens. The app draws the indicator. The hook pins it at the top of the scroll view and writes how much of it the pull reveals. The app also supplies the announcement text. A finger and a trackpad's two-finger scroll pull. A mouse or pen drag stays the browser's, for text selection. The pull applies in the nearest scroll container that overflows, never on the page itself.
A Storybook story shows useRefreshControl on a plain scroll container, with Core's Spinner as the app's indicator. The interaction spec opens it, and the story can hold a refresh until a test ends it.
705f4cb to
dee16ac
Compare
|
/approve-spec dee16ac |
There was a problem hiding this comment.
Needs an engineering owner's decision on #9865, then two changes; #9807 is approved pending CI.
#9807
Approved pending CI: CI has not run on this head yet. Core's internal touch entry re-exports exactly the names Lab's hook reads, with no change to Core's public entry.
#9865
No current spec record governs useRefreshControl, and the last /approve-spec named an earlier head. Approval needs an engineering owner to record the decision for this head by commenting /approve-spec <full head SHA>, which the author may do as an engineering owner, or to provide the governing record. The decision covers the pull and its trigger, which inputs pull, the scroll container's top-edge claim, the announcement, the end, and the release settle, which now starts at the finger's speed.
Required changes:
- The release hands the settle
pan.velocity()(refreshTouchController.ts:724), but the pull is the scroll view's own drag, andspec:AST-100FR2 gives a scrolling surface the scroll view's release velocity, so readpan.scrollVelocity()as the pager and the picker wheel do, and add an accelerating stream to the release test. pullSlope(refreshPull.ts:70-93) re-derives by finite difference the slope Core already exports asrubberBandSlope, so re-export that through the internal touch entry and call it.
Stack
#9807 exports exactly what #9865 imports, and the rubberBandSlope fix adds one name. A pull request lands only when it and every pull request below it are approved. Both drafts come from a fork, so once they are ready for review, a maintainer adds the ci:run label so CI runs.
[Automated review]
| // A lift hands the content's speed to the settle. A cancelled pull's | ||
| // finger never lifted, so its settle starts from rest. | ||
| releaseVelocity.current = | ||
| pan.state === 'ended' ? pan.velocity().y * fingerSlope.current : 0; |
There was a problem hiding this comment.
This hands the settle the pan's velocity, the dragged-surface filter. The pull is the scroll view's own drag and settles on the scroll view's bounce, so spec:AST-100 FR2 gives it the scroll view's release velocity, which Core reads as scrollVelocity() (the pager and the picker wheel use it). On an accelerating flick (10, 10, 10, 20, 30 px per 16 ms), velocity() reads 1,375 px/s and scrollVelocity() reads 906 px/s.
pan.state === 'ended' ? pan.scrollVelocity().y * fingerSlope.current : 0;| /** The step of the finite difference taken on a travel, px. */ | ||
| const SLOPE_STEP = 0.01; | ||
|
|
||
| /** | ||
| * How fast the overscroll moves per px of finger travel at `travel`: 1 where | ||
| * it tracks the finger, less in the rubber band. A release's speed times | ||
| * this slope is the content's speed, which its settle starts at. | ||
| */ | ||
| export function pullSlope( | ||
| travel: number, | ||
| dimension: number, | ||
| mode: PullMode, | ||
| ): number { | ||
| if (!(travel > 0)) { | ||
| return 0; | ||
| } | ||
| const before = Math.max(0, travel - SLOPE_STEP); | ||
| const after = travel + SLOPE_STEP; | ||
| return ( | ||
| (pullOverscroll(after, dimension, mode) - | ||
| pullOverscroll(before, dimension, mode)) / | ||
| (after - before) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Core already exports this slope as rubberBandSlope (motion/rubberBand.ts), described there as "what a finger velocity becomes on screen while banding". Re-export it through the internal touch entry and call it, so the slope cannot drift from Core's rubber band:
const linear = linearStretch(mode);
return travel <= linear ? 1 : rubberBandSlope(travel - linear, dimension);
Lab demo: pull to refresh as a hook. Supersedes #8170, #8171, #8175, #8251, #8688, #8694, #8697.
Stack: #9807 → #9865
Review only this PR: vjeux/astryx@bbd153f...dee16ac
About 4,900 added lines, two thirds tests.
User need
Phone apps need UIKit's pull to refresh; a browser's own pull reloads the page.
Problem and solution fit
useRefreshControlgives an app's scroll container UIKit's pull. The app keeps its markup and draws the indicator (Core's Spinner here). The pull follows UIKit's measured mapping, triggers with the finger down and settles on the bounce.Current specification
spec:AST-094FR1–FR2).Public delta and caller burden
New Lab exports
useRefreshControl, its types andREFRESH_REVEAL_VAR. The hook returns refs for the content and indicator slot,isRefreshingandrefresh():The app supplies the announcement, the indicator and another way to refresh: a pull has no keyboard equivalent.
Design and alternatives
--duration-medium(300 ms in neutral) on--ease-move.Evidence
A person checks the pull on a phone, on the preview.
Scope
Testing
Gates at this head, on main 9a4627b: typecheck, lint and unit tests clean. The interaction spec passes in Chromium (23) and WebKit ×3 (54).
The busy-end proof checks the end's start is not dated before the busy block. Under load (WebKit, 4 workers on 4 CPUs) it passed 40/40 where the old bound failed 5/40. Not-for-landing red-firsts: b2c6335 (the end played early) fails it 3/10. The new unit tests fail on the earlier code at 0109ea1, 1b34a90, a497571, dace28d and bde0539.