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
… for a recognizer inside a shadow root) into the meta#8228 helper
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The context request needs an engineering owner's decision before it can be approved: attachContextRequest rests on spec:AST-094 FR43 and DEC-21, which exist only in open #8646, and its hold attaches a recognizer with a blocking touchmove under a fine pointer, which FR19, FR24 and architecture:react-component-runtime INV13 on main still rule out. An engineering owner records that #8646's text governs here by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the change waits for #8646 to land; that decision should also settle whether a hold-repeat control keeps the row's right click and Shift+F10, which no record states yet. Independently of it:
- Land #8665 and its ancestors first, or retarget, so this diff is the context request's 676 lines rather than 29,372.
- Refuse a pen's own
contextmenuafter its hold and keep a pen barrel press from starting a press, since a pen now gets two requests for one gesture. - Schedule
hold.requestMson the arena's clock and report its deadline, as FR16 requires, with a test.
[Automated review]
| if ( | ||
| pointerType === 'touch' || | ||
| host.fingersDown() > 0 || | ||
| time - lastKeyRequestAt < KEY_ECHO_MS | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
A pen's own contextmenu is not refused here: fingersDown() counts no stylus (GestureHost.ts:2576) and the type is pen, not touch. A pen held still, or a pen barrel press held past 500 ms, then gets hold and pointer for one gesture. Could the handler refuse a menu event from the pointer the hold is tracking, and for a short window after the hold's request, as it does for the key?
| if (requestMs === commitMs) { | ||
| requestHold(); | ||
| } else { | ||
| requestTimer = setTimeout(requestHold, requestMs - commitMs); | ||
| } |
There was a problem hiding this comment.
hold.requestMs waits on a wall-clock setTimeout and stamps host.arena.now() when it fires, so a busy main thread moves the reported time and the arena clock cannot drive it in tests; spec:AST-094 FR16 wants the deadline. Scheduling it on the arena's timer and reporting commit + (requestMs - commitMs) would match the default path, which reports its 500 ms deadline.
| } | ||
| // A right click, or a ctrl-click on macOS (its context click), starts no | ||
| // press (FR43). | ||
| if (event.pointerType === 'mouse' && !isPrimaryClick(event)) { |
There was a problem hiding this comment.
Only a mouse is filtered here, so a pen's barrel button (button: 2) still starts a press, while utils/primaryClick.ts says a pen's barrel button starts none. Applying isPrimaryClick to a pen (here and at line 1149) would also stop the barrel press from feeding the context request's hold.
| * listener is installed. A recognizer that must not begin under a second | ||
| * finger reads it in `shouldBegin`. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
This method lands between fingersDown()'s JSDoc (lines 1540-1547) and fingersDown() itself, so that method loses its documentation and this one carries two blocks. Moving keepsPressBetween above the fingersDown comment keeps both.
d642a33 to
fbb40c1
Compare
|
Thanks. At fbb40c1 a pen that leaves the long press's tolerance before its duration is no hold, so a barrel right click right after such a drag makes its request, and a drag-then-right-click case pins it. The context request's comment now keeps only its reason. |
There was a problem hiding this comment.
Changes requested: the context request follows spec:AST-094 FR43, FR41, FR12, FR15 and FR24 and the owner's rulings on the hold-repeat control and the hold options, and a barrel right click after a pen drag now makes its request, but one pen case still opens nothing.
GestureHost.isPenDownIn(packages/core/src/gestures/GestureHost.ts:1349-1356) also counts a lifted pen that a recognizer is still deciding on, such as a double tap waiting 350 ms after the lift, so after a pen tap on a double-tap target in the row,contextRequest.ts:142-146cancels that pen's barrel right click with no request; skippingendedandcancelledtouches there, astouchForCancelledPointerdoes, fixes it, and a pen-tap-then-right-click case on a double-tap target belongs beside the other pen cases.
Non-blocking: the notePenDrift JSDoc (GestureHost.ts:3824) restates the condition under it, and driftedPens' JSDoc already gives the reason.
[Automated review]
| return this.arena.touches.some( | ||
| touch => | ||
| touch.pointerType === 'pen' && | ||
| touch.target instanceof Node && |
There was a problem hiding this comment.
arena.touches also holds lifted pens awaiting a decision (GestureArena.ts:208-211). After a pen tap on a target with a double tap, which waits 350 ms after the lift, this stays true, so contextRequest.ts:142-146 cancels that pen's barrel right click in the row with no request (a unit probe and trusted pen input in Chromium both show it). Skipping touch.phase === 'ended' || touch.phase === 'cancelled' here, as touchForCancelledPointer does, keeps all 16 context-request cases passing and makes that click request; a hold's own lift stays covered by lastPenLiftIn.
| ); | ||
| } | ||
|
|
||
| /** Note a pen contact that leaves the long press's tolerance before its duration. */ |
There was a problem hiding this comment.
Non-blocking: this restates the condition below, and driftedPens' JSDoc already says why. Drop it, or keep only the reason: "Such a contact is no hold, so its lift arms no menu refusal."
fbb40c1 to
39b1dcb
Compare
|
Thanks. At 39b1dcb a lifted pen that a recognizer still decides on no longer counts as down, so a barrel right click right after a pen tap on a double-tap target makes its request, and a case pins it. The drift helper's comment now keeps only its reason. |
There was a problem hiding this comment.
Recommending approval. attachContextRequest follows spec:AST-094 FR43 and its neighbouring claims (FR41, FR12, FR15, FR24) and the engineering owner's rulings on the hold-repeat control and the cut hold options, and a barrel right click right after a pen tap on a double-tap target now makes its request, pinned by a new case.
Two optional follow-ups, neither blocking:
- A held Shift+F10 or context-menu key makes one request per auto-repeat, because
onKeyDownnever readskey.repeat; cancelling repeats without requesting keeps the README's one request per gesture. onHoldreportscommittedbefore it readsisEnabled, so a request disabled during the hold reportscommittedwith no request; reword its JSDoc or readisEnabledfirst.
[Automated review]
| (key.key === 'F10' && key.shiftKey) || key.key === 'ContextMenu'; | ||
| // A hold-repeat control inside owns only its hold: Shift+F10 on it | ||
| // reaches this request. | ||
| if (!isRequestKey || !isEnabled() || answered.has(key)) { |
There was a problem hiding this comment.
Non-blocking: an auto-repeated keydown (key.repeat) passes this check too, so a held Shift+F10 or context-menu key makes one request per repeat. Cancelling a repeat without requesting would keep one request per key press.
| recognizer.addListener(() => { | ||
| const {state} = recognizer; | ||
| if (state === 'began') { | ||
| options.onHold?.('committed'); |
There was a problem hiding this comment.
Non-blocking: committed is reported before isEnabled is read, so a request disabled during the hold reports committed (then ended) with no request, while the option's JSDoc says committed comes as the hold requests. Reword the JSDoc, or read isEnabled first.
39b1dcb to
c9f05eb
Compare
|
Rebased onto main at c9f05eb. The only conflict was one line in the gesture host's cancel path: main now also clears its pointer kinds there, and both lines stay. The change itself is identical to the approved head. |
There was a problem hiding this comment.
Recommending approval. attachContextRequest follows spec:AST-094 FR43 and its neighbouring claims (FR41, FR12, FR15, FR24), and the engineering owner's rulings on the hold-repeat control and the cut hold options. Two optional follow-ups, fine to leave for the context menu that adopts it:
- A held Shift+F10 makes one request per auto-repeat (
onKeyDownincontextRequest.ts); ignoringkey.repeatafter cancelling it keeps the README's one request per gesture. onHoldreportscommittedfor a request disabled during the hold, while its JSDoc sayscommittedcomes as the hold requests; readisEnabledfirst or reword the JSDoc.
[Automated review]
| (key.key === 'F10' && key.shiftKey) || key.key === 'ContextMenu'; | ||
| // A hold-repeat control inside owns only its hold: Shift+F10 on it | ||
| // reaches this request. | ||
| if (!isRequestKey || !isEnabled() || answered.has(key)) { |
There was a problem hiding this comment.
Non-blocking: a held Shift+F10 or context-menu key requests again on every auto-repeat keydown, so the README's "Each gesture makes one request" does not hold for a held key. Cancelling a repeat (key.repeat) without requesting keeps one request per key press.
| recognizer.addListener(() => { | ||
| const {state} = recognizer; | ||
| if (state === 'began') { | ||
| options.onHold?.('committed'); |
There was a problem hiding this comment.
Non-blocking: committed is reported before isEnabled is read, so a request disabled during the hold reports committed then ended with no request, while the onHold JSDoc says committed comes as the hold requests. Read isEnabled first, or reword the JSDoc.
useLongPress takes her createLongPressDetector with stages, the structure kept whole, and my side's two changes on it: the defaults come from the one long-press source (motion/uikit/longPress.ts) and a long press's drift is a distance, as UIKit measures allowableMovement ((7, 7) keeps the press, (7.2, 7.2) ends it). Her measured context-menu tolerance is per axis ((11, 11) still opens the menu, M-CM3), so the detector gains isPerAxis and contextMenuPress passes it. Her FR24 case 'leaves no timer of the lift behind when it unmounts while the lift is still growing' found the click guard's 400 ms window (utils/clickGuard.ts, from my side) still armed at unmount: that timer is the page's, not the lift's, so the case disarms the guard before it counts. The two READMEs take both sides' rows and sections.
… for a recognizer inside a shadow root) into the meta#8228 helper
… when asked, cancels on a drift, and times its request on the gesture clock) into the meta#8228 helper
…, with no stages (spec:AST-094 FR43) Red: meta#8670 cuts the context request's two-stage hold (commitMs, requestMs, requestOnLift) and its per-axis allowance, as asked there: the hold is UIKit's long press and requests as it begins. ContextMenu and the factory's menu still open a held finger at 735 ms, open at a lift after 400 ms, and allow 11.5 pt on each axis. They should open at 500 ms under any primary pointer, open nothing for a lift before it, and open nothing for a drift past 10 pt.
meta#8665 now carries meta#8360's new head and the maintainer's host review: a blur ends an armed press, a kept press's touchmove follows the press, hover reaches every pointer, and a pen's barrel button starts no press. The one conflict, utils/pressFeedback.ts, takes meta#8665's text: both merges of main resolved it the same way, and this pull request does not change that file.
…peat control owns only its hold - A pen held still requests through the hold, and the browser then fires the pen's own context menu event, during the hold or at its lift: a second request for one gesture. One request only, the hold's. - The engineering owner's ruling on meta#8670: a hold-repeat control inside a row owns only its hold; its right click and Shift+F10 still reach the row's context request. Today both are refused there. Both fail until the context request refuses the pen's echo and leaves a kept press only its hold.
… with no options a record does not need The engineering owner's ruling on meta#8670: cut the two-stage commitMs, requestMs and requestOnLift, and the allowableMovement and isPerAxis tolerance, until a record calls for them. The hold is UIKit's long press, 500 ms and 10 px as a distance, and requests as it begins; onHold reports it committed, then ended or cancelled. LongPressGestureRecognizer loses isPerAxis and geometry its axisTravel, with their tests.
…press, with no options) into the headless context menu packages/core/src/utils/pressFeedback.ts and its test: meta#8670's text, with this stack's press-paint subscription (subscribePressPaint, which ContextMenu's press feedback follows) kept beside it. packages/core/src/hooks/useLongPress.ts, its test, its page and useMenuPress.ts: meta#8670's text. This stack's useLongPress `stages` and press start times served ContextMenu's two-stage hold, which no longer exists (the context request opens ContextMenu, and its hold has no stages), so they go, with their changeset and the hooks entry's LongPressStage type.
… with none of the hold options meta#8670 cut (spec:AST-094 FR43) meta#8670 cuts the context request's two-stage hold (commitMs, requestMs, requestOnLift), allowableMovement and isPerAxis, as asked there: the hold is UIKit's long press and requests as it begins. ContextMenu and the factory passed them for UIContextMenuInteraction's 400 ms commit, 735 ms menu, open at a lift and 11.5 pt on each axis. They pass none now: under any primary pointer a held finger gets the menu at 500 ms, a lift before it is a tap, and a drift past 10 pt (a distance) opens nothing. Under the touch model the commit, now the long press, plays the haptic and the press feedback plays UIKit's press curve until the menu shows. The UIKit table's per-axis drift guess has no reader left and goes. The tests, the interaction specs, the docs, the story and the changesets that described the two-stage hold describe the long press; the tests of a lift after the commit and of a drag between the commit and the menu go with that window.
…ck and Shift+F10
A context menu should open the same way from a held finger or pen, a
right click, and Shift+F10 or the context-menu key. Each component wired
its own handlers, so the rules differed: a held finger on Android could
open the browser's own menu beside Astryx's.
attachContextRequest(element, {onRequest}) is the one internal request a
context menu answers. Each gesture makes one request, naming its input (a
hold, a pointer or the keyboard), so a keyboard-opened menu can focus its
first row. The hold is UIKit's long press and an arena participant, so it
loses to a claim another recognizer made first. A mouse held still makes
no request. A control that keeps its press for a hold-repeat owns only its
hold, while its right click and Shift+F10 still reach the request. The
browser's own menu is refused once, for a finger while it is down inside
the element and for a pen during and right after any hold of it there.
Inside nested requests, a right click or a key reaches only the innermost.
It listens on its own element only, and only while enabled.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. attachContextRequest follows spec:AST-094 FR43 and its neighbouring claims (FR41, FR12, FR15, FR24), and the engineering owner's rulings on the hold-repeat control and the cut hold options; nothing blocks.
Next: mark the pull request ready for review, and a maintainer adds the ci:run label so CI runs on this fork branch.
Two optional follow-ups, fine to leave for the context menu that adopts it:
- A held Shift+F10 makes one request per auto-repeat (
onKeyDownincontextRequest.ts); ignoringkey.repeatafter cancelling it keeps the README's one request per gesture. onHoldreportscommittedfor a request disabled during the hold, while its JSDoc sayscommittedcomes as the hold requests; readisEnabledfirst or reword the JSDoc.
[Automated review]
User need
A context menu should open the same way from a held finger or pen, a right click, and Shift+F10 or the context-menu key. Each component wired its own handlers, so the rules differed: a held finger on Android could open the browser's own menu beside Astryx's.
Problem and solution fit
One internal
attachContextRequest(element, {onRequest})decides for every component. It reports one request per gesture, naming its input (hold,pointerorkeyboard), its pointer and its point, so a keyboard-opened menu can focus its first row. No shipped component uses it yet; the headless context menu and row menus build on it next.Current specification
spec:AST-094FR43: one shared context request from a long press, a context click, or Shift+F10 and the context-menu key; FR24 for listening only while enabled. No record changes.Public delta and caller burden
None: the module is internal and not exported. A component passes its element, an
onRequestcallback, and optionallyisEnabledandonHold.Design and alternatives
Evidence
Scope
Leaves draft with #8693, its first user. Feel check: not applicable here; #8693 carries the human check.
Testing
Local gates at 9e2d08a, one commit on main 273031c. It replays c9f05eb onto today's main:
git range-diffchanges only main's context lines around oneGestureHost.tshunk. There, main'shold?.keepLiftSelection()and this PR's pen-lift note both stay.tsc --project packages/core/tsconfig.json --noEmit, strict ESLint, Prettier on the changed files andpnpm review:pre(no findings): clean.CI=1,LP_NUM_THREADS=4, one worker), three runs: 177 passed and 36 skipped, none failed.