Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: against main, this pull request's diff includes all of #8665 (29,050 of its 29,164 authored lines), so it can't be reviewed or merged as the 642-line createPressTracker change it describes. Please land #8665 first and rebase this branch onto main, so the diff is the factory change alone. The factory itself follows architecture:react-component-runtime INV13 and keeps the tracker's behavior; while rebasing, please also fix the changeset and gestures README sentence that calls every touch module outside the recognizer family a factory, since VelocityTracker is still a class (INV13 lists it as a known gap).
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: press tracking is built by `createPressTracker(options)`, and `PressTracker` is the interface of what it returns, as every touch module outside the recognizer family is built from a factory. `usePressTracker` and its callers do not change. Not exported from the package entry point. |
There was a problem hiding this comment.
VelocityTracker is still a class outside the recognizer family (packages/core/src/gestures/VelocityTracker.ts:59), the gap architecture:react-component-runtime INV13 records, so this release note overstates the rule. Suggest "…as every touch module outside the recognizer family is, apart from VelocityTracker", or dropping the clause; the same sentence is in packages/core/src/gestures/README.md:180-181.
There was a problem hiding this comment.
Incremental review of d714d09..3e4d7c0; prior findings carried: spec_authority, authored_loc, context, user_impact, api, evidence_by_spec_claim, concrete_defects, disposition
Requesting changes: the changeset and gestures README now describe the factory rule accurately, naming VelocityTracker as the one class left outside the recognizer family, so that earlier note is resolved. Against main, the diff still includes all of #8665 (29,050 of its 29,165 authored lines), so it can't be reviewed or merged as the 643-line createPressTracker change it describes. Please land #8665 first and rebase this branch onto main, so the diff is the factory change alone; nothing else blocks the factory change.
[Automated review]
PR Analysis Report
The exact-head preview is not available yet. Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved, and the diff also needs to be split. Against main, it adds a sentence to spec:AST-094 FR41 that the current record does not state (“A mouse or pen drag that a recognizer claims suppresses text selection for that drag only; an unclaimed mouse drag keeps today's behavior.”); an engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately, 28,082 of the diff's 28,197 authored lines are #8665's open stack, so please land #8665 first and rebase this branch onto main, leaving the 647-line createPressTracker change, which follows architecture:react-component-runtime INV13 and shows no defect.
[Automated review]
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| suppresses text selection for that drag only; an unclaimed mouse drag keeps | ||
| today's behavior. |
There was a problem hiding this comment.
This sentence is new normative text for FR41 that main does not state: today FR41 says a mouse “otherwise keeps its released behavior”. It arrives here from #8665's stack, and an engineering owner needs to approve or reject it; approval is recorded by commenting /approve-spec <full head SHA> on the pull request.
meta#8665's review fixes: an armed press under a fine pointer brings the claiming listeners, keeps a kept press's finger and goes when the page hides; a press claimed at its own pointerdown keeps the page's selection; FR41 says what a claimed drag does with the selection. One conflict, in finePointerListeners.test.ts's imports: this pull request's createPressTracker with meta#8665's new recognizer imports, and meta#8665's new kept-press case builds its tracker with createPressTracker, since the class is gone here.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved, and the diff also needs to be split. Against main it adds text to spec:AST-094 FR41 that the current record does not state (“A mouse or pen drag that a recognizer claims takes no new text selection for that drag only. A claim made after the drag has moved clears the page's selection; a press claimed at its own pointerdown, before it moves, keeps it.”), with a matching verification row; an engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Separately, 28,320 of the diff's 28,445 authored lines are #8665's open stack, so please land #8665 first and rebase this branch onto main, leaving the 667-line createPressTracker change, which follows architecture:react-component-runtime INV13 and shows no defect.
[Automated review]
| (`spec:AST-017` FR59; DEC-9). A mouse or pen drag that a recognizer claims | ||
| takes no new text selection for that drag only. A claim made after the | ||
| drag has moved clears the page's selection; a press claimed at its own | ||
| pointerdown, before it moves, keeps it. An unclaimed mouse drag keeps its | ||
| released behavior. |
There was a problem hiding this comment.
This is new normative FR41 text that main does not state: today FR41 says a mouse “otherwise keeps its released behavior”. It arrives from #8665's stack, rewritten there since the earlier review, with the matching verification row at line 663; an engineering owner needs to approve or reject it, recorded by commenting /approve-spec <full head SHA> on the pull request.
…or, and Lab's route through core's exports 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; 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; and Lab reaching Core only through its exports, internal entries included. It absorbs meta#8073 (closed: the press tracking this rests on), meta#8562 (the internal entries) 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. Press tracking is built by createPressTracker (from meta#8763), and 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>
Three recognizers beside main's tap, long press and pan, each reading its own UIKit table group: a swipe recognized from the path between events, a pinch whose velocity settles, and a screen-edge pan from the leading or trailing edge. The edge pan takes the horizontal axis only on a view that lies within that edge's band. The sides its edges resolve to under the view's writing direction and the edge zone are measured once, together, for touch-action and the touch-down decision alike, and again when the host reports a layout change; and it is a finger's only (a mouse, a trackpad or a pen never starts it). meta#8061's change, rebased onto main as one commit; its rotation and hover recognizers and its top and bottom edges, which no component uses yet (and rotation's threshold is unmeasured), land with their first users. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
… 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. 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). 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>
meta#8360's change on the internal touch foundation: one click guard shared by the menu press and the gesture host (its click listener stays once the menu press arms it, as its record has it, and lasts only while armed for the gesture host, FR24), the host as the one touch owner (finger scroll, page holds, attachments, the press tracker hook and a held control's kept touch, the long press's single source, presented hit testing on iOS WebKit), and the gesture browser proof in Chromium and WebKit with the README. It carries meta#8074, meta#8270, meta#8293, meta#8315, meta#8320 and meta#8356, and folds in meta#8358 (the long press's tolerance comment and an unused import). Rebased onto section 2 as one commit; the touch-action rule now comes with meta#8071, and meta#8072's claimed-touchend fix, which this line never had, comes back with meta#8309. The animator's busy proof is judged against the harness's compositor-only control (meta#8906), with a main-thread mutant as its negative, and the spring's curve is placed against the control's own clock, so the late frames a loaded runner sends fail nothing. With the host serving every pointer (meta#8071), this change's own pieces follow it: a press tracker takes a mouse press as a press, painting at once and braking no scroll (FR11); a finger's or a pen's hold that does something keeps text unselected under any primary pointer, while a mouse keeps its released behavior (FR41); a claimed pen's stylus touchend is cancelled as a finger's (FR21); and `perEventPointer`, from before FR43, changes nothing. These come from meta#8665, which keeps only the undecided rule for text selection during a mouse or pen drag. The hover recognizer and the host's hover routing land with hover's first user. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
The scroll lock no longer pins <body>, which reset the page's offset to 0 (the page jumped and Safari's toolbar came back). It holds `overflow: hidden` on the root and, through the gesture host's page hold, cancels only a one-finger pan that nothing inside the overlay takes: a scroller, a range input, a pinch or a zoomed page's pan still goes through. The root's scroll styles become claim stacks (rootScrollStyles), so a lock and a pull to refresh compose. The top-layer check asks CSS.supports for `:modal` and `:popover-open` instead of catching errors from `matches`. The touch module bundle test names useScrollLock and the imports that reach it as carrying the page hold.
…come from pointer events Where a finger comes from its pointer events, a claimed finger's touchend is cancelled too (spec:AST-094 FR21), on the host's shared blocking handler and only for an enabled recognizer; a lift's claim is read after the lift, so a hold that claims with it cancels its touchend; a blur-cancelled pen's lift is refused. A second finger fails a one-finger pan wherever fingers come from, and the arena cancels a moving recognizer the browser takes. meta#8309's and meta#8336's changes, rebased onto meta#8098 as one commit. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
…king ships only the press clocks A gesture touch carries the modifier keys its press began with, so a group can leave a Ctrl-, Cmd- or Shift-click on a link to the browser. Press tracking reads only the timing table's press clocks, with a bundle guard that keeps the host's module graph small (architecture:component-style-authoring INV6), and the README documents the recognizers' kind and the press tracker factory. meta#8763's and meta#8797's changes, rebased onto meta#8309 as one commit; its press tracker factory and recognizer kind now come with meta#8076 and meta#8071.
334fe0e to
a20b6c8
Compare
vjeux
changed the title
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Requesting changes: against main, this diff carries all of #8309's open stack (31,164 of its 31,279 authored lines, including scroll-lock, DropdownMenu-trigger and press-feedback behavior changes), so it can't be reviewed or merged as the 128-line modifier-keys and press-clocks change it describes. Please land #8309 and the pull requests beneath it first, then rebase this branch onto main so the diff is this commit alone. The change itself follows the internal touch-module rules, and its tests and Core's typecheck pass. While rebasing, please also pass modifierKeysOf(event) on the untracked-touch path in GestureHost.ts, so a finger that lands where nothing takes part keeps its keys too.
[Automated review]
| untracked.push({ | ||
| id: this.nextId++, | ||
| x: touch.clientX, | ||
| y: touch.clientY, | ||
| time, | ||
| pointerType: 'touch', | ||
| target, | ||
| }); |
There was a problem hiding this comment.
This untracked contact reaches arena.untrackedBegan (line 1861), which builds the touch it hands to recognizers tracking another finger with modifiersOf(input). With no keys here, that touch always reads Ctrl, Cmd, Shift and Alt as false, unlike the tracked paths at lines 1844 and 2516. Adding ...modifierKeysOf(event), keeps the changeset's promise that a gesture touch carries the keys held when its press began.
Section 3 of the merge plan: #8360 → #8098 → #8309 → #8763 → #8342. One commit on #8309 holding only this change. It absorbs #8797 (the modifier keys), which closes with a link here; the commit credits its authors. The press tracker factory and the recognizers'
kindit used to carry now come with #8076 and #8071.Maintainer impact
architecture:component-style-authoringINV6).kindor the press tracker factory.Intended invariant
A gesture touch reports the modifier keys its press began with; press tracking reads only the press clocks, and the host's module graph stays inside its bundle guard. Nothing a control does changes.
Change
types.ts,GestureArena.ts,GestureHost.ts: a gesture touch carriesctrlKey,metaKey,shiftKeyandaltKeyas they were when its press began, read from the pointer or touch event.PressTracker.ts: imports the press clocks from the timing table, not the whole table;GestureHost.bundle.test.tspins it.README.md: each recognizer reports itskind, which code reads in place ofinstanceof(architecture:react-component-runtimeINV13), and press tracking is built bycreatePressTracker, withVelocityTrackerthe one class INV13 lists as a gap.Evidence
pointerdownortouchstart.Scope
Testing
Local gates at a20b6c8, one commit on #8309's 5fcc1c5 holding only this change (8 files, +118/−10):
pnpm test:ui(core UI): 688 files, 16,855 tests passed.pnpm exec vitest run --project node: 806 files passed; the only failures are main's own story-tree and release-cache reds, tracked in test(storybook): run the story-tree check where Storybook is built, and bring the tree back to its shape #8509 and fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520.playwright.interaction.config.ts, a phone in Chromium and WebKit): Chromium 54 passed; WebKit 45 passed and 9 skipped where its driver has no native scroll, fling, pinch-zoom or pen.tsc --project packages/core/tsconfig.json --noEmit, strict ESLint and Prettier on every changed file: clean.pnpm check:repo: clean.