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
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.
…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.
3cdb556 to
dbe81cd
Compare
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 shrink to its own commit. Against main it adds requirements the current records do not state to spec:AST-094 FR11 and FR19, architecture:react-component-runtime INV13, component:BottomSheetSwitcher FR5, component:Drawer FR5 and component:MobileNav FR3, all from the stack beneath this branch; an engineering owner records approval by commenting /approve-spec <full head SHA> on the pull request, and the author may do so as an engineering owner. Separately, 19,430 of the diff's 19,597 authored lines belong to #8360, #8098 and #8309, so those should land first and this branch be rebased onto main, leaving its own 173-line commit, which reads correctly; in that stack, useFingerScroll should call the shared useAttachment instead of copying its node-following.
[Automated review]
| `family:action-groups` member tracks activates at the lift (FR26), and a toggle | ||
| under `spec:AST-020` FR9's slide-off exception (Switch, Checkbox, | ||
| CheckboxListItem) may also complete on a lift after the finger slid off. | ||
| CheckboxListItem) may also complete on a lift after the finger slid off. Where the engine hit-tests an element a compositor animation moves at the motion's start (iOS WebKit), a touch on moving content goes to what was drawn under the finger at touch-down, and a tap there (no recognizer claimed it, the browser did not take it, and it stayed within 45 pt) MUST activate it: the browser's own click for that touch is cancelled or swallowed, and that element receives one click, an editable focused first. |
There was a problem hiding this comment.
New normative text from #8360's stack: a MUST that a tap on compositor-moving content activates what was drawn under the finger with one click, the browser's own click cancelled or swallowed. Current FR11 says a control's activation "stays the browser's click" with two exceptions, so this adds a third. It needs an engineering owner's approval, recorded with /approve-spec <full head SHA>.
| element whose recognizers decide only at the lift keeps its `touchmove` | ||
| listener passive, and its `touchend` listener stays cancelable so the | ||
| gesture can cancel the click (FR21). Known limit: a native scroll that starts on an element with a | ||
| gesture can cancel the click (FR21). While an element that holds, or sits inside, an element with a recognizer that claims moving touches moves on the compositor (iOS WebKit), the document MAY carry a non-passive `touchmove` listener; it cancels only the moves of a touch a recognizer owns under this rule and comes off when the motion ends. While it is on, a native scroll that starts anywhere on the page waits for a free main thread. Known limit: a native scroll that starts on an element with a |
There was a problem hiding this comment.
New MAY from #8360's stack: a document-level non-passive touchmove listener while such content moves, during which every native scroll on the page waits for the main thread. Current FR19 says "Only an element with a recognizer that must claim mid-gesture registers that blocking listener". It needs an engineering owner's approval, recorded with /approve-spec <full head SHA>.
| small glue in its own build over these modules. The gesture host attaches for | ||
| every pointer, with no touch-only query; under a fine pointer only a component that | ||
| uses a gesture listens (`spec:AST-094` FR19, FR24, FR43). The modules are not public exports and are not | ||
| uses a gesture listens (`spec:AST-094` FR19, FR24, FR43). The gesture host, press feedback, page hold, finger-scroll watch and click guard hear document-wide input through one capture-phase listener per event type on the window, present while a subscriber needs it. They run in subscription order, before any document or element listener; it is passive unless a subscriber cancels the event. The modules are not public exports and are not |
There was a problem hiding this comment.
New INV13 text from #8360's stack: the touch modules hear document-wide input through one capture-phase window listener per event type, in subscription order. Current INV13 does not state it. It needs an engineering owner's approval, recorded with /approve-spec <full head SHA>.
…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.
dbe81cd to
38070ea
Compare
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 shrink to its own commit. Against main it adds requirements the current records do not state to spec:AST-094 FR11 and FR19, architecture:react-component-runtime INV13, component:BottomSheetSwitcher FR5, component:Drawer FR5 and component:MobileNav FR3, all from the stack beneath this branch; an engineering owner records approval by commenting /approve-spec <full head SHA> on the pull request, and the author may do so as an engineering owner. Separately, 19,586 of the diff's 19,753 authored lines belong to #8360, #8098 and #8309, so those should land first and this branch be rebased onto main, leaving its own 173-line commit, which reads correctly and whose new tests fail before it and pass with it. In that stack, useFingerScroll should call the shared useAttachment instead of copying its node-following.
[Automated review]
| const attachedRef = useRef<{node: Node; detach: () => void} | null>(null); | ||
| // Follows the node on every commit: a ref that now points at another | ||
| // element moves the watch to it. | ||
| useIsomorphicLayoutEffect(() => { | ||
| const node = isDocument(scroller) ? scroller : scroller.current; | ||
| if (attachedRef.current?.node === node) { | ||
| return; | ||
| } | ||
| attachedRef.current?.detach(); | ||
| attachedRef.current = | ||
| node == null | ||
| ? null | ||
| : { | ||
| node, | ||
| detach: watchFingerScrolls( | ||
| getGestureHost(isDocument(node) ? node : node.ownerDocument), | ||
| node, | ||
| ), | ||
| }; | ||
| }); | ||
| // Synchronizes with the gesture host's finger-scroll watch: unmounting | ||
| // releases the watch this hook attached. | ||
| useIsomorphicLayoutEffect( | ||
| () => () => { | ||
| attachedRef.current?.detach(); | ||
| attachedRef.current = null; | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
These two layout Effects re-implement the node-following useAttachment already owns (gestures/useAttachment.ts:73-98: follow the node behind a ref after each commit, release a replaced node, release at unmount), which useGestureRecognizer and usePressTracker call. architecture:react-component-runtime INV16 asks for a shared concern to be extended in its owner and called once. Suggest letting useAttachment take a document as well as an element ref and calling it here once, so a fix to replaced-node release reaches the finger-scroll watch too.
…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.
38070ea to
ffb66b1
Compare
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 shrink to its own commit. Against main it adds requirements the current records do not state to spec:AST-094 FR11 and FR19, architecture:react-component-runtime INV13, component:BottomSheetSwitcher FR5, component:Drawer FR5 and component:MobileNav FR3; an engineering owner approves or rejects them by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- #8360, #8098 and #8309 need to land first, so this diff is its own 173-line commit instead of 19,834 lines.
hooks/scrollLockTouchGuard.ts(#8098) re-implements the edge checks and scroll-chain walk ofgestures/nativeScroll.ts; extend the native-scroll probe and call it instead.gestures/useFingerScroll.ts(#8360) re-implements the node-following ofuseAttachment; call it instead.
[Automated review]
| function canMoveY(element: Element, dy: number, range: number): boolean { | ||
| // A finger moving down drags the content down: scrollTop decreases. | ||
| return dy > 0 | ||
| ? element.scrollTop > EDGE_TOLERANCE - 0.5 | ||
| : element.scrollTop < range - EDGE_TOLERANCE + 0.5; | ||
| } | ||
|
|
||
| function canMoveX( | ||
| element: Element, | ||
| dx: number, | ||
| range: number, | ||
| isRtl: boolean, | ||
| ): boolean { | ||
| const min = isRtl ? -range : 0; | ||
| const max = isRtl ? 0 : range; | ||
| // A finger moving right drags the content right: scrollLeft decreases. | ||
| return dx > 0 | ||
| ? element.scrollLeft > min + EDGE_TOLERANCE - 0.5 | ||
| : element.scrollLeft < max - EDGE_TOLERANCE + 0.5; | ||
| } |
There was a problem hiding this comment.
These edge checks re-implement canMoveY and canMoveX from gestures/nativeScroll.ts:189-212 (the same ±0.5 px tolerance and RTL scrollLeft range), and isPanAbsorbedBeforePage below walks the same parentAcrossShadow chain as createNativeScrollProbe. nativeScroll.ts imports nothing of the gesture host, so the page hold could call it: export the edge helpers, or a chain walk that stops before the page and reports a contained edge, and delete these copies so a fix to one reaches both.
…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.
ffb66b1 to
8e3889e
Compare
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 shrink to its own commit. Against main it changes component:BottomSheetSwitcher FR5 and component:Drawer FR5 (pull-to-refresh and the page's bounce stop behind a modal flow or drawer) and component:MobileNav FR3 (the clip moves to the page's scroller). The overscroll stop also reaches Dialog, AlertDialog, BottomSheet and Lightbox, whose records do not mention it. An engineering owner records approval or rejection of these record changes by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner.
hooks/scrollLockTouchGuard.ts:38-57re-implementscanMoveY,canMoveXand the scroll-chain walk fromgestures/nativeScroll.ts, and the two already differ; please share one implementation there.- The last sentence of
.changeset/scroll-lock-keeps-the-page.mdsays closing MobileNav used to release another overlay's lock, but onmainthe lock pinned<body>while MobileNav wrote only the root, so it never did; please reword it. - 2,663 of the 2,836 changed lines are #8098 and #8309; once those land, a rebase leaves this pull request's own 173-line commit.
[Automated review]
| function canMoveY(element: Element, dy: number, range: number): boolean { | ||
| // A finger moving down drags the content down: scrollTop decreases. | ||
| return dy > 0 | ||
| ? element.scrollTop > EDGE_TOLERANCE - 0.5 | ||
| : element.scrollTop < range - EDGE_TOLERANCE + 0.5; | ||
| } | ||
|
|
||
| function canMoveX( | ||
| element: Element, | ||
| dx: number, | ||
| range: number, | ||
| isRtl: boolean, | ||
| ): boolean { | ||
| const min = isRtl ? -range : 0; | ||
| const max = isRtl ? 0 : range; | ||
| // A finger moving right drags the content right: scrollLeft decreases. | ||
| return dx > 0 | ||
| ? element.scrollLeft > min + EDGE_TOLERANCE - 0.5 | ||
| : element.scrollLeft < max - EDGE_TOLERANCE + 0.5; | ||
| } |
There was a problem hiding this comment.
These edge checks re-implement canMoveY and canMoveX from gestures/nativeScroll.ts:189-212: the same 1 px tolerance, the same ±0.5 px slack and the same RTL scrollLeft range. isPanAbsorbedBeforePage below walks the same flat-tree scroll chain as buildChain. The copies already differ: only the probe counts overflow: overlay and honours touch-action, and only this guard stops at top-layer and fixed elements. Exporting the edge checks, and a chain walk with a stop rule, from nativeScroll.ts keeps one implementation. It imports nothing of the gesture host, so the overlays' import guard still holds.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] `useScrollLock`, and so Dialog, BottomSheet, Drawer and Lightbox, keeps the page where it is while an overlay is open. The lock used to pin `<body>` with `position: fixed` at minus the scroll offset and scroll back on release: the page's offset read 0 for the whole lock, and on iOS Safari the jump to the top brought the toolbar back, where it stayed after the overlay closed. Now the page gets `overflow: hidden` (on the root, or on `<body>` when the body's own overflow is the one the viewport uses), and on touch screens a one-finger pan that would scroll the page is cancelled, which iOS Safari before 26.4 needs because it scrolls an `overflow: hidden` page under a finger. Pans that a scroller inside the overlay can take, drags on a native range input, pinch-zoom, and one-finger drags on a pinch-zoomed page (they pan the zoomed view) still work. The lock no longer pins `<body>`, and `window.scrollY` keeps its value while the lock holds. The lock cancels those pans itself, leaving a touch a gesture recognizer owns to it, and judges each pan after a small slop so a swipe's first wobble does not fix the wrong axis. It follows the browser's scroll chain, so an overlay rendered inside a scrolling container still holds the page: a modal dialog's pans go to the page, not to that container. It also sets `overscroll-behavior-y: none` on the root and `<body>`, which turns off pull-to-refresh (Chrome on Android) while an overlay is open. MobileNav's own clip goes through the same page scroll-style claims, so closing or unmounting it no longer releases another overlay's lock. |
There was a problem hiding this comment.
The last sentence says closing or unmounting MobileNav no longer releases another overlay's lock. On main it never did: the lock pinned <body> (position: fixed; overflow: hidden), MobileNav wrote only the root's overflow, and closing the drawer left the pin in place. The collision arises only once this change moves the lock onto the root. Suggested wording: "MobileNav's clip now shares the page's scroll-style claims with the lock, which also holds the root, so neither overwrites the other's overflow."
| | FR3 | During a handoff, the entering sheet is above the retained sheet. A taller retained sheet aligns down to a shorter entering sheet, and the retained sheet fades only after required transform motion completes. Under reduced motion the handoff follows `design:reduced-motion`: no transform motion runs, and the sheets cross-fade (DR2) or change at once (DR3) in the same order. | Current source and transition tests; `design:reduced-motion` DR2, DR3 | | ||
| | FR4 | Closing retains the outgoing sheet through its exit, then closes the dialog. A modal flow restores focus to the element captured when that modal flow opened. | Current source and focus tests | | ||
| | FR5 | `hasScrim=true` uses `showModal()`, a native backdrop, and native focus containment, and locks page scroll: the page does not scroll while a modal flow is presented; the lock is held continuously across a handoff, never released and re-acquired between steps; it still holds beneath a `Dialog` opened from a step; it is released after the flow's exit (DEC-1). `hasScrim=false` uses `show()` without a backdrop or scroll lock and leaves the page interactive. In a non-modal flow the sheet dialogs order themselves with local values inside the switcher's isolated flow wrapper (DEC-1); the page-level value that wrapper carries remains a `spec:AST-027` conformance gap. | DEC-1; `spec:AST-027` FR2, FR4, FR8 | | ||
| | FR5 | `hasScrim=true` uses `showModal()`, a native backdrop, and native focus containment, and locks page scroll: the page does not scroll while a modal flow is presented, and pull-to-refresh and the page's bounce stop behind it (`overscroll-behavior-y: none`), since the page is held under a modal presentation (`spec:AST-094` FR19), as behind a UIKit modal presentation; the lock is held continuously across a handoff, never released and re-acquired between steps; it still holds beneath a `Dialog` opened from a step; it is released after the flow's exit (DEC-1). `hasScrim=false` uses `show()` without a backdrop or scroll lock and leaves the page interactive. In a non-modal flow the sheet dialogs order themselves with local values inside the switcher's isolated flow wrapper (DEC-1); the page-level value that wrapper carries remains a `spec:AST-027` conformance gap. | DEC-1; `spec:AST-027` FR2, FR4, FR8 | |
There was a problem hiding this comment.
New requirement that the current FR5 does not state: pull-to-refresh and the page's bounce stop behind a modal flow (overscroll-behavior-y: none). The spec:AST-094 FR19 citation covers only cancelling one-finger pans under a modal scroll lock. This needs an engineering owner's approval, recorded with /approve-spec <full head SHA>.
| | FR3 | The desktop inline-size budget MUST accept pixel numbers or CSS lengths and remain bounded by the viewport. At or below 640px, page-reveal mode MUST preserve 56px and use `width` as a cap; full-mobile mode MUST override `width` with `100dvw`. | Current docs, styles, and width tests | | ||
| | FR4 | Visibility through Drawer-owned behavior MUST follow `isOpen`, from the render after hydration on (FR17). Uncanceled Escape routed by the shared stack and uncanceled modal-backdrop activation request `false` when `purpose` allows (FR15), and a `DrawerHeader` close action requests `false` through the callback it is given; Drawer prevents its handled native `cancel` event from closing the dialog directly. During controlled exit, Drawer keeps its host and content region mounted while rendering the caller's current `children`. | Current prop contract and lifecycle tests | | ||
| | FR5 | At open time, `hasScrim={true}` MUST select native `showModal()` with `aria-modal`, a focus trap, body scroll locking, and a visible backdrop; `hasScrim={false}` MUST select a manual Popover API host that leaves the page behind interactive. Changing `hasScrim` while open is not a supported native-mode transition. | Current source, docs, and initial-mode tests | | ||
| | FR5 | At open time, `hasScrim={true}` MUST select native `showModal()` with `aria-modal`, a focus trap, page scroll locking, and a visible backdrop; under the lock, pull-to-refresh and the page's bounce stop (`overscroll-behavior-y: none`), since the page is held under a modal presentation (`spec:AST-094` FR19), as behind a UIKit modal presentation; `hasScrim={false}` MUST select a manual Popover API host that leaves the page behind interactive. Changing `hasScrim` while open is not a supported native-mode transition. | Current source, docs, and initial-mode tests | |
There was a problem hiding this comment.
Same new requirement as BottomSheetSwitcher FR5, plus "page scroll locking" in place of "body scroll locking". It needs the same engineering-owner approval.
| | FR1 | MobileNav renders one native `<dialog>` Navigation overlay containing a Drawer with a stable Header, Close button, and scrollable Content region. The overlay carries `mobile-nav` with the resolved `side`; Drawer, Header, and Content carry no MobileNav public target. | Current source, docs, and tests | | ||
| | FR2 | `isOpen` and `onOpenChange` win when given; otherwise the drawer reads and changes the enclosing AppShell's drawer state. The dialog uses AppShell's drawer id when there is one, else its own; a caller `id` replaces either (Current-state impact). | Current source and tests | | ||
| | FR3 | Opening calls `showModal()`, and records the focused element as the opener; where focus lands inside the open drawer is uncontracted here. While open, the page's root clips its overflow and keeps its scrollbar gutter; closing releases both. | Current source and tests | | ||
| | FR3 | Opening calls `showModal()`, and records the focused element as the opener; where focus lands inside the open drawer is uncontracted here. While open, the page's scroller clips its overflow and keeps its scrollbar gutter: the root, or `<body>` when `<body>` sets its own overflow and the root's stays `visible`; closing releases both. | Current source and tests | |
There was a problem hiding this comment.
This moves the clip from the page's root to the page's scroller (<body> when <body> sets its own overflow and the root's stays visible), which the current FR3 does not state. It needs an engineering owner's approval.
A gesture touch carries the modifier keys its press began with, read from its pointer or touch event, so a group can leave a Ctrl-, Cmd- or Shift-click on a link to the browser. A finger that lands where nothing takes part reports its keys to a recognizer tracking another finger too. Nothing reads the keys yet: the group that leaves such a click to the browser is their first user.
8e3889e to
b29b3c5
Compare
vjeux
changed the title
github-actions
Bot
removed
needs:design-review
There was a problem hiding this comment.
Recommending approval. Gesture touches now carry the modifier keys their press began with. The change stays inside Core's internal gesture modules, and nothing reads the keys yet, so no behavior changes. Both new tests fail against main's gesture sources and pass here.
Optional comment tidy-ups, none blocking:
- The one-line JSDoc on
modifierKeysOf(GestureHost.ts) andmodifiersOf(GestureArena.ts) repeats each helper's name and return type, so both can go. - The
TouchModifiersJSDoc (types.ts) joins two topics in its second sentence, and it says a group of links already leaves modified clicks to the browser, though nothing reads the keys yet.
[Automated review]
| return event.composedPath()[0] ?? retargeted; | ||
| } | ||
|
|
||
| /** The modifier keys an input event reports, for the contact it begins. */ |
There was a problem hiding this comment.
Non-blocking: this repeats the name and the TouchModifiers return type. It could go.
| */ | ||
| export type ClickDisposition = 'allow' | 'suppress' | 'defer'; | ||
|
|
||
| /** The modifier keys a contact's input reports, none when left out. */ |
There was a problem hiding this comment.
Non-blocking: the name and the === true lines below already say this. It could go.
| * reports them. A Ctrl-, Cmd- or Shift-click on a link opens a new tab or | ||
| * window, and a group of links leaves that to the browser. |
There was a problem hiding this comment.
Non-blocking: this sentence joins two topics, and no group reads these keys yet. Suggested: "A Ctrl-, Cmd- or Shift-click on a link opens a new tab or window. A group of links needs these keys to leave that click to the browser."
One commit on main. It absorbs the closed #8797, whose change this is. Leaves draft with its first reader: the group that leaves a modified click on a link to the browser.
Maintainer impact
A group of links (a tab bar, a menu of links) that takes presses through the gesture host cannot tell a Ctrl-, Cmd- or Shift-click from a plain one, so it cannot leave the browser its own open-in-a-new-tab.
Intended invariant
A gesture touch reports the modifier keys its press began with. Nothing a control does changes: no component reads the keys yet. No behavior change.
Change
types.ts:GestureTouchcarriesctrlKey,metaKey,shiftKeyandaltKey; a new contact's input may give them, none when left out.GestureArena.ts: a touch takes the keys its input gives.GestureHost.ts: it reads them from the pointer or touch event that begins a contact, a finger that lands where nothing takes part included.Evidence
pointerdownortouchstart.Scope
Testing
Local gates at b29b3c5, one commit on main 3615956:
GestureHost.test.ts: the two modifier-key cases fail with main's host and pass here. The units ofgestures,hooks,utilsand Core's own tests: 134 files, 2,552 tests passed. The touch-support bundle test: 9 passed.tsc --project packages/core/tsconfig.json --noEmit, strict ESLint and Prettier on the changed files,check:*andpnpm review:pre: clean.