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.
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.
There was a problem hiding this comment.
Approval waits on an engineering owner: MobileNav's close-time input hold, the page held behind a non-modal Drawer while it slides in, Dialog's and Drawer's finger hold under a fine primary pointer, and the new text in component:Drawer FR5, component:BottomSheetSwitcher FR5 and component:MobileNav FR3 that arrives through #8098 go beyond what current records 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 the records updated, or the behaviours are dropped and the record edits left to #8098. Independently of that decision, these still need changes:
- Land the four stacked pull requests (#8360, #8098, #8309 and #8763) first and rebase, so this compare is the input hold alone rather than 23,749 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.
A presentation's shield starts out taking every hit unless a mouse moved during an earlier hold, and Dialog and Drawer call this on every entrance whatever the primary pointer. A mouse press made where the mouse already rests (a quick second click on an opener) lands here and does nothing, which DialogTouchEntrance.interaction.browser.spec.ts:322-377 asserts. spec:AST-094 FR35 says "A mouse keeps today's behavior" and its verification row fails "a changed mouse path". Gating the callers on a coarse primary pointer, as MobileNav does, would keep a mouse-first device untouched.
| for (const type of POINTER_EVENTS) { | ||
| view.addEventListener(type, onPointer, WINDOW_LISTENER); | ||
| } | ||
| for (const type of TOUCH_EVENTS) { | ||
| view.addEventListener(type, onTouch, WINDOW_LISTENER); | ||
| } | ||
| for (const type of PRESS_EVENTS) { | ||
| view.addEventListener(type, onPress, WINDOW_LISTENER); | ||
| } |
There was a problem hiding this comment.
These 14 window capture listeners go on at a document's first hold, for the length of every Dialog and Drawer entrance, even under a fine pointer with no press. spec:AST-094 FR24's verification row fails "an idle document listener outside a press other than the one shared press controller and the one stray-click swallower".
| 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' is not "the layer alone": both scopes add the same full-viewport shield (:321-322), and 'layer' also makes the content inert (:334-341). A caller that picks it to keep the rest of the screen live would block every tap. The header at :10 also says the gesture host asks isInHeldLayer; it asks holdsTouch.
| 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.
This reads ref.current once and depends on [ref, isHeld, scope], so a replaced node stays held and the new one is not (architecture:react-component-runtime INV6), and the Effect names no external system (INV3). Nothing calls useInputHold yet; dropping it until an adopter needs it, or following the node per commit as gestures/useAttachment.ts does, would avoid shipping the gap.
| '@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.
This misdescribes the code: a moving mouse passes during a presentation (inputHold.ts:489-502), the gesture host asks holdsTouch directly rather than through its touch gates (GestureHost.ts:1700; inputHold.test.tsx:211 asserts nothing is registered), and MobileNav uses the hold too.
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), its cases in the gesture browser proof (Chromium and WebKit) and 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 main after section 2 as one commit; the touch-action rule came with meta#8071. Where fingers come from their pointer events, a claimed finger's touchend is cancelled from its element (FR21), under a fine primary pointer wherever the browser sends Touch Events and inside an open shadow root too; at most eight such lifts wait for their touchend, and each goes once taken or after 500 ms, kept across the host's uninstall, which can run in the microtask after the pointerup, before the touchend that comes later in the same task (FR24). An armed press's page listeners go once the arena has decided its last touch, a tap that waits on a double tap included, and after its last detach the host holds no listener, note, frame or timer (FR24). 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); 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>
…finger landed On iOS WebKit the host resolves moving content at touch-down and asks the engine what is under the finger. When a busy main thread hands it the touch later, the content has moved on: the host now asks at the point where what was under the finger at the touch's timestamp is drawn, so a tap on a moving sheet's Done still clicks Done (spec:AST-094 FR11). A presented source reports its offset at a given time for this. The presented browser proof drives the sheet with Core's animator and a critically damped spring, up into place and down off the screen over the scrim: a tap on the drawn Done clicks it once, and a drag on the drawn sheet catches it where it is drawn, in both directions.
…the finger (FR11, FR19); INV13 names the shared input listener Where the engine hit-tests an element a compositor animation moves at the motion's start (iOS WebKit), a touch on that content goes to what was drawn under the finger at touch-down, and a tap there activates that element with one click while the browser's own click is cancelled or swallowed (FR11). While such content around a recognizer that claims moving touches is moving, the document may carry a non-passive touchmove listener that cancels only owned moves and comes off when the motion ends; while it is on, a native scroll waits for a free main thread (FR19). 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 (architecture:react-component-runtime INV13).
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 page's scroller 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, and MobileNav's clip moves onto them. 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. The records state the clip on the page's scroller (MobileNav FR3) and that pull-to-refresh and the page's bounce stop behind a modal overlay (BottomSheetSwitcher FR5, Drawer FR5). AlertDialog's layout tests stub the page hold, whose pointer read belongs to the lock, so their guard still sees only the layout's own media queries.
…h rule of the Touch Events path Where a finger comes from its pointer events, the host does what its Touch Events path does (spec:AST-094 FR10, FR19, FR28): the lift of a pen or a pointer-read finger the window's blur cancelled is refused, while a mouse's keeps its click, and a contact that ends in pointercancel is forgotten; a second finger fails an unclaimed one-finger pan wherever it lands; and a target gets a blocking listener only for an enabled participant. The arena cancels a moving recognizer the browser takes. A refused lift is noted as a claimed lift is, and the pointers a blur cancelled go, as the host's other notes do, after its last detach (FR24). The claimed lift's touchend cancel these build on comes with meta#8360. 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:
'viewport' over the whole viewport while a modal layer enters; 'surface'
over the layer's own box while a non-modal one enters, the page around it
live; 'layer' over the viewport, its content inert, while a layer leaves.
It 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 hold only under a coarse primary pointer, a touch
presentation (FR13): Dialog and Drawer hold a finger's tap or drag during
their entrance (holdEntranceInput), a non-modal Drawer only on its panel;
MobileNav's drawer takes no finger input while it slides in; AppShell lets
the slide-out play to its end. Under a fine primary pointer nothing is held
and nothing listens (FR24). Where the primary pointer is fine and the
screen also takes touch, a finger is not held yet; AST-094's current-state
impact and Drawer's record name that gap.
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>
|
Fixed in b42ae2c:
Each behavior change failed its new test first; the description lists the gates. |
There was a problem hiding this comment.
Approval waits on an engineering owner: the input hold's dismissal scope (scope: 'layer', which makes a layer's content inert, blurs its focus and shields every pointer) has no governing spec, and the stacked commits add new requirements to spec:AST-094 FR11 and FR19, architecture:react-component-runtime INV13 and the Drawer, BottomSheetSwitcher and MobileNav records; an engineering owner can provide or identify the governing spec, or accept them as filed by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). Whatever that decision, a few changes are still needed:
- Land #8360, #8098, #8309 and #8763 first and rebase, so this compare is the input hold alone (24,304 authored lines today).
- Have MobileNav call
holdEntranceInputinstead of its own copy, and route the hold's window listeners through the shared input stream or say why it cannot. - Keep a resting mouse's press during an entrance on a coarse-pointer device, as FR35's "A mouse keeps today's behavior" asks, or have FR35 amended.
- Say "on a phone or tablet" rather than "on a touch screen" in the changeset and the Dialog and Drawer docs, and fix the Chinese MobileNav text that still describes a close-time hold.
[Automated review]
| * open, so such a timer ends the hold before the slide-in does, and under | ||
| * load it can also run after it. | ||
| */ | ||
| async function slideInEnd(drawer: HTMLElement | null): Promise<unknown> { |
There was a problem hiding this comment.
This repeats what holdEntranceInput (Layer/holdEntranceInput.ts:51-72) does for Dialog and Drawer: read the coarse primary pointer, then hold the layer until its entrance animations end. The two already differ (this waits on every animation with a 550 ms cap; the helper keeps only running, finite ones and has no cap). Could the helper take the element whose entrance it reads and an optional cap, so MobileNav calls it once?
| * A finger's or a pen's press that lands anywhere while a layer is held is | ||
| * held with its moves, its release, its click and the compatibility mouse | ||
| * events a browser sends for it, to the end of that press, even when the | ||
| * hold ends first: the whole tap reaches nothing. Window listeners, the first |
There was a problem hiding this comment.
Every Astryx pressable installs press feedback on mount (utils/pressFeedback.ts:752-766), so on any page with one the shared input stream's pointer listeners are on the window before a layer is held, and window capture listeners run in registration order. The press feedback controller and the gesture host therefore hear a held press before these listeners swallow it (the stream dispatches to every subscriber whatever an earlier one stopped, utils/inputStream.ts:145-157). Subscribing the hold through the stream, with a way to run first and end the dispatch, would make this true and drop the second set of window listeners.
| state.shield = addShield(layer, state.isShieldFull); | ||
| // A presentation held while a mouse is the latest pointer heard lets it | ||
| // through from the start. | ||
| if ( |
There was a problem hiding this comment.
latestPointer is cleared when a document's last hold ends (:268-269), so on a coarse-primary device every new presentation starts with the shield taking hits, and a press where a mouse already rests lands on it (the case at DialogTouchEntrance.interaction.browser.spec.ts:394-402). spec:AST-094 FR35 says "A mouse keeps today's behavior", and its verification row fails a changed mouse path. A pointer reading from a source that already runs (the input stream's open pointer channels) would let the shield start lowered for a mouse.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] On a touch screen, a tap or a drag that lands while a `Dialog` or a `Drawer` is still entering no longer closes it, runs one of its actions, moves focus or scrolls the page: it reaches nothing until the entrance has played, and the dialog or drawer finishes entering and stays open. Around a `Drawer` without a scrim only a tap or a drag on the panel is held, and the page stays live. Keys and assistive technology stay live during the entrance, the content stays in the accessibility tree, a pinch still zooms the page, and a mouse or a trackpad still clicks. |
There was a problem hiding this comment.
"On a touch screen" also covers a touchscreen laptop, whose finger is not held (DialogTouchEntrance.test.tsx:135), and "a mouse or a trackpad still clicks" misses the first press of a resting mouse on a phone or tablet. Suggest "On a phone or tablet (a coarse primary pointer)" and "still clicks once it has moved".
| type: '(isOpen: boolean) => void', | ||
| description: | ||
| '当抽屉可见性变化时调用（点击背景遮罩、按 Escape 键或点击关闭按钮）。', | ||
| '当抽屉可见性变化时调用（点击背景遮罩、按 Escape 键或点击关闭按钮）。使用手指操作时，抽屉滑入期间不接受输入；关闭后、对话框关闭之前也不接受输入，随后焦点回到打开它的元素。', |
There was a problem hiding this comment.
This Chinese text says the drawer also takes no input after a close until the dialog closes, but a close holds nothing at this head (MobileNav.tsx:643-645, MobileNavTouchPresentation.test.tsx:185); the English text only says focus returns once the dialog closes.
Depends on #8763 and stacks on its head. Review only this PR's change: vjeux/astryx@dbe81cd...b42ae2c
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. 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), and around a non-modal presentation only input on the presentation itself is held. 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'shields the whole viewport while a modal layer enters.'surface'shields only the layer's own box while a non-modal layer enters, so the page around it stays live.'layer'shields the viewport while a layer 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 hold only under a coarse primary pointer, the condition of a touch presentation (FR13). Under a fine primary pointer nothing is held and nothing listens (FR24), so a mouse keeps today's behavior, including a press where it already rests.
holdEntranceInput, which reads them withgetAnimations()). A Drawer without a scrim holds only its panel. An AlertDialog enters through Dialog, so it is held too.Known gap: where the primary pointer is fine and the screen also takes touch (a touchscreen laptop), a finger isn't held during an entrance yet.
spec:AST-094's current-state impact and Drawer's record state it.How to check it
In Chrome's or Safari's phone emulation:
Evidence
check:repo,check:knowledge, eslint and Prettier clean;pnpm test:ui17,434 passed; the Node tests 14,688 passed (2 expected failures); Storybook built; browser tests: the input hold, Dialog and Drawer entrance specs 58 of 58 in Chromium over two runs and 25 in WebKit with 4 skipped (the native scroll and pinch cases WebKit's driver can't inject, and Chromium-only cases), and MobileNav's touch-presentation spec 14 of 14 in Chromium over two runs; under stress (Chromium pinned to 2 cores with 4 workers, and WebKit in a 3-CPU container with 16 workers, twice), all of them again.Writing rubric results
Records touched:
component:Drawer(Current-state impact: the entrance-hold gap, now only the fine-primary case) andspec:AST-094(Current-state impact: one sentence on Dialog's, Drawer's and MobileNav's fine-primary gap).