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:spec-owner-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.
This pull request targets main, so it currently carries its whole open stack (153 files, about 28,600 changed lines) rather than just the exit-copy commit, and approval waits on two things. First, the head adds draft requirements that main does not state (spec:AST-094 FR1–FR23 with DEC-1/DEC-2, the touchOpen: 'press' clause and DEC-8 in useMenuPress.spec.md, and the press-open sentence of DropdownMenu.spec.md FR8) and proposes amending current records (theme-tokens INV3, user-states DR1/DR3, the buttons press treatment and AST-025 FR14), so an engineering owner needs to approve or reject them by commenting /approve-spec followed by the full head SHA on this pull request; the author may do so if they are an engineering owner. Second, please land the stack first and rebase so this pull request contains only the exit-copy commit, bring useMenuPress.spec.md in line with the shipped 150 ms hold-open, 10 px handoff and continueGesture (it still says 500 ms and the settle rule), and add a nested sub-menu case for the new outside-pan dismissal.
[Automated review]
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still gives the long-press delay as 500 ms, but hooks/useMenuPress.ts:90 now defaults to menu.holdOpenMs (150 ms, plus 150 ms inside a scrolling region). Please update the row to what the code ships.
There was a problem hiding this comment.
This record comes from the DropdownMenu press change below this one in the stack, and that change is being brought in line with the code (the 150 ms hold-open and continueGesture). Nothing to change in this pull request.
| | FR7 | While a press is tracked in a menu that scrolls, a pointer resting within the edge zone of the menu's top or bottom MUST scroll the menu toward that edge and re-read the row under the pointer as the rows move. | `useMenuPress.test.tsx` autoscroll case | verify | | ||
| | FR8 | On a mouse a DropdownMenu trigger MUST open its menu on press-down (button 0, no Control), and a drag from the trigger into the menu that releases over an enabled row MUST act on it. The opening gesture's release MUST act only if the pointer entered the menu or the press lasted the settle time; otherwise nothing acts and the menu stays open. Pressing the trigger of an open menu MUST close it and MUST NOT reopen it in the same gesture. | Proposed DEC-4; `DropdownMenu.test.tsx` press model suite, `menuPressGesture.test.ts` trigger states | proposed; pending owner review | | ||
| | FR9 | Under a finger a tap on the trigger MUST open the menu through its click. A press held on the trigger for the long-press delay MUST open the menu with the finger down, prevent the default of the finger's later `touchmove`s until the gesture ends, and continue under FR1–FR4 with the same settle rule. | Proposed DEC-4; `DropdownMenu.test.tsx` held-touch case, `useMenuPress.test.tsx` | proposed; pending owner review | | ||
| | FR9 | By default a finger's tap on the trigger MUST open the menu through its click, and a press held for the long-press delay MUST open it with the finger down and prevent the default of the finger's later `touchmove`s until the gesture ends. With touch open `press` the press-down MUST open it at once, and a cancel before the pointer enters the menu MUST close it again. Both continue under FR1–FR4 with the same settle rule. | Proposed DEC-4, DEC-8; `DropdownMenu.test.tsx` touch cases, `useMenuPress.test.tsx` | proposed; pending owner review | |
There was a problem hiding this comment.
FR9 says both modes continue “with the same settle rule”, but a finger now picks only after MENU_PRESS_HANDOFF_PX (10 px) of travel (hooks/menuPressGesture.ts:57), so a held finger released over a row before 10 px acts on nothing. Stating that here, and listing continueGesture in the public-contract row, would keep the record in line with the code.
There was a problem hiding this comment.
This record comes from the DropdownMenu press change below this one in the stack, and that change is being brought in line with the code (the 150 ms hold-open and continueGesture). Nothing to change in this pull request.
| popover.hide(); | ||
| }, [popover]); | ||
|
|
||
| // A pan that starts outside the open menu, in its touch presentation, |
There was a problem hiding this comment.
This outside-pan dismissal is exercised only with the root menu open. The layer-runtime architecture's change coupling asks a local outside or touch change to verify nested surfaces: could you add a case with an open sub-menu flyout (and a header/footer sub-menu dialog), for a pan that starts outside the menu and one that starts inside the sub-menu?
There was a problem hiding this comment.
This dismissal comes from the DropdownMenu change below this one in the stack. The rule now lives in the layer runtime's usePanOutsideDismiss (#8198), which adds the nested cases: a pan inside a parent popover but outside its child closes only the child, and a pan in a dialog outside its popover closes only the popover. This pull request only adds the exit copy.
PR Analysis Report
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | View full report |
05b28df to
90f052b
Compare
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
removed
the
needs:spec-owner-review
90f052b to
d8da416
Compare
vjeux
changed the title
There was a problem hiding this comment.
This pull request targets main, so it carries its whole open stack (194 files, about 31,000 changed lines) rather than only the exit-copy commit, and approval waits on an engineering owner's decision about the draft spec:AST-094 (FR1–FR23, DEC-1, DEC-2), its proposed amendments to current records, and the current claim that should govern the layer runtime's exit copy. An engineering owner records that decision by commenting /approve-spec followed by the full head SHA on this pull request, and the author may do so if they are an engineering owner; otherwise the record is settled in its own pull request. Independently of that decision, please:
- land the stack first and rebase so this pull request contains only the exit-copy commit;
- measure the copy's placement without the transform it copies from running animations, since in Chromium a layer caught at
scale: 0.8 0.6gets a copy 32 px away; - remove
nameandformfrom cloned form controls, since inserting the copy unchecks the layer's checked radio for good; - replace the
position: fixedandz-index: 2147483647fallback, which current AST-027 FR1 and FR8 rule out, and add real-browser evidence for the copy as AST-013 FR10 requires; - fix the changesets that name the refused role
timing.reducedMotionCrossFadeMsand contradict each other on Reduce Motion.
[Automated review]
| const place = () => { | ||
| style.setProperty('left', `${box.left}px`); | ||
| style.setProperty('top', `${box.top}px`); | ||
| const landed = copy.getBoundingClientRect(); |
There was a problem hiding this comment.
This measures the copy after commitPresented has written the running curve's scale/translate/transform, so the correction cancels the transform's offset too. In Chromium, a layer at left 100 caught at scale: 0.8 0.6 about 160px 0 stands at 132, and its copy is placed at left: 68px and shows at 100, 32 px away. Measuring before commitPresented (or with the transform properties set to none) keeps the copy where the layer stands.
| if (topLayer) { | ||
| copy.setAttribute('popover', 'manual'); | ||
| } | ||
| element.after(copy); |
There was a problem hiding this comment.
Inserting the deep clone here puts a checked radio with the same name and form into the document, which unchecks the original radio in the layer, and removing the copy later does not restore it (checked before, unchecked after, in Chromium). Removing name and form from the cloned form controls before this line avoids it.
| ); | ||
| style.setProperty('overflow', 'visible'); | ||
| // On top of the stacking order until it is in the top layer. | ||
| style.setProperty('z-index', '2147483647'); |
There was a problem hiding this comment.
A page-level z-index on a fixed box is what AST-027 FR1 and FR8 rule out (“a fixed position, or a larger number is not a substitute” for the top layer). Where the popover API is missing the copy keeps this for the whole close; could the fallback follow the layer runtime's reduced path, or skip the animated close?
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion roles. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a measured touch motion value (`springs.sheetPresent`, `timing.reducedMotionCrossFadeMs`), at the root or in an adaptation rule, and writes one `--motion-*` variable per role it sets. A role the theme leaves out keeps its measured default, an unknown role id is refused, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. No shipped component reads the roles yet. Adaptation rules gain a `platform` condition: `ios` for the browsers of iPhone and iPad, or `other`. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
timing.reducedMotionCrossFadeMs is not a role on this head: expandMotionRoles throws “is not a motion role”, and reduced-motion-record.md says it is no longer a theme role. A different example role would keep the release note accurate.
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/src/busy.test.ts # internal/interaction-harness/src/busy.ts # internal/interaction-harness/src/frames.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts # packages/core/src/utils/pressFeedback.ts
meta#8076's new head carries main 70ea859 with meta#8073's writer of the press attribute on main's press controller, and its README names both writers. utils/pressFeedback.ts keeps that controller with this line's own changes on it: the pointer, drag, scroll and visibility listeners through the shared input stream (meta#8356) and touchcancel only while the primary pointer is coarse (meta#8270); the key path stays at the window's bubble phase, so a Space press the page cancels paints nothing. usePressTracker stays in its own module, one tracker for a component's life (meta#8315), so meta#8073's rebuilt-tracker hook and its test are not taken. The harness files are meta#8076's.
…s, and the action-group tracker writes its own The header and writePress called it the one writer of the press attribute, but main's action-group tracker writes its groups' presses itself. They now say what holds, as meta#8076's gestures README does.
…he press attribute The release note said every press painter writes data-astryx-press through one internal writer, but the action-group tracker (hooks/actionGroupTracker.ts) writes its groups' presses itself, as the gestures README and utils/pressFeedback.ts already say. The note now says paintPress writes every press it tracks and the action-group tracker writes its own.
…llowance under every primary pointer The useMenuPress record's FR9 puts the trigger's held finger under spec:AST-094 FR29 under every primary pointer (FR43), so a finger on a touchscreen laptop may drift 13 pt before the open, as on a phone. The trigger read (pointer: coarse) as its press started and took the long press's 10 px where the pointer was fine. Three cases fail until it does not: a 12 pt drift opens under a fine pointer too, a (12, 12) settle opens there, and nothing reads a pointer query, the trigger's press included.
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
Strict ESLint reported six unused bindings in files this pull request carries: the host's HAPTIC_SWITCH_ATTRIBUTE import, two unused imports in the pan's tests (scrollProbe, ScreenEdgePanGestureRecognizer), the swipe test's drift, the presented-hit-testing test's gestures, and a GestureHost test's before, which now asserts what it was read for: a second finger's moves leave the pan where it was.
…eal angular speed The release velocity of a rotation whose listener sets rotation = 0 on each change, at 2 degrees every 16 ms, read 4.363 rad/s against the 2.182 rad/s of the same stream without a re-base: the test asserted only more than 1.5. It now asserts the 2.18 rad/s (spec:AST-100 FR2), and fails.
The rotation's velocity tracker samples the raw unwrapped angle, which setting rotation does not move, so the setter's shift of the samples read each re-base as extra turn and doubled the release velocity of a listener that re-bases on every change. The setter no longer touches the tracker.
…mates while it runs The proof read the element's animations 230 ms in, after the dip had ended, and asserted only that every animated key is opacity, which an empty list passes: with the cross-fade removed from the plan (the move still landing), the proof still passed. It now reads the keyframe keys right after the call starts and asserts they include opacity and nothing else; the same mutant fails it. Chromium and WebKit pass. (F1's in-flight read from c648034.)
…roller, and its claimed moves are cancelled under pointer events Two comparisons stop at the shadow boundary, each failing a case: - once the host's listener on a recognizer's inner node has seen the finger, isFingerDownIn reads it as outside a light-DOM scroller around that root, so a scroll under it is not the finger's (FR20); - under inputMode 'pointer-events', a claimed pan on a shadow host cancels none of its finger's moves over the inner content, since the finger's target is the inner node and its touches reach the host retargeted (FR19).
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
There was a problem hiding this comment.
This pull request is on hold for an engineering owner's decision on the spec text it adds to architecture:layer-runtime (the Exit copy section, INV13, its verification row, the exit-copy change-coupling bullet, the owning-code entry, INV13 in the layering triggers and the reworded Current gaps sentence): an engineering owner records that decision by commenting /approve-spec followed by the full head SHA, and the author may do so as an engineering owner. Whatever that decision, the branch needs these changes before it can merge:
- Land #8360 first and rebase, so this pull request carries only the exit copy (28,216 authored lines today, 966 of them the exit copy).
- Drop the
spec:AST-057edit: the swipe row still settles on a literal 200 ms (packages/core/src/Item/useSwipeAction.ts:86), so the new sentence describes motion the row does not play. - Make the copy match its record: keep a root-scrolling layer's overflow and scroll position (
layerExitCopy.ts:280,:283-291), give form-associated custom elementsform=""(:128-129), and stop descendants' keyframes from restarting (:279). - In #8360, let the gesture host attach recognizers for every pointer, as current
spec:AST-094FR19 and FR43 require (GestureHost.ts:12-22).
[Automated review]
| style.setProperty('align-self', 'normal'); | ||
| style.setProperty('pointer-events', 'none'); | ||
| style.setProperty('animation-name', 'none'); | ||
| style.setProperty('overflow', 'visible'); |
There was a problem hiding this comment.
This inline overflow: visible beats the runtime's [data-astryx-layer-scrolls]{overflow-y:auto} (Layer/layerSlideRules.ts:245), which useLayer sets on the layer element itself when its content overflows (Layer/useLayer.tsx:645-648), and the scroll capture below (:283-291) walks only descendants. A copy of such a layer starts the close from the top of the list and spills below the layer's box, against the Exit copy section's "it keeps the layer's scroll positions". Copy the live root's computed overflow-x/overflow-y and capture the root's own scroll with its descendants' (as motion/transientCopy.ts:162-169 does), with a test that scrolls the layer itself.
| const FORM_CONTROLS = | ||
| 'button, fieldset, input, object, output, select, textarea'; |
There was a problem hiding this comment.
form="" reaches only these native controls, so a form-associated custom element in the copy still takes an ancestor <form> as its owner (the copy stands right after the layer), counts in form.elements and can make the form invalid while the close plays. INV13 says the copy has no form owner: also set form="" on elements whose definition is form-associated (customElements.get(node.localName)?.formAssociated), with a test, or narrow INV13 and the Exit copy section to native controls.
| style.setProperty('justify-self', 'normal'); | ||
| style.setProperty('align-self', 'normal'); | ||
| style.setProperty('pointer-events', 'none'); | ||
| style.setProperty('animation-name', 'none'); |
There was a problem hiding this comment.
Only the root stops its animations. Every cloned descendant keeps its classes, so its stylesheet keyframes start again from the first frame when the copy is shown (the press release on [data-astryx-press="fading"], fill mode both, included), and an animation outranks the inline values commitPresented writes. Set animation-name: none on every element of the copy, with a test for a descendant keyframe animation.
| clocks). The row's slide and spring motion plays on theme motion roles | ||
| (`spec:AST-094` FR4). |
There was a problem hiding this comment.
Item still settles on the literal SWIPE_SETTLE_MS = 200 (packages/core/src/Item/useSwipeAction.ts:86, :368), which this branch leaves unchanged, so this sentence describes a motion source the row does not read. Drop this edit and the matching one at lines 223-226 so the record reads as it does on main, and make the change together with the row's code if it is wanted.
| * ONLY WHERE THE PRIMARY POINTER IS COARSE | ||
| * | ||
| * - Recognizers attach, and the host registers listeners, only while the | ||
| * primary pointer is coarse (`spec:AST-094` FR19, FR24; coarsePointer.ts). | ||
| * Where it is fine, an attach is recorded but nothing is installed: no | ||
| * document, window or element listener, no `touch-action` or other style, | ||
| * nothing delivered, so a finger on a touchscreen laptop gets the | ||
| * browser's defaults. The host follows the query: turning coarse installs | ||
| * it and styles every registration; turning fine cancels every touch in | ||
| * flight and removes it all. A development warning names the first attach | ||
| * under a fine pointer. |
There was a problem hiding this comment.
Current spec:AST-094 FR19 says "Recognizers attach for every pointer, with FR43's exceptions", and FR43 "Gestures attach for every pointer, a mouse, a pen, and a finger, not only a coarse primary pointer". This host installs nothing where the primary pointer is fine and cancels every touch when it turns fine (:1339-1351). Bring it in line in #8360, keeping only FR43's listed exceptions pointer-specific.
Depends on #8076. Review only this PR's change: vjeux/astryx@3fb31dd...1aaf2db (its latest push only merges main).
Why
A touch presentation cannot animate a popover closing: the browser hides a popover the moment it closes (a pick, Escape, a light dismiss), so there is nothing left on screen to animate.
What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do (sheets, menus, gestures and their motion). DropdownMenu's touch menu (#8080) works around this with a private copy of its panel. Popover (#8187) needs the same, and each copy has to solve the same problems: no duplicate ids or form state, no
z-indexfight, and the right position when the close interrupts an entrance.Options and tradeoffs
Why this choice
The shared helper lets the real popover close at once for focus and assistive technology, while an inert, id-free copy plays the motion in the top layer. It is recorded as
architecture:layer-runtimeINV13, "One exit copy per closing layer", which needs an engineering owner's approval.How to check it
pnpm test:interaction layerExitCopy.interaction.browser.spec.ts(Chromium, and WebKit throughinternal/interaction-harness/scripts/webkit-container.mjs).z-index.No component calls it yet: Popover (#8187) is first.
Evidence
Writing rubric results
R1 narration, R2 research, R4 rejected alternatives and R5 change references: none. R3 evidence: INV13's row names the two test files. R6 open question, for an engineering owner: approve INV13.
Scope