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
cf96937 to
5f964ec
Compare
5f964ec to
bc6133a
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request is on hold for a spec decision: the shared presentation hold in packages/core/src/Layer/presentationHold.ts and the draft spec:AST-094 requirements (FR1–FR23) have no current governing record, so an engineering owner needs to settle whether the Layer module owns one hold for every layer (today architecture:layer-runtime INV9 keeps pointer and touch handlers with their owning component), identify the record that carries it, and approve or reject AST-094 by commenting /approve-spec <full head SHA>; the author may do so as an engineering owner. Independently of that decision, these changes are needed:
- Land the six dependency pull requests and rebase, so the compare against
mainholds only the presentation-hold commit instead of 37,287 authored lines. - Release the
usePressFeedbacktext-selection lock with a non-experimental changeset, sincespec:AST-017FR22 keeps stable runtime behavior out of[experimental]. - Remove the
platformadaptation axis frompackages/cli/assets/theme.template.ts, whichdefineThemerejects. - Let the status effect in
useAlertMotionrelease its own hold, so StrictMode's effect replay acquires it again. - Correct the two changesets that misstate the focus change and the theme
mdpoint.
[Automated review]
| * are primary-pointer precision, contrast preference, motion preference, and | ||
| * platform (`ios` for the browsers of iPhone and iPad, `other` for the rest). |
There was a problem hiding this comment.
This lists a platform adaptation axis, but defineTheme refuses when: {platform: 'ios'} (packages/core/src/theme/expandMotionRoles.test.ts:137-145) and spec:AST-012 FR2 closes when to width, height, pointer, contrast and motion. A theme author who follows this gets a build error; drop the platform clause.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [experimental] A held finger or pen no longer starts text selection or the iOS callout on a press-tracked control or cell, or on a recognizer created with the new `preventsTextSelection` option (`@astryxdesign/core/gestures/experimental`), as on UIKit controls. The gesture host turns them off for the contact and restores them when it ends, on a non-text input under the finger inside such an element too (WebKit's own style makes every input selectable), and only for recognizers that would take the touch; the document press controller (`usePressFeedback` rows) takes the same lock on any coarse pointer. Text fields keep their selection, a mouse keeps desktop selection, and other pans leave their content selectable. |
There was a problem hiding this comment.
The usePressFeedback part of this changes stable runtime behavior on Button, Link, Item and every other press-feedback surface under a finger, which spec:AST-017 FR22 keeps out of [experimental]. Give that part its own [fix] or [feat] entry naming the stable change.
| // Unmounting lets the alert go at once, hold included. | ||
| useEffect( | ||
| () => () => { | ||
| releaseHoldRef.current?.(); | ||
| releaseHoldRef.current = null; | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Under <StrictMode> this cleanup runs in the effect replay and releases the hold that the status effect below acquired; for an alert mounted open, the replayed status effect returns at was === now, so the hold never comes back in development (architecture:react-component-runtime INV3). Letting the status effect own the release of its hold would make the replay re-acquire it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: one presentation hold for every layer (`Layer/presentationHold`). While a layer presents, a touch, a press or a click anywhere reaches nothing (keys and assistive technology still act); while it dismisses, the same, and the layer's content is inert. The gesture host holds the layer's touches through its touch gates. AlertDialog's touch presentation now uses it, with no change in behavior. Not exported from the package entry point. |
There was a problem hiding this comment.
Focus now leaves the fading alert's content (its children become inert), as the pull request description says, so “no change in behavior” undersells it; name the focus change.
| // 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 is the default compact definition, so a theme that overrides md still gets 768 px here, while .changeset/touch-presentation.md promises the theme's md point (spec:AST-012 FR9 asks for the effective md). Either read the active theme's definition or say 768 px in the note.
bc6133a to
77d538e
Compare
vjeux
changed the title
Under a finger AlertDialog held its own input during its presentation: the action tracker dropped touches through an isInert predicate, and render guards dropped a click, Enter or Escape while it left. It now uses the Layer module's one hold (meta#8342): - useAlertMotion holds the alert with scope 'viewport' from the presentation's start until UIKit's touch window after the start the browser reports (alertController.present.ignoresTouchesForMs, a cancellable timer), or until the reduced-motion cross-fade ends; and with scope 'layer' until the dismissal's fade ends. acceptsTouches reads the hold. - AlertDialogTouch drops the tracker's isInert predicate: the gesture host's touch gate holds the alert's touches. AlertDialog drops its click guards; the Escape guard stays, since inert content does not stop the dialog's own cancel event. One detail moves: while the alert fades out, focus leaves its inert content, and Dialog still returns it to the opener at the end. The leave test now also checks what a tap hits (the shield, on the alert and over the opener) and a key during the exit (focus already out, nothing runs).
d7f968b to
2eec9ff
Compare
There was a problem hiding this comment.
This pull request is on hold for a spec decision. No current record states a shared presentation hold in the Layer module (packages/core/src/Layer/presentationHold.ts), and architecture:layer-runtime INV9 keeps backdrop, pointer and touch handlers with their owning component. An engineering owner needs to provide or identify the record that states the hold and its shield, and to record that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever that decision is, these changes are needed:
- Land #8045, #8062, #8074, #8075 and #8336 and rebase, so that the compare against
mainholds only the presentation-hold commits instead of 32,819 authored lines. - Give the velocity tracker the stillness reset that
spec:AST-100FR2 requires, and drop its alternative filters and thevelocityStrategyoption (architecture:interaction-modalityINV10);PanGestureRecognizer.test.tscurrently expects 2,000 px/s after a still pause. - File
.changeset/reduced-motion-record.mdas an internal change, since the animator it describes is not exported. - Either limit the press-feedback selection lock to touch, or show the before-and-after pen evidence that
spec:AST-017FR59 asks for.
[Automated review]
| * header describes, `uikit` by default. | ||
| * @experimental | ||
| */ | ||
| export type VelocityStrategy = 'uikit' | 'lsq2' | 'iosFling' | 'window'; |
There was a problem hiding this comment.
spec:AST-100 (current) says the first Core velocity source has "no selectable alternative filters", and architecture:interaction-modality INV10 keeps one release filter. FR2 also requires a release of 0 after a still finger, while the uikit filter "never decays" (lines 13-20) and PanGestureRecognizer.test.ts:77-92 expects 2,000 px/s after a 100 ms rest. Keeping only uikit, adding the stillness reset and removing velocityStrategy would conform.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] The experimental touch animator follows design:reduced-motion. Under Reduce Motion every `animate` and `play` (travel or opacity, on a spring or any curve) becomes an opacity-only cross-fade of 200 ms on a coarse primary pointer, or for a call marked `touchRelease`, and completes at once on any other pointer; nothing moves, scales or springs, and travel lands with a plain style write while the element is transparent, never as an animated transform. `timing.reducedMotionCrossFadeMs` is now 200 ms and is no longer a theme role. The `reducedMotion` and `crossfade` call options are removed; `crossfadeAs` stays. `animator.reducedMotion()` reports the mode for feedback with no fade of its own, `reducedMotionFade()` gives the same fade to code beside the animator, and `reducedMotion.stylex.ts` holds its constants for StyleX transitions. |
There was a problem hiding this comment.
The animator has no export path at this head, and uikit-motion-animator-curves.md already records this behavior as [chore] Internal. As a [fix] naming removed reducedMotion and crossfade options, this reads as a change to an API consumers could import. Filing it as [chore] Internal would match its siblings.
| if (!isHeld || layer == null) { | ||
| return; | ||
| } | ||
| // Released by the cleanup alone. |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks a new Effect to name the external system it synchronizes. A comment saying it synchronizes the layer's hold (its shield, its content's inert, the gesture host's gate) with isHeld would cover it; the acquire and release pairing is already right.
| @@ -283,6 +301,9 @@ function onPointerDown(event: PointerEvent): void { | |||
| gesture: idlePressGesture(), | |||
| pointerId: event.pointerId, | |||
| timer: null, | |||
| // A held finger never starts text selection on the surface (UIKit's | |||
| // controls and cells never do), on any coarse pointer. | |||
| unlock: lockSelection([element], event.target), | |||
There was a problem hiding this comment.
This lock also runs for a pen (only mouse returns early at line 270), and a pen is a fine pointer under spec:AST-017 FR59, which asks for before-and-after fine-pointer evidence. Either limit the lock to pointerType === 'touch', or keep pen and show that evidence.
There was a problem hiding this comment.
Approval waits on a spec decision: no current record gives the Layer module an input hold for every layer, so an engineering owner needs to record its presenting scope beyond spec:AST-094 FR35's growth case and its dismissal scope (inert content, focus not given back), or accept both as filed by commenting /approve-spec d535664aec9b191e539ecfa9834f79549d4eaf83 on this pull request, which the author may do as an engineering owner. Separately, these need changes:
- Land #8336 first and retarget, so the diff is the five input-hold files rather than 24,028 authored lines.
- Fix the hold itself: keep the shield until a touch still down at the release ends (
inputHold.ts:236-242), stop held presses reaching capture-phase listeners (:176-178), makeuseInputHoldfollow a replaced node (:317-327), and remove the touch gate with the last hold, with a gate test that fails without it (inputHold.test.tsx:189-204).
[Automated review]
There was a problem hiding this comment.
Approval waits on a spec decision: spec:AST-094 FR35 now covers holding input while a presentation enters, but no current record gives the Layer module one input hold for every layer or its dismissal scope (every touch dropped, content inert, focus not given back), so an engineering owner needs to record it, with or without that scope, or accept the module as filed by commenting /approve-spec with this pull request's full head SHA, which the author may do as an engineering owner. Separately, these need changes:
- Land #8336 first and retarget, so the diff is the five input-hold files rather than 24,078 authored lines.
- Keep the shield until a touch still down at the release ends (
inputHold.ts:236-242), and makeuseInputHoldfollow a replaced or late node (:317-327). - Fix
InputHoldScope's "the layer alone" (:68): both scopes cover the viewport, so FR35's non-modal sheet case has no scope. - Remove the touch gate with the last hold, with a test that fails without it (
inputHold.test.tsx:189-204).
[Automated review]
|
Offer for this PR: the input hold decides by each event's pointer ( What it fixes: the shield takes every hit, so on a touch screen a mouse or a trackpad loses its presses while a presentation enters. FR35 says "A mouse keeps today's behavior". Found in #8630's review. What changes:
This also covers the touch still down at the release from my earlier offer here (it now reaches nothing to its end), so it supersedes that offer. A test that observes a held touch listens on the window before the hold starts: the document no longer hears it. Evidence:
The window logic comes from the Dialog and Drawer entrance hold built for #8630, which will call |
There was a problem hiding this comment.
Approval waits on a spec decision: spec:AST-094 FR35 covers holding pointer input while a presentation enters, but no current record gives the Layer module one input hold for every layer or its dismissal scope (every hit taken, content inert, focus not given back), so an engineering owner needs to record it, with or without that scope, or accept the module as filed by commenting /approve-spec with this pull request's full head SHA, which the author may do as an engineering owner. Whatever that decision, these need changes:
- Land #8336 first and retarget, so the diff is the five input-hold files rather than 24,137 authored lines.
- Let a mouse keep its path while a layer is held (FR35: "A mouse keeps today's behavior"), and hold a touch still down at the release to its end (
inputHold.ts:129-138,:236-242). - Fix
InputHoldScope's "the layer alone" (inputHold.ts:68), since both scopes cover the viewport and FR35's non-modal sheet case has no scope. - Make
useInputHoldfollow a replaced or late node (inputHold.ts:317-327), and remove the touch gate with the last hold, with a test that fails without it (inputHold.test.tsx:189-204).
[Automated review]
| // shield takes hits whatever its layer root opts out of (a full-viewport | ||
| // layer that is `pointer-events: none` and lets a backdrop child take the | ||
| // hits): inherited, they would let a held tap fall through to the page. | ||
| pointerEvents: 'auto', |
There was a problem hiding this comment.
With pointer-events: auto the shield takes every pointer's hits, and SWALLOWED (lines 129-138) stops mouse presses and clicks, while the gesture host's gate also empties a held target's participants for every pointer type (gestures/GestureHost.ts:1413-1418). spec:AST-094 FR35 keeps the mouse path ("A mouse keeps today's behavior"), so a mouse or trackpad on a device whose primary pointer is coarse loses its presses while a layer enters. Deciding by each event's pointer, with a mouse case in the browser spec, would conform.
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
There was a problem hiding this comment.
Approval waits on a spec decision: architecture:react-component-runtime INV13 now names the input hold and spec:AST-094 FR35 covers its presenting scope, but no current record states its dismissal scope (scope: 'layer': a full-viewport shield, inert content, and focus blurred out of the layer while it dismisses). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the dismissal scope is written into a current record, or the hold keeps only its presenting scope. Whatever is decided, these still need changes:
- Land #8336 first and retarget, so this pull request is the five input-hold files rather than 28,479 authored lines.
- Let mouse and trackpad presses through during a hold, since FR35 keeps the mouse path, and add a mouse case.
- Hold a touch that is still down when the hold ends until it lifts, so its release cannot reach the page beneath.
- Stop held presses before the page's capture listeners and add
dblclickandauxclick, correct the'layer'scope's documentation, follow node replacement inuseInputHold, and remove the touch gate with the last hold, with a test that fails without it.
[Automated review]
There was a problem hiding this comment.
Approval waits on two spec decisions: no current record states the input hold's dismissal scope (scope: 'layer': a full-viewport shield, inert content, and focus blurred out of the layer while it dismisses), and this head rewrites spec:AST-094 FR41 (a mouse or pen drag a recognizer claims takes no text selection) through the merged #8336 without an exact-head spec-owner approval. An engineering owner can record both decisions by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the dismissal scope can be written into a current record or cut and the FR41 text can land with #8336. Whatever is decided, these still need changes:
- Land #8336 first and retarget, so this pull request is the input hold's eight files rather than 29,687 authored lines.
- Let mouse and trackpad presses through during a hold, since FR35 keeps the mouse path, and add a mouse case.
- Hold a touch that is still down when the hold ends until it lifts, so its release cannot reach the page beneath.
- Stop held presses before the page's capture listeners and add
dblclickandauxclick, correct the'layer'scope's documentation, follow node replacement inuseInputHold, and update the changeset and test comments that still describe a touch gate.
[Automated review]
| // shield takes hits whatever its layer root opts out of (a full-viewport | ||
| // layer that is `pointer-events: none` and lets a backdrop child take the | ||
| // hits): inherited, they would let a held tap fall through to the page. | ||
| pointerEvents: 'auto', |
There was a problem hiding this comment.
This makes the shield take every hit whatever the pointer, and the gesture host's isInHeldLayer check (GestureHost.ts:1564-1569) empties participants for a mouse and a pen too, though its comment says only that a held layer "takes no touch". So a mouse or trackpad loses its presses while a layer is held, against spec:AST-094 FR35: "A mouse keeps today's behavior (DEC-16)." Deciding by each event's pointer, with a mouse case in the browser spec, would conform.
| if (current.viewport === 0 && current.layer === 0) { | ||
| if (current.shield != null) { | ||
| liveShields.delete(current.shield); | ||
| current.shield.remove(); |
There was a problem hiding this comment.
Removing the shield here while a finger is still down on it sends that touch's pointerup to whatever lies beneath and its touchend to the detached shield, where no document listener hears it end. FR35 says a held tap or drag must not "reach anything beneath it". Holding that touch to its end would close the gap.
| for (const type of SWALLOWED) { | ||
| shield.addEventListener(type, swallow, {capture: true, passive: false}); | ||
| } |
There was a problem hiding this comment.
These listeners sit on the shield itself, so window, document and ancestor capture listeners see a held press first, with the shield as its target (a resting swipe row closes, a touch Tooltip hides). dblclick and auxclick are also missing from SWALLOWED and bubble on. Either stop held presses earlier or state the real boundary in the header.
| import {useEffect, type RefObject} from 'react'; | ||
| import {parentAcrossShadow} from '../utils/flatTree'; | ||
|
|
||
| /** What a hold covers: the whole viewport, or the layer alone. */ |
There was a problem hiding this comment.
'layer' does not cover "the layer alone": both scopes add the same full-viewport shield (:196-198), and 'layer' also makes the content inert. A non-modal sheet, whose page FR35 keeps live, has no scope that fits.
| {isHeld, scope}: {isHeld: boolean; scope: InputHoldScope}, | ||
| ): void { | ||
| useEffect(() => { | ||
| const layer = ref.current; |
There was a problem hiding this comment.
ref.current is read once and the Effect depends on [ref, isHeld, scope], so a replaced or late-mounted layer node is never held (architecture:react-component-runtime INV6). useAttachment in this compare re-checks the node after every commit; the same shape here, with replacement and late-mount tests, would cover it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: one input hold for every layer (`Layer/inputHold`). While a layer presents, a touch, a press or a click anywhere reaches nothing (keys and assistive technology still act, and a pinch still zooms the page); while it dismisses, the same, and the layer's content is inert. The gesture host holds the layer's touches through its touch gates, and `isInputShield` lets a hit test see past a hold's shield. Not exported from the package entry point; no component uses it yet. |
There was a problem hiding this comment.
At this head the gesture host asks isInHeldLayer itself and the hold registers no touch gate, so "through its touch gates" describes a mechanism that is not there.
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.
…av (spec:AST-094 FR35)
Layer/inputHold holds a layer's pointer input while its presentation
enters or leaves: holdInput(layer, {until, scope}) raises a shield (scope
'viewport' while it enters; 'layer' while it leaves, its content inert),
decides by each event's pointer (a finger's or a pen's press is held to
its end; a mouse passes once it moves with no button down, DEC-16), never
cancels a touch's start, and cancels a one-finger move so a pinch still
zooms (FR10). The gesture host asks the hold for each touch (holdsTouch),
so the hold imports nothing from gestures/ and a component in the root
entry may use it statically (architecture:react-component-runtime INV13).
Its first users: Dialog and Drawer hold a finger's tap or drag during
their entrance (holdEntranceInput). MobileNav's drawer takes no finger
input while it slides in (a 'viewport' hold until the slide-in's
transition ends) or out (a 'layer' hold from the close request until the
dialog closes), and AppShell lets the slide-out play to its end.
Every browser proof lands its input at an exact point (an entrance held
where it is, the page's clock, or an exact step of a drag) and plays once.
From meta#8342 and its per-event offer, meta#8630 (Dialog and Drawer) and meta#8374
(MobileNav).
Co-authored-by: Cindy Zhang <cindyxz@meta.com>
There was a problem hiding this comment.
Approval waits on an engineering owner: MobileNav's close-time input hold (its drawer inert, focus blurred and every hit taken for up to 250 ms), the page held around a non-modal Drawer while it slides in, and Dialog's and Drawer's finger hold under a fine primary pointer go beyond what spec:AST-094 FR35 and FR13 state, so an engineering owner either accepts them by commenting /approve-spec <full head SHA> on this pull request (the author may, as an engineering owner) or has FR35, component:MobileNav and component:Drawer updated, or they are dropped. Independently of that decision, these still need changes:
- Land the five stacked pull requests (#8360, #8098, #8309, #8906 and #8763) first and rebase, so this compare is the input hold alone rather than 35,733 authored lines.
- A mouse press made before the mouse moves is swallowed during every Dialog and Drawer entrance, against FR35's "A mouse keeps today's behavior"; gating the hold on a coarse primary pointer, as MobileNav does, also avoids the 14 window listeners each entrance adds under a fine pointer (FR24).
.changeset/layer-input-hold.md, the'layer'scope's doc comment anduseInputHold's node handling need correcting (details inline).
[Automated review]
| if (state.shield == null) { | ||
| state.shield = addShield(layer); | ||
| // A presentation held while a mouse is the latest pointer heard lets it | ||
| // through from the start. | ||
| if ( | ||
| scope === 'viewport' && | ||
| latestPointer.get(layer.ownerDocument) === 'mouse' | ||
| ) { | ||
| state.shield.style.pointerEvents = 'none'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nothing is known about the pointer before the hold starts, so a presentation's shield takes every hit until the mouse moves with no button down, and holdsTouch (line 193) keeps a held target from every recognizer meanwhile. Dialog and Drawer call this on every entrance whatever the primary pointer, so a press where the mouse already rests (the second click of a quick double-click on an opener) lands on the shield and does nothing, against spec:AST-094 FR35's "A mouse keeps today's behavior". Holding only under a coarse primary pointer, as MobileNav does, and starting the shield lowered until a finger's or a pen's press is heard would keep the mouse path.
| export function holdEntranceInput(layer: Element): (() => void) | null { | ||
| if (typeof layer.getAnimations !== 'function') { | ||
| return null; | ||
| } | ||
| const entrance = layer.getAnimations().filter(isEntrance); | ||
| if (entrance.length === 0) { | ||
| return null; | ||
| } | ||
| return holdInput(layer, { | ||
| until: Promise.all(entrance.map(async animation => animation.finished)), | ||
| scope: 'viewport', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Under a fine primary pointer with no touch screen, each held entrance still installs the press hold's 14 window capture listeners for its length, outside any press (inputHold.ts:655-663). spec:AST-094 FR24 has a component under a fine pointer add document listeners only during a press, and its verification row fails an idle one beyond the shared press controller and stray-click swallower. MobileNav gates on TOUCH_QUERY (FR13); the same gate at the two callers would conform.
| import {useEffect, type RefObject} from 'react'; | ||
| import {parentAcrossShadow} from '../utils/flatTree'; | ||
|
|
||
| /** What a hold covers: the whole viewport, or the layer alone. */ |
There was a problem hiding this comment.
Both scopes get the same full-viewport shield (lines 321-322), and 'layer' also makes the content inert (lines 334-341), as the header says at line 34. "the layer alone" would lead a caller to pick 'layer' to keep the rest of the screen live; "the viewport shield plus inert content" matches the code.
| useEffect(() => { | ||
| const layer = ref.current; | ||
| if (!isHeld || layer == null) { | ||
| return; | ||
| } | ||
| // Released by the cleanup alone. | ||
| return holdInput(layer, { | ||
| until: new Promise<never>(() => {}), | ||
| scope, | ||
| }); | ||
| }, [ref, isHeld, scope]); |
There was a problem hiding this comment.
ref.current is read once and the deps do not change when the node behind the ref is replaced or mounts later, so the new node is never held and the detached one stays held (architecture:react-component-runtime INV6), and the Effect names no external system (INV3). Nothing calls this hook at this head; dropping it until an adopter needs it, or following the node per commit as gestures/useAttachment.ts does, would cover it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: one input hold for every layer (`Layer/inputHold`). While a layer presents, a touch, a press or a click anywhere reaches nothing (keys and assistive technology still act, and a pinch still zooms the page); while it dismisses, the same, and the layer's content is inert. The gesture host holds the layer's touches through its touch gates, and `isInputShield` lets a hit test see past a hold's shield. Not exported from the package entry point; Dialog's and Drawer's entrance hold uses it. |
There was a problem hiding this comment.
Two statements no longer match the code: a moving mouse passes a presentation's shield (inputHold.ts:488-501), so not every press or click "reaches nothing", and the gesture host asks holdsTouch (GestureHost.ts:1621) rather than holding through touch gates (inputHold.test.tsx:211 asserts nothing is registered). MobileNav uses the hold too.
Depends on #8763 and stacks on its head. Review only this PR's change: vjeux/astryx@a20b6c8...f41ae1e
This PR folds in #8630 (Dialog and Drawer) and #8374 (MobileNav), under the maintainers' section plan. Both close with a link here, and the commit credits their authors.
Why
On a phone, a tap or a drag can land while a Dialog, a Drawer or the MobileNav drawer is still entering. That touch reaches whatever is under the finger: a tap on the backdrop closes the dialog, a button runs mid-entrance, and a drag scrolls the page. The MobileNav drawer also takes taps while it slides in or out. UIKit delivers no touch until a presentation finishes.
What led to it
This is part of the touch-presentations work, which brings Astryx's touch behavior to iOS's (
spec:AST-094). FR35 asks every touch presentation to take no pointer input while it enters. Keys and assistive technology stay live, and so does a mouse (DEC-16). Each component that needed this had started to grow its own hold. The maintainers' section plan lands the shared hold together with its first users, in this one PR.Options and tradeoffs
Why this choice
Option 3.
Layer/inputHold(internal) addsholdInput(layer, {until, scope}):'viewport'holds input while a presentation enters.'layer'holds it while the presentation leaves, with the layer's content inert and focus moved out of it.holdsTouch). The hold itself imports nothing from the gesture modules, so a component in the root entry can import it statically (architecture:react-component-runtimeINV13).Its first users:
holdEntranceInput, which reads them withgetAnimations()). An AlertDialog enters through Dialog, so it is held too.How to check it
In Chrome's or Safari's phone emulation:
Evidence
check:repo, eslint and Prettier clean;pnpm test:ui16,903 passed; the Node tests 14,501 passed (2 expected failures); Storybook built; browser tests: the input hold, Dialog and Drawer entrance specs 54 of 54 in Chromium over two runs, and 23 in WebKit with 4 skipped (the native scroll and pinch cases WebKit's driver can't inject, and Chromium-only cases); MobileNav's touch-presentation spec 16 of 16 in Chromium over two runs.holdEntranceInputreturns null. For MobileNav, its holds are removed. For the hold itself, it stops following a touch to its end.