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]
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]
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]
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]
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.
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]
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. A held key's repeats make no request. The
hold is UIKit's long press and an arena participant, so it loses
to a claim another recognizer made first. A hold disabled before
it requests reports nothing. 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, because the pull request is still a draft from a fork. To run it, mark the pull request ready for review, and a maintainer adds the ci:run label.
attachContextRequest follows spec:AST-094 FR43 and its neighbouring current claims (FR12, FR15, FR24, FR41) and the engineering owner's rulings on hold-repeat controls and the hold options; the unit cases and the Chromium browser case cover each rule, and the earlier notes on a held key's repeats and onHold's committed are fixed.
Non-blocking:
- The module-scope
answeredset (contextRequest.ts:62) would split under a swizzled copy of the module, so a package request nested in a copied one could answer the same right click twice; keeping it in a realm store, as the gesture host registry does, keeps one set.
[Automated review]
| * Menu events and keys a request already answered: one inside nested | ||
| * requests reaches only the innermost, as a hold resolves to one winner. | ||
| */ | ||
| const answered = new WeakSet<Event>(); |
There was a problem hiding this comment.
Non-blocking: a swizzled component that copies this helper keeps its own set, so a right click on a package request nested inside a copied one, or the reverse, makes two requests. The gesture host already lives in one realm store (hostRegistry.ts:23-24); realmValue(sharedKey('gestures/context-request', 1), () => new WeakSet<Event>()) from utils/sharedRealm would keep one set here too.
|
/approve-spec 8cb9f21 |
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 on its own; #8693, its first user, builds on it. Feel check: not applicable; nothing visible changes until a component adopts it.
Testing
Local gates at 8cb9f21, one commit on main b6b0c68. It replays ccfe5a8 onto today's main, with one fixture header sentence split in two. The runs below used 6fdf902, the replay before that comment-only split.
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), ten runs: 690 passed and 120 skipped, none failed.