Repository navigation
feat(core): the internal touch foundation: press tracking and the animator - #8076
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
1efff6c to
c929a3b
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs a maintainer decision before it can be approved: the two module-level experimental subpaths are not a boundary spec:AST-017 FR21 states today (it names the owning component's /experimental subpath), and what they expose is specified only by the draft spec:AST-094. Approval needs a maintainer to provide or identify the governing spec for module-level experimental subpaths and to accept or reject AST-094's draft requirements; an engineering owner can record that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever that decision, these still need changes:
- Land the stacked pull requests first or retarget this one, so its diff is only the subpath commit and the stable adaptive-breakpoint change is reviewed in its own pull request.
- The four experimental hook docs produce docsite pages and ShadCN items that import from
@astryxdesign/coreat unmarked routes, which fails the docsite, registry and CLI search checks. useGestureRecognizerdeliversonGesturefrom a layout effect whenisEnabledturns off,usePressTrackercallsonHighlightChangefrom its cleanup, and both lose their element when it is replaced behind the same ref.
[Automated review]
| }, [ref, recognizer, manageStyle]); | ||
|
|
||
| useIsomorphicLayoutEffect(() => { | ||
| recognizer.setEnabled(isEnabled); |
There was a problem hiding this comment.
Turning isEnabled off mid-gesture cancels it here, so the caller's onGesture receives cancelled from inside this layout effect (the hook's own test records ['began', 'cancelled'] on that rerender). A consumer callback should fire from the change that caused it, not from an effect reacting to it; either cancel where isEnabled flips or document this as the external sync and stop listener delivery for a disable.
| ); | ||
|
|
||
| useIsomorphicLayoutEffect(() => { | ||
| const element = ref.current; |
There was a problem hiding this comment.
ref.current is read once, in an effect keyed on the ref object, so replacing the element behind the same ref ({open ? <div ref={ref}/> : <span ref={ref}/>}) leaves the recognizer on the detached node; a pan on the new element yields no states at all. A callback ref, or tracking the node and re-attaching when it changes, would follow the replacement. usePressTracker below has the same shape.
| ); | ||
| return () => { | ||
| detach(); | ||
| tracker.dispose(); |
There was a problem hiding this comment.
dispose() calls setHighlighted(false), which calls the caller's onHighlightChange(false) when the control unmounts while highlighted, so a consumer callback runs from effect cleanup. Making dispose() silent on unmount avoids a state update into an unmounting tree.
| ], | ||
| }, | ||
| relatedHooks: ['usePressTracker', 'useTouchPresentation'], | ||
| importPath: '@astryxdesign/core/gestures/experimental', |
There was a problem hiding this comment.
The docsite generator ignores this importPath: it resolves the hook to @astryxdesign/core, because only a ./gestures export would match, and publishes hooks/use-gesture-recognizer with a ShadCN item that writes export * from '@astryxdesign/core';. With the other three hook docs, this adds four routes missing from the ShadCN route lock (docsite-data, docsite-browser and registry-contract fail) and changes the astryx search experimental goldens. The generator needs to honor this path, and the hooks need marked experimental routes or none, before the lock is refreshed.
| A gesture recognizer system with UIKit's semantics, for the touch | ||
| presentations of Astryx core components. It is internal to | ||
| `@astryxdesign/core`: components import it from `../gestures`, and nothing | ||
| here is exported from the package entry. |
There was a problem hiding this comment.
This now contradicts gestures/experimental.ts, which exports the system from @astryxdesign/core/gestures/experimental and asks for this README to be updated with it; gestures/index.ts says the same in its header.
| where a switch outside any scroll view is needed. | ||
|
|
||
| Done (iOS 26.2 simulator, Oct 23 2026; in the switch's own table, | ||
| `uikit/switch.ts`): M-SW02, M-SW03, M-SW04, M-SW05, M-SW06, M-SW09 (UISwitch |
There was a problem hiding this comment.
uikit/switch.ts does not exist at this head, and neither does uikit/contextMenu.ts, cited at line 254. Either point at where those values live or drop the references until those tables land.
| // and large touch displays stay anchored, phones in either orientation get | ||
| // the sheet. The query is the shared touch presentation's | ||
| // (utils/touchPresentation.ts). | ||
| export const COMPACT_TOUCH_PRESENTATION_QUERY = COMPACT_TOUCH_QUERY; |
There was a problem hiding this comment.
This moves the stable adaptive boundary for five menus and for DropdownMenu sub-menus, whose default is adaptive (useMenuDrillIn.tsx reads this constant). A coarse-pointer viewport 744 to 768px wide (an iPad mini in portrait) now gets the popover or flyout instead of the sheet or drill-in, and a landscape phone wider than 768px gets the reverse. That contradicts the description's "Stable callers see no change", and it belongs in its own pull request with its own decision.
PR Analysis Report
The exact-head preview is not available yet. Generated by PR Enrichment workflow | View current CI run |
5d15897 to
9159640
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: spec:AST-017 FR21 places experimental exports on a component's /experimental subpath, so the two module-level subpaths need a governing spec, and what they expose rests on the draft spec:AST-094 (FR1–FR23, DEC-1, DEC-2). An engineering owner can record that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or can name the record that should state module-level experimental subpaths. Whatever that decision, these still need changes:
- Land the stacked pull requests first or retarget this one, so its diff is only the subpath commit and the stable
adaptiveboundary change is decided in #8062. - The four experimental hooks' docsite pages and ShadCN items resolve to
@astryxdesign/core, which does not export them, at unmarked routes, and the refreshed route lock would publish them that way. useGestureRecognizerandusePressTrackercallonGestureandonHighlightChangefrom effects, and keep their recognizer on the old node when the element behind the ref is replaced.
[Automated review]
| "aliases": [] | ||
| }, | ||
| { | ||
| "name": "hook-use-animator", |
There was a problem hiding this comment.
Locking these four hook routes publishes the pages and ShadCN items as the generator builds them today: resolveImportPathForPkg (apps/docsite/scripts/generate-data.mjs:366–375) gives them @astryxdesign/core, which exports none of the four, so the item installs only export * from '@astryxdesign/core';. spec:AST-017 FR27 also asks for marked experimental routes. The generator needs to honor the docs' importPath (or map <dir>/experimental exports) before these routes are locked.
9159640 to
a0acff9
Compare
a0acff9 to
38007d4
Compare
There was a problem hiding this comment.
Changes are needed before this can be approved: the diff still carries its open dependency stack (14,048 authored lines against about 670 lines of its own boundary guards), and the gesture host it folds in contradicts current spec:AST-094. Please land the stack or retarget onto its top so this diff carries only the boundary work, and fix the points below in the pull requests that own them.
GestureHostlistens only under a coarse primary pointer, while AST-094 FR19 and FR43 andarchitecture:react-component-runtimeINV13 attach gestures for every pointer; it also lets onescrolltake a finger that has not moved (FR20) and writestouch-actionover a narrower author value (FR18).- Under a fine primary pointer, a finger press on an action group never ends its touch and breaks the group's next press (
family:action-groupsFR5). useGestureRecognizercallsonGesturefrom an Effect when its element unmounts or is replaced mid-gesture (INV2), and recognizers are told apart byinstanceofwhilePressTrackeris a class (INV13).utils/pressFeedback.tsand.changeset/gesture-press-tracking.mdstill callwritePressthe press attribute's only writer, and the gestures README and barrel still describe a Lab import route the new guard forbids.
[Automated review]
| } | ||
|
|
||
| private shouldListen(): boolean { | ||
| if (!this.isPrimaryPointerCoarse()) { |
There was a problem hiding this comment.
Under a fine primary pointer this returns before looking at any registration, so a mouse, trackpad or pen never reaches a pan, swipe or long press. spec:AST-094 FR19 and FR43 and architecture:react-component-runtime INV13 now attach gestures for every pointer, listening on the element under a fine pointer and adding document listeners only during a press (FR24).
| element.style.getPropertyValue(property), | ||
| ); | ||
| } | ||
| element.style.setProperty(property, value); |
There was a problem hiding this comment.
This writes the derived value over whatever the element had; nothing reads the author's computed touch-action. A tap recognizer on an element styled touch-action: none ends up with manipulation. spec:AST-094 FR18 (DEC-8) requires the derived value to be intersected with the author's and never wider than it.
| ) { | ||
| continue; | ||
| } | ||
| if (touch.target instanceof Node && scroller.contains(touch.target)) { |
There was a problem hiding this comment.
Any scroll above an unclaimed finger hands it to the browser here, with no travel check, so a resting finger's tap fails and its touchend is cancelled. spec:AST-094 FR20 counts a native scroll only once that finger has moved the 10 px scroll take distance (DEC-8).
| if ( | ||
| this.isInstalled || | ||
| this.groupPressPointer != null || | ||
| !isPointerKind(event.pointerType) || |
There was a problem hiding this comment.
isPointerKind accepts touch, but this group path runs only while the host is not installed, so usesTouchEvents is still its initial true and onPointerUp's touch branch never ends the finger's arena touch. Under a fine primary pointer one finger press leaves a live touch and the group's next mouse press gives no tap (family:action-groups FR5 covers a finger as well as a mouse and pen).
| ) { | ||
| return; | ||
| } | ||
| current?.detach?.(); |
There was a problem hiding this comment.
This detach (and the unmount one at line 254) cancels a recognizer mid-gesture inside a layout Effect while the listener added at line 75 is still subscribed and unguarded, so onGesture hears cancelled from the Effect. architecture:react-component-runtime INV2 and the comment at line 63 rule that out; a guard like usePressTracker's isReleasingRef would keep it out.
| ); | ||
| } | ||
|
|
||
| export class PressTracker implements TouchConsumer { |
There was a problem hiding this comment.
architecture:react-component-runtime INV13 keeps classes to the recognizer family (base, arena, host) and builds every other module from create… factories, and it tells recognizers apart by kind, never by instanceof (line 150 here, and GestureHost.ts lines 160, 344 and 361, touchAction.ts line 118).
| } | ||
|
|
||
| /** | ||
| * Write `next` on `element` if its state is one of `onlyFrom`: THE ONE |
There was a problem hiding this comment.
hooks/actionGroupTracker.ts still sets and removes data-astryx-press directly (lines 252, 259, 266, 272 and 442), as the gestures README now says, so this is not the one writer for every painter; the same claim is in .changeset/gesture-press-tracking.md.
|
Thanks.
|
There was a problem hiding this comment.
Changes are needed before this can be approved: the diff still carries the open #8061 and #8071 commits plus a 7,765-line commit of its own (13,299 authored lines in all), Core's type check fails on this head, and the gesture host contradicts current spec:AST-094. Please land the stack and #8548 or retarget onto them, split the internal entries from the foundation, and fix the points below in the pull requests that own them.
GestureHostlistens only under a coarse primary pointer (FR19, FR43,architecture:react-component-runtimeINV13) and writes itstouch-actionover a narrower author value (FR18).ScreenEdgePanGestureRecognizeraccepts a pen or a mouse, which FR43 keeps off.useGestureRecognizercallsonGesturefrom a layout-effect cleanup when its element unmounts or is replaced mid-gesture (INV2), andPressTrackeris a class told apart withinstanceofchecks (INV13).animator.interaction.browser.spec.tsimportscountBetweenandtraceRendering, which the harness does not export until #8548 lands, and the press-writer comments, the changeset and the gestures README misdescribe this head.
[Automated review]
| private shouldListen(): boolean { | ||
| if (!this.isPrimaryPointerCoarse()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Under a fine primary pointer the host installs nothing, so a mouse, trackpad or pen never reaches a pan, swipe or long press. Current spec:AST-094 FR19 and FR43 and architecture:react-component-runtime INV13 attach gestures for every pointer (listening only on elements that use one, with document listeners only during a press, FR24).
| const style: GestureHostStyle = this.isPrimaryPointerCoarse() | ||
| ? gestureHostStyle(registration.recognizers.filter(r => r.isEnabled)) | ||
| : {}; |
There was a problem hiding this comment.
The derived value is written over the element's own touch-action without reading it, so an author's none becomes manipulation. FR18 requires the result to be intersected with the author's value and never wider than it.
| super({ | ||
| maximumNumberOfTouches: 1, | ||
| hysteresis: UIKIT_SCREEN_EDGE_PAN.hysteresis.value, | ||
| ...options, | ||
| }); |
There was a problem hiding this comment.
No pointer restriction here, so on a coarse-primary tablet a pen or a mouse dragging from the edge band begins and ends this pan; FR43 keeps the screen-edge pan off under a mouse, a trackpad or a pen. Defaulting allowedPointerTypes to ['touch'] would match it.
| useIsomorphicLayoutEffect( | ||
| () => () => { | ||
| attachedRef.current?.detach?.(); | ||
| attachedRef.current = null; |
There was a problem hiding this comment.
This detach (and the replacement path at line 247) cancels a recognizer mid-gesture while the listener from line 75 is still subscribed, so onGesture hears cancelled from a layout-effect cleanup (INV2). Running it under a guard, as usePressTracker does with isReleasingRef, keeps the callback out of the Effect.
| countBetween, | ||
| markElement, | ||
| startScreencast, | ||
| traceRendering, |
There was a problem hiding this comment.
countBetween and traceRendering are not exported by @astryxdesign/interaction-harness on this head, so tsc for Core fails with TS2305 twice until #8548 lands.
| ); | ||
| } | ||
|
|
||
| export class PressTracker implements TouchConsumer { |
There was a problem hiding this comment.
INV13 keeps classes to the recognizer family and builds every other module from create… factories; this class (and the instanceof check at line 150) contradicts it.
|
Thanks.
|
There was a problem hiding this comment.
Changes are needed before this can be approved: the diff still carries the open #8061 and #8071 commits plus a 7,549-line commit of its own (13,894 authored lines in all), Core's type check fails on this head, and the gesture host contradicts current spec:AST-094. Please land #8061, #8071 and #8548 first or retarget onto them, split the internal entries from the foundation, and fix the points below.
GestureHostinstalls no listener where the primary pointer is fine, so a mouse, trackpad or pen gets no gesture (FR19, FR43,architecture:react-component-runtimeINV13).animator.interaction.browser.spec.tsimportscountBetweenandtraceRendering, which the interaction harness does not export until #8548 lands (TS2305).usePressTracker's default scroll brake also drops a mouse or pen press after a finger-driven scroll, where FR11 keeps the brake touch-only.gestures/index.tsstill says Lab imports the barrel through internal workspace imports, theresolveMotioncomments cite DEC-11 where DEC-15 now applies, andinternal/touchre-exports the iOS style gate that no Lab file uses.
[Automated review]
| private shouldListen(): boolean { | ||
| if (!this.isPrimaryPointerCoarse()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Under a fine primary pointer this keeps the whole host off, so a mouse, trackpad or pen never reaches a recognizer (a 96 px mouse drag gives a pan no transition). Current spec:AST-094 FR19 ("Recognizers attach for every pointer, with FR43's exceptions"), FR43 and architecture:react-component-runtime INV13 ("no touch-only query") attach gestures for every pointer, with FR24's budget: listen only on an element that uses a gesture, and add document listeners only during a press.
| countBetween, | ||
| markElement, | ||
| startScreencast, | ||
| traceRendering, |
There was a problem hiding this comment.
countBetween and traceRendering are not exported by @astryxdesign/interaction-harness on this head (they arrive with #8548), so tsc --project packages/core/tsconfig.json --noEmit fails with TS2305 twice and Core's typecheck fails. Landing #8548 first, or leaving the trace case out until it lands, clears it.
| if ( | ||
| touchId != null || | ||
| options.isEnabled?.() === false || | ||
| options.isBraking?.(touch) === true |
There was a problem hiding this comment.
This brake applies to every pointer: with usePressTracker's default (GestureHost.isScrollBraking), a mouse or pen press within 150 ms of a finger-driven scroll gets no touchDown, no highlight and no events. spec:AST-094 FR11 keeps the scroll brake touch-only (DEC-21); checking touch.pointerType === 'touch' before braking would conform.
| * @position Core's own barrel. Not exported from the package; Lab imports it | ||
| * through internal workspace imports. |
There was a problem hiding this comment.
Lab no longer reaches this barrel through internal workspace imports: scripts/lab-core-imports.test.mjs rejects any Lab import outside Core's exports, and the README now names @astryxdesign/core/internal/touch. Worth matching that here.
| IOS_SUPPORTS_QUERY, | ||
| IOS_TOUCH_STYLE_KEYS, |
There was a problem hiding this comment.
No Lab file imports the iOS style gate, and an engineering owner asked earlier on this pull request that the iOS idiom exports (IOS_SUPPORTS_QUERY, IOS_TOUCH_STYLE_KEYS) be dropped. Leaving these two off the entry keeps it to what Lab builds on.
|
Thanks.
|
There was a problem hiding this comment.
Changes are needed before this can be approved: the diff still carries the open #8061 and #8071 commits plus a 7,122-line commit of its own (13,725 authored lines), Core's type check fails on this head, and the gesture host contradicts current spec:AST-094. Please land #8061, #8071 and #8548 first or retarget onto them, split press tracking from the animator, and fix the points below.
GestureHostinstalls no listener where the primary pointer is fine, so a mouse, trackpad or pen gets no gesture and no fine-pointer press (FR19, FR43,architecture:react-component-runtimeINV13).animator.interaction.browser.spec.tsimportscountBetweenandtraceRendering, which the interaction harness does not export until #8548 lands (TS2305).- A screen-edge pan adds a
windowresizelistener even where the primary pointer is fine, where FR24 keeps a gesture's listeners on its own element. animator.tsstill cites DEC-11 where DEC-15 now applies,.changeset/reduced-motion-record.mddescribes helpers that already exist, andusePressTracker'sdefaultVetoreplaces the tracker'skind-based tap check.
[Automated review]
| } | ||
|
|
||
| private shouldListen(): boolean { | ||
| if (!this.isPrimaryPointerCoarse()) { |
There was a problem hiding this comment.
Under a fine primary pointer this keeps the whole host off, so a mouse, trackpad or pen never reaches a recognizer (a 96 px mouse drag gives a pan no transition), and usePressTracker never gets a mouse press for FR11's fine-pointer profile. Current spec:AST-094 FR19 ("Recognizers attach for every pointer, with FR43's exceptions"), FR43 and architecture:react-component-runtime INV13 ("The gesture host attaches for every pointer, with no touch-only query") require the opposite; under a fine pointer FR24 lets a component listen on its own element and add document listeners only during a press.
| countBetween, | ||
| markElement, | ||
| startScreencast, | ||
| traceRendering, |
There was a problem hiding this comment.
countBetween and traceRendering are not exported by @astryxdesign/interaction-harness on this head (they arrive with #8548), so tsc --project packages/core/tsconfig.json --noEmit fails with TS2305 twice and Core's typecheck fails. Landing #8548 first, or leaving the trace case out until it lands, fixes it.
| } | ||
| const view = this.doc.defaultView; | ||
| if (this.layoutWatched.size === 0 && view != null) { | ||
| view.addEventListener('resize', this.onLayoutChange, PASSIVE_CAPTURE); |
There was a problem hiding this comment.
This window listener (and the observer below) is added as soon as a screen-edge pan attaches, under every pointer, so on a fine-pointer page it is the one idle gesture listener while the host otherwise listens for nothing. spec:AST-094 FR24: "Under a fine pointer a component that uses a gesture listens only on its own element and adds document listeners only during a press". Adding it only while the primary pointer is coarse, and re-measuring on the pointer-kind change the host already watches, would match.
| * resolveMotion.ts, which names the motion role it plays (`springs.settle` | ||
| * once `registerSpringsTable()` has run, the Reduce Motion fade's curve, | ||
| * which the animator registers alone). While touch motion stays internal | ||
| * (`spec:AST-094` DEC-11), a role plays its table value: nothing reads a CSS |
There was a problem hiding this comment.
DEC-15 superseded DEC-11's "all internal"; resolveMotion.ts and the motion README now say "the per-role tables stay internal (DEC-15)". This line and the comment at line 2348 still cite DEC-11.
| } | ||
|
|
||
| /** PressTracker's own default: an ancestor's single-finger single tap. */ | ||
| function defaultVeto(recognizer: GestureRecognizer): boolean { |
There was a problem hiding this comment.
The hook always passes vetoesAncestorRecognizer, so this shape check replaces the tracker's own kind-based vetoesSingleTap on every hook-built tracker, though the comment calls it the tracker's default. INV13 tells a recognizer apart by its kind field; letting the tracker's default apply when the caller gives no veto keeps one rule.
|
Thanks.
|
There was a problem hiding this comment.
Changes are needed before this can be approved: the gesture host still listens for nothing where the primary pointer is fine, which contradicts current spec:AST-094 FR19 and FR43 and architecture:react-component-runtime INV13, and the diff still carries the open #8061 and #8071 commits plus a 7,123-line commit of its own. Please conform GestureHost to FR43 (under a fine pointer, listen only on elements that use a gesture and add document listeners only during a press, per FR24), or keep the host out of this pull request until #8665 lands. Then land #8061 and #8071 first or retarget onto them, and split press tracking from the animator so each piece is reviewed on its own.
[Automated review]
| private shouldListen(): boolean { | ||
| if (!this.isPrimaryPointerCoarse()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Under a fine primary pointer this returns before any document listener is installed, so a mouse, trackpad or pen never reaches a recognizer or usePressTracker outside an action group. Current spec:AST-094 FR43 says gestures attach "for every pointer, a mouse, a pen, and a finger, not only a coarse primary pointer", and architecture:react-component-runtime INV13 says the host attaches "with no touch-only query". Under a fine pointer FR24 lets a component that uses a gesture listen on its own element and add document listeners only during a press, which is what the action-group path already does.
|
A recognizer that waits on one being disabled no longer calls its listener from the Effect: one count of Effect-driven passes keeps every gesture hook quiet (INV2; new test). |
There was a problem hiding this comment.
Changes are needed before this can be approved: the gesture host still listens for nothing where the primary pointer is fine, which contradicts current spec:AST-094 FR19 and FR43 and architecture:react-component-runtime INV13, and the diff still carries the open #8061 and #8071 commits plus a 7,243-line commit of its own. Please conform GestureHost to FR43 (under a fine pointer, listen only on elements that use a gesture and add document listeners only during a press, per FR24), or keep the host out of this pull request until #8665 lands. Then land #8061 and #8071 first or retarget onto them, split press tracking from the animator, and add the real-touch browser cases FR18–FR22 ask for (a narrower author touch-action, the 9 and 10 px take distance, the screen-edge band) where CI runs them.
[Automated review]
| if (!this.isPrimaryPointerCoarse()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Under a fine primary pointer this returns before anything is registered, so a mouse, trackpad or pen never reaches a recognizer outside an action group. Current spec:AST-094 FR19 and FR43 attach recognizers for every pointer, and architecture:react-component-runtime INV13 says the host has no touch-only query; FR24 lets a component that uses a gesture listen on its own element under a fine pointer and add document listeners only during a press.
| it('registers no document listener where the primary pointer is fine, or while nothing enabled is attached (spec:AST-094 FR19, FR24)', () => { | ||
| const {box} = mount('<div id="box"></div>'); | ||
| const added = vi.spyOn(document, 'addEventListener'); | ||
| const removed = vi.spyOn(document, 'removeEventListener'); | ||
| added.mockClear(); | ||
| removed.mockClear(); | ||
| const types = (spy: typeof added) => | ||
| spy.mock.calls | ||
| .map(call => call[0]) | ||
| .filter(t => /^(touch|pointer)/.test(t)); | ||
| // A fine primary pointer: nothing is registered, an element's touchmove | ||
| // listener included. | ||
| const fine = new GestureHost(document, { | ||
| clock, | ||
| isPrimaryPointerCoarse: () => false, | ||
| }); | ||
| const boxAdded = vi.spyOn(box, 'addEventListener'); | ||
| const detachFine = fine.attach(box, new PanGestureRecognizer()); | ||
| expect(types(added)).toEqual([]); | ||
| expect(boxAdded).not.toHaveBeenCalled(); | ||
| detachFine(); | ||
| // A coarse one, with only a disabled recognizer: still nothing. | ||
| const coarse = new GestureHost(document, { | ||
| clock, | ||
| isPrimaryPointerCoarse: () => true, | ||
| }); | ||
| const pan = new PanGestureRecognizer({isEnabled: false}); | ||
| const detach = coarse.attach(box, pan); | ||
| expect(types(added)).toEqual([]); | ||
| expect(boxAdded).not.toHaveBeenCalled(); | ||
| // Enabling it registers the listeners; disabling removes them. | ||
| pan.setEnabled(true); | ||
| expect(types(added)).toContain('touchstart'); | ||
| expect(boxAdded).toHaveBeenCalledWith('touchmove', expect.any(Function), { | ||
| passive: false, | ||
| }); | ||
| pan.setEnabled(false); | ||
| expect(types(removed)).toContain('touchstart'); | ||
| detach(); | ||
| }); |
There was a problem hiding this comment.
This case pins the touch-only host that FR19 and FR43 reject. Under FR24 it would become the fine-pointer resource count: one listener on the element that uses a gesture, and document listeners only during a press.
|
The gesture browser proof now runs in CI's interaction proofs ( |
There was a problem hiding this comment.
Changes are needed before this can be approved: the gesture host now serves a mouse, a pen and a finger as spec:AST-094 FR19 and FR43 require, but the diff still carries the open #8061 and #8071 commits plus a 7,682-line commit of its own (14,669 authored lines in all), and FR20's 10 px scroll take still has no real-touch browser case. Please land #8061 and #8071 first or retarget onto them, move the gesture browser proof to the host's commit, and split press tracking from the animator. Then add a case to the gesture browser proof where a native scroll takes an unclaimed press: a still finger with sub-pixel jitter, 9 and 10 px straight, and (7, 7) and (8, 8) diagonally, as FR18–FR22's verification lists.
[Automated review]
| } | ||
| }); | ||
|
|
||
| test('measures a pan’s travel per axis: a free pan stays possible at (7, 7) and (8, 8) and begins at (10, 10) (FR19)', async ({ |
There was a problem hiding this comment.
This proves the pan's per-axis claim (FR17, FR19): at (8, 8) the pan stays possible. FR18–FR22's verification also lists (7, 7) and (8, 8), with a still finger and 9 and 10 px straight, for FR20's straight-line scroll take, where a native scroll takes an unclaimed press once the finger has moved 10 px (the host's onScroll). That take is proven only in jsdom with a synthetic scroll event (GestureHost.test.ts:1189-1213). A press inside #scroller dragged through those states, surviving under 10 px and cancelled at 10, would close it.
|
The gesture browser proof moved to #8071, the host's change, with a real-touch case for a scroll's 10 px take of a held press: a still finger, 9 px and (7, 7) keep it; 10 px and (8, 8) lose it. |
There was a problem hiding this comment.
This looks ready from the review's side: every change matches the current touch-presentation and runtime specs, nothing new is exported, and the unit suites, type check, lint and Chromium browser proofs pass, including the new real-touch case for the 10 px scroll take. The size falls under the behavior-module exemption, and merging still needs the required checks, which were skipped on this draft. One optional addition: a regression case for keyboard activation passing the click swallower (FR21's detail === 0 rule), which works today but has no test.
[Automated review]
There was a problem hiding this comment.
Changes requested, for test evidence only: nothing new is exported, every change matches the current touch-presentation and runtime specs, and the unit suites, type check, lint and the Chromium and WebKit interaction proofs pass here. spec:AST-094 FR20 was rewritten on main (only a finger's first uncancelable move, judged on its main axis, can hand the touch to the browser), and its new verification row asks for Chromium touch cases of a vertical pan-y drag over a scroller with room and of one where no scroller has room; gestures.interaction.browser.spec.ts covers only the horizontal drag. Please add those two cases beside it; in Chromium both vertical drags also end in pointercancel, so the no-room case should show which signal ends the press. The size is covered by the behavior-module exemption.
[Automated review]
| expect(entries.some(entry => entry.name.startsWith('click:'))).toBe(false); | ||
| }); | ||
|
|
||
| test('a horizontal drag on a passive pan-y control keeps its press: uncancelable moves the browser cannot pan are not a claim', async ({ |
There was a problem hiding this comment.
Current spec:AST-094 FR20's verification row also lists "a vertical drag under pan-y over a scroller with room" and "a vertical drag where no scroller has room" for Chromium phone emulation. Two cases beside this one, on the same #passive press, would cover the row. In Chromium both vertical drags also end in pointercancel, which FR20 counts, so the no-room case should show which signal ends the press.
… the gesture host The browser negotiation and the gesture host on top of the recognizers: touch-action derived from what is attached and never wider than the element's own (spec:AST-094 FR18, DEC-8), a native scroll probe that follows a slotted node into its shadow tree, the tap haptic, and the host that claims a finger, suppresses the click a claimed touch would send, and hands a touch to the browser when it takes over. The host serves a mouse, a pen and a finger alike (FR19, FR43). Where the primary pointer is fine, an element that uses a gesture keeps one passive pointerdown listener and the page has none until a press, whose listeners go once its contacts end (FR24); a right click, a pen's barrel button and a ctrl-click on macOS start no press. A detach re-syncs the host even when the element keeps another attachment, so one left disabled keeps no listener or observer. It gives a screen-edge pan its view's box and, through one ResizeObserver and no listener, reports a resize of that box or of the viewport, so the pan's edge-zone answer and the touch-action derived from it follow the layout (FR18, FR19, FR24, DEC-22). Recognizers report their kind, which the host and the touch-action rules read instead of checking a class (architecture:react-component-runtime INV13). The gesture system's real-touch browser proof runs with it, under playwright.interaction.config.ts on a phone in Chromium and WebKit: touch-action per element, claims and cancelled moves, click suppression, pinch, a second finger, a long press under a blocked main thread, the 10 px take, the screen-edge band in both writing directions, a scroll's take of a held press only once the finger is 10 px from where it landed, and the browser taking a passive control's press by its scroll or, where nothing can scroll, its pointercancel (FR18-FR22); and, under a fine pointer, a mouse and a pen and the page listeners a press adds and removes (FR43, FR24). meta#8071's and meta#8072's changes, with meta#8665's serving of every pointer, rebased onto main after meta#8061 as one commit; the host's hover routing lands with the hover recognizer's first user. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
…mator Core's internal touch foundation on the gesture recognizers (meta#8061) and the gesture host (meta#8071 + meta#8072): UIControl press tracking with its press profiles and hooks, built by createPressTracker; and the animator, which starts a motion from rest at its commit, holds the value on screen inline while a curve runs, and lets a call wait for a presented frame. It absorbs meta#8073 (closed: the press tracking this rests on) and meta#8340 (the animator's start and hold fixes), with the held dip's load fixes, and keeps meta#8641's rule: a gesture that cancels itself while its finger moves keeps that finger until it lifts. A gesture that ends on an unmount or a replaced node calls no listener from the effect. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
|
The two vertical-drag cases FR20's verification asks for are in the gesture browser proof in #8071: over a page with room to scroll the browser takes the press, and where no scroller has room the browser's |
There was a problem hiding this comment.
Changes requested, for one test: every change matches the current touch-presentation, runtime and export specs, nothing new is exported, and the two vertical FR20 browser cases now pass in Chromium. The vertical branch of the first-uncancelable-move rule (GestureHost.ts:1259) can be replaced with true while every gesture unit test and Chromium case still passes, although a host changed that way cancels a press on a pan-x control under a vertical drag that this head keeps to the lift. Please add a case that fails in that situation, for example the horizontal pan-y case in GestureHost.test.ts turned 90°: a press under pan-x pinch-zoom in an overflow-x list, a vertical uncancelable touchmove, and the press not cancelled. The new no-room browser case cannot catch it, because Chromium sends pointercancel before any uncancelable move there.
[Automated review]
| return Math.abs(dx) >= Math.abs(dy) | ||
| ? touch.scroll.canScroll(dx, 0) | ||
| : touch.scroll.canScroll(0, dy); |
There was a problem hiding this comment.
Replacing touch.scroll.canScroll(0, dy) with true here keeps every gesture unit test and every Chromium case in gestures.interaction.browser.spec.ts green, yet in Chromium that host cancels a press on a pan-x pinch-zoom element under a vertical drag (uncancelable moves arrive with no pointercancel), where this head keeps the press to the lift. A jsdom case like keeps a horizontal drag under pan-y when Chromium stops asking after one unconsumed move, turned 90° (a pan-x row in an overflow-x list, vertical uncancelable moves, no cancelled), would make this branch fail when it breaks (spec:AST-094 FR20).
… stops asking (spec:AST-094 FR20) The jsdom case for a horizontal drag under pan-y, turned 90 degrees: a pan-x pinch-zoom row in an overflow-x list, dragged down, whose later moves arrive uncancelable. Its first uncancelable move is judged on its main axis, vertical, where pan-x lets the browser pan nothing, so the press is kept to the lift. A host that counts every vertical first uncancelable move as the browser taking the touch fails it; no other gesture unit test or browser case does.
|
Added the 90° turn of the horizontal |
There was a problem hiding this comment.
Looks ready from the review's side: the new pan-x case in GestureHost.test.ts fails when the vertical branch of FR20's first-uncancelable-move rule is replaced by true, which closes the last evidence gap. Every change matches the current touch-presentation, runtime and export specs, and nothing new is exported. What remains sits outside the review: the pull request is still a draft, so its required checks have not run, and #8071 is planned to land first. Separately, five doc comments in GestureHost.ts and GestureArena.ts sit above the wrong declarations, if you want to tidy them.
[Automated review]
There was a problem hiding this comment.
This looks ready from the review's side: press tracking, the gesture hooks and the animator's new start and hold modes match the current touch-presentation, runtime and export specs, nothing new is exported, and the unit suites, type check, lint and the Chromium animator proof pass at this head. Two doc comments landed out of place and are worth moving: GestureArena.ts:474 now sits above deferResolution instead of stop, and animator.ts:294-296 above SetOptions instead of AnimatorOptions. The pull request is still a draft, so its required checks have not run yet.
[Automated review]
| @@ -457,12 +472,42 @@ export class GestureArena { | |||
| } | |||
|
|
|||
| /** Cancel an active recognizer, or fail a possible one. */ | |||
There was a problem hiding this comment.
This is stop’s doc comment; deferResolution was inserted below it, so it now sits above deferResolution’s own doc and stop (line 507) has none. Moving it down to stop fixes both.
| @@ -231,6 +294,21 @@ export interface MotionCurve { | |||
| /** | |||
| * Options of `createAnimator`. | |||
There was a problem hiding this comment.
This doc belongs to AnimatorOptions (line 312); the new SetOptions was inserted between them, so it now reads as an orphan above SetOptions’ own doc.
Behavior-module PR: internal touch foundation, animator, press tracking
Stacked on #8071, one commit on
main, with no merge commits: review only this pull request's two commits, vjeux@0ddfa8937e and its test vjeux@91d49e1438.Why
Touch components need Core's animator, press tracking and gesture host without making them public.
What it absorbs, and why
One solution, the internal touch foundation, so these close with a link here:
Left out
defineConstsbuild fix and the stylesheet's undefined-variable check stay in fix(build): resolve defineConsts read across files in astryxStylex #8075.RotationGestureRecognizerandHoverGestureRecognizerare not exported: nothing uses them, and rotation's threshold is not measured.paintPressand its held state land with the action-group tracker,useAnimatorwith Lab's SearchField, and Core's internal entry points for Lab (chore(core): Lab reaches Core's pan recognizer, gesture types, touch presentation and motion through internal entries #8562's change) with the first Lab component that uses them: each with its first user.What changes
createPressTracker: the press clocks and profiles,usePressTracker, and release velocity on UIKit's filter (spec:AST-100FR1, FR2).start: 'presented'and'commit', additive reversals andhandle.curve. A browser proof counts no Layout or Paint while a spring plays, on the harness's rendering trace.How to check it
pnpm exec vitest run scripts/motion-node-load.test.mjs: the motion modules and recognizers load in a plain Node process.usePressTracker, changehighlightDelaywhile mounted: the next touch uses the new delay.pnpm test:interaction animator.interaction.browser.spec.tsin Chromium and WebKit: a motion started right before a blocked main thread plays from its start after the block.Evidence
pnpm test:ui(16,704), the node suites, lint, Storybook, and the motion and gesture interaction specs in Chromium (103) and WebKit (96); the held-dip, block and no-Layout-or-Paint proofs pass six times each at eight workers.Credit
Co-authored-by: Cindy Zhang (her commits in #8073 and #8076).
Scope