Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
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 needs an engineering owner's decision before it can be approved: every change here implements the draft spec:AST-094 (FR1–FR3 and FR12), which no owner has approved yet, and that record would also narrow architecture:theme-tokens and amend design:user-states DR1/DR3, family:buttons and spec:AST-025 FR14 for touch presentations. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, a few changes are needed:
- Land #8052–#8054 first and rebase, so this pull request carries only its 585-line ContextMenu commit instead of 4,639 lines.
- AST-094 FR7 and its Current-state impact cite
spec:AST-090, which does not exist onmain, so drop that citation until the record lands. - Add the
## Writing rubric resultsto this description (or land #8052 first), give the projection tie test an exact tie so it fails without the tie rule, and bring themotion/uikitheader comments and the switch line inMEASUREMENTS.mdin line with the files at this head.
[Automated review]
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 is not on main (it is only proposed in #8003), so this citation, and the one in Current-state impact, points at nothing an owner can read. Drop it until that record lands, keeping FR7’s own requirement.
| it('breaks an exact tie toward the direction of travel', () => { | ||
| expect( | ||
| selectSnapPoint([0, 100], {position: 50, velocity: 0.0001, rate: 0.5}) | ||
| .value, | ||
| ).toBe(100); | ||
| expect( | ||
| selectSnapPoint([0, 100], {position: 50, velocity: -0.0001, rate: 0.5}) | ||
| .value, | ||
| ).toBe(0); | ||
| }); |
There was a problem hiding this comment.
With ±0.0001 units/s the projection lands at 50.0000001, so 100 is simply nearer and the tie branch never runs: deleting || (isTie && isAhead) leaves all 11 projection tests passing. A velocity of ±1e-30 makes it an exact tie within the 1e-9 tolerance, so the test then depends on the rule.
| * sources.test.ts enforces both. | ||
| * | ||
| * Most interaction springs below are GUESSES today: UIKit does not document | ||
| * the springs of its own transitions, and the simulator measurements that | ||
| * will replace them (method in motion/README.md) have not run yet. Each | ||
| * guess starts from the nearest sourced spring and names it, so a component | ||
| * built on the table gets the right value the moment the measurement lands, | ||
| * without changing. |
There was a problem hiding this comment.
This header predates the measurements: only segmentedThumb and banner are GUESS among the 31 entries below, and the enforcing test is uikit/uikit.test.ts, not sources.test.ts (the same note is in scroll.ts and timing.ts).
| where a switch outside any scroll view is needed. | ||
|
|
||
| Done (iOS 26.2 simulator, Oct 23 2026; in the switch's own table, | ||
| `uikit/switch.ts`): M-SW02, M-SW03, M-SW04, M-SW05, M-SW06, M-SW09 (UISwitch |
There was a problem hiding this comment.
uikit/switch.ts does not exist at this head, so this records switch captures as landed in a table the repository does not have.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
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 | Storybook | Sandbox | View full report |
85e2d19 to
cae55cf
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: every change here rests on the draft spec:AST-094, and the new defineTheme({motion}) roles and platform adaptation condition contradict that record's own FR4 and DEC-1 (touch motion is not themable) as well as the current architecture:theme-authoring-contract ("An adaptation change preserves the fixed condition vocabulary") and architecture:theme-tokens INV1. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request once the records state the chosen direction, and the author may do so as an engineering owner. Independently of that decision, a few changes are needed:
- Land #8045, #8053, #8054, #8257 and #8258 first and rebase, so this pull request carries only its 702-line commit instead of about 15,200 authored lines.
- Keep the "Touch motion in a theme" docs and the
[feat]changeset out until a component reads the motion roles, and makedefineThemereject unknown or wrong-kind role ids without relying on the motion tables being imported first. - Drop the
spec:AST-090citation from AST-094, add## Writing rubric resultsto the description, make the projection tie test and the press curve's theme-spring test fail without the behavior they cover, and bring themotion/uikitheaders in line with this head.
[Automated review]
| `${location}: a motion role is named by its table path, such as "sheet.present" or "springs.navigationPush".`, | ||
| ); | ||
| } | ||
| const registered = motionRole(id); |
There was a problem hiding this comment.
This check runs only for a registered role, and nothing in defineTheme's import graph imports the motion tables, so in a consumer's theme the registry is empty: motion: {durations: {'springs.sheetPresent': 300}} is written as --motion-springs-sheet-present: 300ms, and a misspelt id writes an orphan variable. The refusal test passes only because expandMotionRoles.test.ts imports '../motion/uikit' first. Validating against a role list the theme module owns would make the documented refusal hold for consumers.
| content: [ | ||
| { | ||
| type: 'prose', | ||
| text: 'On a touch screen, sheets, alerts, menus, navigation and scrolling play the springs, durations and curves measured from iOS. Each value is a motion role named by its table path, such as `springs.sheetPresent` or `timing.curves.easeInOut`. A theme retunes a role through `motion.springs`, `motion.durations` or `motion.curves`, at its root or in an adaptation rule; a role it leaves alone keeps its measured default.', |
There was a problem hiding this comment.
No component plays these springs yet (nothing imports the animator or resolveMotion), so a theme author who follows this section sees no change. AST-094 says consumer docs wait until a component ships its touch presentation; could this section wait too, or say plainly that no component reads the roles yet?
| }); | ||
| }); | ||
|
|
||
| it('takes a theme’s spring and latency; the onset, the commit and the menu’s moment are not roles', () => { |
There was a problem hiding this comment.
With pressFeedbackCurve changed to play the table's spring instead of motion.spring, this test still passes: curve.at(0) === 1 and curve.at(0.3) > 1 hold for any spring. Comparing the themed curve with the table's at a fixed time (say 0.2 s) would make it fail without the behavior it names.
cae55cf to
c325a79
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: nearly every change here rests on the draft spec:AST-094, and the new defineTheme({motion}) roles contradict that record's own FR4 and DEC-1 as well as the current architecture:theme-tokens INV1. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request once the records state the chosen direction, and the author may do so as an engineering owner. Independently of that decision, a few changes are needed:
- Land #8076 and the pull requests it merges first, then rebase so this pull request carries only its 712-line ContextMenu commit instead of about 30,900 authored lines.
- Align AST-094 FR8 with the current
design:reduced-motionDR2 and DR4 (or mark it as a proposed amendment of that record), and drop thespec:AST-090citations. - Make the adaptive presentation honor a theme's
mdas its release note says (or say 768 px), keep themotion/reducedMotion.stylexsubpath behind the experimental boundary or give it a real record and release note, and drop theplatformaxis the theme template still documents. - Add
## Writing rubric resultsto the description, make the projection tie and press-curve theme-spring tests fail without their behavior, register the context-menu table insidepressFeedbackMotion, and run the interaction proofs inside the existingtestowners instead of a separately routed job.
[Automated review]
| on: a transition that travels becomes a cross-fade carrying the same change | ||
| of state. An entrance lands in place and fades in, an exit fades out in | ||
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen |
There was a problem hiding this comment.
A 0.5 s cross-fade here, and "UIKit's Reduce Motion spring" for menus two lines down, conflict with the current design:reduced-motion DR2 ("opacity only", "200 ms or less" on a coarse pointer) and DR4 (no springs or longer animation under reduced motion), which this paragraph does not name; the head's own Reduce Motion fade is 200 ms (motion/reducedMotion.stylex.ts:24). Could FR8 defer to DR1–DR4, or say it proposes amending that record?
| export const COMPACT_TOUCH_QUERY = | ||
| DEFAULT_COMPACT_DEFINITION.coarsePointerQuery; |
There was a problem hiding this comment.
This is the default theme's definition, so a theme that overrides md (for example md: 900) still gets the 768 px edge, while the touch-presentation changeset tells consumers the sheet follows "the theme's md point" and spec:AST-012 FR9 says "the effective md point". Either build the query from the active theme's width points (compactDefinition(points) takes them), or have the release note say 768 px.
| @@ -177,6 +190,13 @@ const STATIC_EXPORTS = { | |||
| types: './dist/utils/interactionOverlay.stylex.d.ts', | |||
| default: './dist/utils/interactionOverlay.stylex.js', | |||
| }, | |||
| // Proposed; pending owner review (spec:AST-094 OQ4): the Reduce Motion | |||
There was a problem hiding this comment.
AST-094 at this head has only OQ1 and OQ2, so this justification points at nothing. Since the subpath sits outside any experimental boundary, it becomes stable API on its first release (spec:AST-017 FR21), and the [fix] changeset never names it; should it go behind the experimental boundary until an owner approves it?
| element: Element | null | undefined, | ||
| ): PressFeedbackMotion { | ||
| return { | ||
| spring: resolveMotion(element, contextMenu.press.feedbackSpring), |
There was a problem hiding this comment.
resolveMotion returns the table's value for a role whose table is not registered, and nothing here calls registerContextMenuTable() (the test registers it itself), so a caller gets UIKit's spring whatever the theme sets. Calling the idempotent registerContextMenuTable() at the top of pressFeedbackMotion would make this function's doc hold on its own.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This job routes itself (its own path list includes packages/[^/]+/src/) and is not in the test join's needs, so a failing interaction proof on a core component never fails test. spec:AST-030 FR11 says new coverage inside an existing surface "MUST join those owners and MUST NOT create a separately routed lane", and FR7 says "Any owned lane failure MUST fail its join". Could the proofs run under the existing test owner, or the job join test?
| * 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.
At this head defineTheme has no platform condition (CONDITION_AXES in packages/core/src/theme/themeAdaptations.ts is width, pointer, contrast and motion, and an unknown when key is refused), so a theme author who follows this line written by astryx init --features theme gets an error. Drop the platform clause?
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the ContextMenu motion roles and the defineTheme({motion: {springs, durations, curves}}) field they rest on have no current spec now that spec:AST-094 is no longer on this branch (it is proposed in #8378), and the field's --motion-<role> tokens contradict architecture:theme-tokens INV1. An engineering owner needs to provide the governing record on main (for example AST-094 made current, with INV1 amended to admit motion-role tokens), identify another current record that covers these additions, or direct that the roles come out; an engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision, a few changes are needed:
- Land #8076 and the pull requests it merges first, then rebase so this pull request carries only its 712-line ContextMenu commit instead of about 30,200 authored lines.
- Run the interaction proofs inside the existing UI test owner so a failing proof fails
test, asspec:AST-030FR11 and FR7 require. - Give the new
astryxStylexundefinedVariablesoption a[feat]changeset, rewrite the reduced-motion changeset as an internal note, and describe the adaptive presentation behavior change in the description. - Register the context-menu table inside
pressFeedbackMotion, start the visible curve from the themed latency, and make the press-curve theme test and the projection tie test fail without their behavior.
[Automated review]
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This job decides its own scope from its own path list (line 3850 includes packages/[^/]+/src/ and apps/storybook/) and is not in the test join's needs, so a failing interaction proof on a core component never fails test. spec:AST-030 FR11 says new coverage inside an existing surface "MUST join those owners and MUST NOT create a separately routed lane", and FR7 says "Any owned lane failure MUST fail its join". Running the proofs as steps of the existing UI test owner, scoped by check-scope, would satisfy both.
| element: Element | null | undefined, | ||
| ): PressFeedbackMotion { | ||
| return { | ||
| spring: resolveMotion(element, contextMenu.press.feedbackSpring), |
There was a problem hiding this comment.
resolveMotion returns the table's value for a table nothing has registered, and nothing in this file calls registerContextMenuTable() (the test registers it itself), so a caller that follows this function's doc gets UIKit's spring whatever the theme sets. Since registration is idempotent, calling it here would make the documented contract hold.
| } | ||
|
|
||
| /** When the visible curve starts, ms after the finger landed. */ | ||
| export const PRESS_FEEDBACK_START_MS = feedbackOnsetMs + feedbackLatencyMs; |
There was a problem hiding this comment.
This start is the table's onset plus the table's latency, and pressFeedbackCurve never reads motion.latencyMs, so a theme that retunes contextMenu.press.feedbackLatencyMs changes what pressFeedbackMotion reports but not when the copy starts moving. Deriving the start from the motion passed in would let the themed latency take effect.
| const curve = pressFeedbackCurve({motion}); | ||
| expect(curve.at(0)).toBe(1); | ||
| expect(curve.at(0.3)).toBeGreaterThan(1); |
There was a problem hiding this comment.
These two assertions also hold if pressFeedbackCurve ignores motion.spring and plays the table's spring, so the test cannot fail when the theme stops driving the curve. Comparing the themed curve with pressFeedbackCurve() at a fixed time (for example 0.3 s) would make it fail without the behavior.
|
|
||
| it('breaks an exact tie toward the direction of travel', () => { | ||
| expect( | ||
| selectSnapPoint([0, 100], {position: 50, velocity: 0.0001, rate: 0.5}) |
There was a problem hiding this comment.
At ±0.0001 points per second the projection lands 1e-7 past 50, more than the 1e-9 tie tolerance in selectSnapPoint, so the nearer point wins and the tie branch never runs: removing || (isTie && isAhead) leaves this test green. A velocity whose offset falls inside the tolerance, such as ±1e-30, would exercise the tie.
| '@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.
At this head the animator is internal and has never been released or marked experimental, so this [fix] would tell readers of the changelog about an "experimental touch animator" and removed reducedMotion and crossfade options that never shipped. An internal [chore] note, like the other touch changesets, would describe the change.
| '@astryxdesign/build': patch | ||
| --- | ||
|
|
||
| [fix] `astryxStylex` resolves `defineConsts` values read from another file. With the `{stylexOptions}` form (the one Storybook uses), a file that read only constants from a `.stylex.ts` module lost that import once StyleX compiled it, so the constants never reached the CSS: a constant used as a media-query key failed the build ("Invalid empty selector") and one used as a value shipped as a custom property nothing declares. A constant defined on the library side of the layer split and read on the product side stayed unresolved in either form. A production build now also warns, naming the variable and the rule that reads it, when its stylesheet reads a StyleX-generated custom property that nothing declares; set `undefinedVariables: 'error'` to fail the build instead. |
There was a problem hiding this comment.
Besides the defineConsts fix, this adds the undefinedVariables option, a warning every production build now runs, and the undefinedGeneratedVariables export. spec:AST-017 FR7 puts compatible capabilities under [feat], so the option and the warning would read correctly as their own [feat] changeset.
…y pointer is coarse and a recognizer is enabled From CodeBunny on meta#8073 (spec:AST-094 FR19), fixed once here: the host wrote its managed touch-action on every device. It now writes it only while the primary pointer is coarse and an attached recognizer is enabled, and restores the element's own value when either stops being true; with the default coarse-pointer read it follows the media query's change too. The test observes no touch-action write under a fine pointer, the write once coarse, its removal while the recognizer is disabled and again when the pointer turns fine.
Inside a trace, waitForAnimation and watch.started() read the element through handles, which find no element rather than wait for one, so a wait on an element the page removed rejected with "no animation started on the element" instead of "the element is not in the document", unlike the untraced path. At its deadline, a traced wait with no element now rejects as missing. Traced twin of the missing-element self-test; it fails on the previous code. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…med or not The planner's comment on the cross-fade's length said it is not a theme role and no theme lengthens it. Under spec:AST-094 DEC-11 no motion role is themeable: the fade's length is no role at all, and nothing lengthens it.
A promotion keeps the stylesheet's will-change reasons beside its own, and read them with getComputedStyle as it started: in a drag's first move, that forces a style recalculation inside the input handler. The stylesheet's value is now read in a task of its own after the element is attached, and again after each demotion, so a promotion uses what was read. One that comes before that read (an element attached in this task that moves at once) still reads it then. A test that counts style reads in the promoting task fails on the previous code; one for the element that moves at once passes on both.
…rred unmount runs It read will-change right after attach(null), before the unmount's microtask, so it passed while that microtask could still promote the element and leave timers behind. It now awaits the microtask, then checks will-change and that no timer is left. It fails on meta#8521's earlier head (97436b2), before the unmount cleared what the landing rebuilt.
…pseudo-element support
…ith one passive listener and press-time document listeners
family:action-groups FR5 (and the AST-094 FR19/FR24 exception proposed with
it): the behavior applies to every pointer, mouse and pen included, so a
group must hear a press where the primary pointer is fine, where the host
otherwise registers nothing. attach() takes { actionGroup: true }: under a
fine pointer the group element gets one passive pointerdown listener; a
press on it adds document pointermove, pointerup and pointercancel
listeners (and window blur, plus the click listener for the click the lift
owes), and they go at the release. Only the group's own recognizers and
consumer take that press. Under a coarse pointer the host's usual listeners
serve the group, as every other recognizer.
Tests (failing first): idle under a fine pointer, only the group's one
passive pointerdown and nothing for a recognizer that is not a group; a
press adds the document listeners and the release removes them; a
recognizer that is not a group on the press path takes no part; detach
removes the group listener, and a coarse pointer keeps the usual
listeners.
…ond route around ./internal/* Lab's per-module tsconfig paths (core's motion, gestures and touchPresentation) and the Sandbox/docsite resolver that served them opened a route around architecture:react-component-runtime INV13's ./internal/* subpath exports. Nothing in Lab imports those paths here; they're dropped, and the import guard now fails on any Lab import core doesn't export and on any Lab tsconfig alias for core. Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
… detaches The deferred unmount demotes the detached node, and demote asks for a fresh will-change read; the detached node now gets none, so no timer outlives the element (architecture:react-component-runtime INV5). Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
… that finger until it lifts (spec:AST-094 FR14) A recognition that claimed its touches and was then cancelled gave them back to the browser unless another recognition owned them, so a finger resting there could pinch-zoom the page. A gesture that cancels itself on a finger's move, a long press dragged past its tolerance after it began, gave back a finger that was dragging: the host stopped cancelling its moves and the page scrolled the rest of the drag, a hand change FR14 forbids and UIKit never makes (its scroll view never takes a touch another recognizer had). A cancel during a finger's move now keeps that finger claimed until it lifts; a resting finger is given back as before, and a waiting recognition that fails still gives its claim back. The new arena case fails on the previous code: the dragging finger reads unclaimed right after the cancel.
…edge-zone strip decides at UIKit's 15 px The host's arena environment had no viewSize, so under the real host a screen-edge pan never saw its edge zone while a touch was down and decided at 10 px, while its attach-time touch-action (read through the default environment) had already claimed the axis (spec:AST-094 FR18). Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…fter layout rounding Lane 0's real-host browser spec found a strip authored at the 40.2 px edge region lays out at 40.203 px (layout snaps to 1/64 px), so the strict comparison read it as off the zone and the pan decided at 10 px instead of UIKit's 15 (spec:AST-094 FR18). The zone takes half a pixel of tolerance: the rounded strip is the zone, a view a pixel wider still is not. Failing-first test at 40.203 px: it claims pan-x and decides at 15. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
Main's meta#8521 is its head 25b468c squashed, which this pull request already carries: every file it touches is byte-identical to 25b468c. In the two files that conflicted (the motion README and animator.ts), the resolution is this side's version, main's merged meta#8521 with this pull request's later work on top. Nothing of main is dropped.
An attach reads the element's own will-change in a task of its own. A shared animator (animatorFor) is never detached, so its node can leave the document with that read still waiting, and the read then forces a style pass on a node with no style: a test run that mounts and unmounts many pagers piled up such reads until a later wait timed out. The read now skips a node out of the document; a promotion that needs it reads it then, as one on a node attached in the same task does. A detach and dispose already cancel it. The new test fails on the previous code (one read of a removed node); the in-task read test now puts its element in the document, where the read happens.
…(spec:AST-094 FR18, DEC-13) A screen-edge pan attached to a stack container, as wide as the screen, with a touch 4 px from its edge: inside the 40.2 px band the pan decides at UIKit's 15 px whatever the element's width, and then tracks the travel past it. From several edges too. Both fail while a wider element decides at FR19's 10 px.
…'s width (spec:AST-094 FR18, DEC-13) FR18: inside the 40.2 px band a screen-edge pan decides at UIKit's 15 px screen-edge hysteresis; outside it the author's scrolling is untouched and FR19's 10 px governs every other claim. The pan decided at 10 px on any element wider than the band, so NavigationStack's edge pan, attached to the stack container, began 5 px early. The edge zone still decides only the touch-action; a touch outside every band still fails at once. The two cases that pinned the 10 px decision go.
…er's move keeps that finger until it lifts) into the experimental touch exports
…than the band (spec:AST-094 FR18, DEC-22) FR18 as amended in meta#8631 (DEC-22): the band's pan-x claim and 15 px start hold only where the screen-edge pan is attached to an element within the band; on an element that reaches beyond it, whose touch-action cannot claim per touch, a touch in the band decides at FR19's 10 px like every other claim. This restores the 10 px decision 3d85c30 removed and keeps 15 px on an element within the band. The cases pin DEC-22 both ways (a wide element with a touch at x=4 begins at 10, an in-band strip at 15, and from several edges at 10 on a wide element); the 10 px cases fail on 3d85c30.
…xperimental touch exports The motion README's Files table is the union: main's rows, with meta#8663's resolveSnapPoint.ts and selectPage.ts, plus this branch's, in main's order. # Conflicts: # packages/core/src/motion/README.md
…g internal meta#8076 keeps touch motion internal, so a theme cannot retune the press feedback's spring or latency (spec:AST-094 FR4, DEC-11): the curve reads the table's values, the test checks that nothing comes from a theme, and the changeset and the table no longer say a theme can retune them. The CLI's motion doc no longer lists the values, so the changeset names only core.
There was a problem hiding this comment.
Changes requested. The ContextMenu timeline and press curve are well sourced and fit UIKit's measured frames, but this pull request's diff against main still carries the open #8076 stack: 16,482 authored lines beside its own 688. Please land #8076 first (or retarget onto its branch) and rebase so only the six ContextMenu files remain; two issues in that stack should be fixed there before it lands:
useGestureRecognizerforwards a mid-gesture cancel toonGesturefrom an Effect when its element unmounts or is replaced, unlikeusePressTracker, which guards the same release..changeset/gesture-press-tracking.mdsays every press painter writes through one writer, butactionGroupTrackerstill writesdata-astryx-pressdirectly.
[Automated review]
| useIsomorphicLayoutEffect(() => { | ||
| const element = ref.current; | ||
| const current = attachedRef.current; | ||
| if ( | ||
| current != null && | ||
| current.element === element && | ||
| current.deps.length === deps.length && | ||
| current.deps.every((dep, index) => Object.is(dep, deps[index])) | ||
| ) { | ||
| return; | ||
| } | ||
| current?.detach?.(); | ||
| attachedRef.current = | ||
| element == null ? null : {element, deps, detach: attach(element)}; | ||
| }); |
There was a problem hiding this comment.
This detach runs inside a layout Effect (and again in the unmount cleanup below), and GestureHost's detach cancels a recognizer that is mid-gesture. cancelled is an action state, so the listener subscribed above forwards it to onGesture, because isSyncingRef is only set around setEnabled. A component that unmounts or swaps its node while a finger is down then gets its callback from React's commit, which architecture:react-component-runtime INV2 rules out and which usePressTracker in this file already prevents. Holding the guard around both detach calls, plus a mid-gesture unmount and node-swap test, would close it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: UIControl press tracking with control and cell profiles and the gesture hooks, with every press painter writing `data-astryx-press` through one internal writer in `utils/pressFeedback.ts`. Not exported from the package entry point. |
There was a problem hiding this comment.
hooks/actionGroupTracker.ts (lines 252–272 and 441–442) still sets and clears data-astryx-press directly with its own fade timer, and paintPress has no caller outside its test, so "every press painter … through one internal writer" is not true at this head. Either move the tracker onto paintPress or drop the claim here and in pressFeedback.ts's header.
| } | ||
|
|
||
| /** When the visible curve starts, ms after the finger landed. */ | ||
| export const PRESS_FEEDBACK_START_MS = feedbackOnsetMs + feedbackLatencyMs; |
There was a problem hiding this comment.
pressFeedbackCurve reads motion.spring and motion.onsetMs but never motion.latencyMs, so the curve's start is always this table-derived constant, whatever pressFeedbackMotion returns. That is harmless while every value is the table's, but the description's "the curve takes them" holds for two of the three; deriving the start from the motion passed in, or dropping latencyMs from PressFeedbackMotion, would make the contract say what it does.
Depends on #8076 (the UIKit constants table, its registry and the animator); review the last two commits.
Maintainer impact
Part of the UIKit-parity program: ContextMenu's touch invocation follows UIContextMenuInteraction's timeline (#8201 onward). Without measured numbers in the UIKit constants table, each component would guess its own timings and drift from iOS; the table's test refuses any value without a source.
Intended invariant
Every leaf in
motion/uikit/*.tscarries an@source(measured, Apple documentation, or a GUESS with a@measuretask); components never inline physics; and touch motion stays internal, so no value here is a motion role a theme retunes (spec:AST-094FR4, DEC-11). Nothing imports the new values yet.Change
Measured on iOS 26.2 (iPhone 17 Pro simulator) with a UIKit reference app that logs recognizer states and per-frame Core Animation values,
motion/uikit/contextMenu.tsrecords:ContextMenu/contextMenuPressCurve.tsis that chase in closed form, so the compositor plays it with no per-frame JavaScript; stepping the spring each frame would stall whenever the main thread is busy.Proposed values, pending owner review (each a GUESS with its measurement task): the tolerance's exact value, 11.5 (M-CM3); the commit haptic's
navigator.vibratestand-in, a medium impact (M-CM5).Evidence
Scope
Testing
What a reviewer can check:
pnpm vitest run packages/core/src/motion: the table's test fails any value without a source tag or measurement ID; delete one@sourceand it goes red.pnpm vitest run packages/core/src/ContextMenu: the press curve is compared with the recorded UIKit frames, for a long press and for a press lifted at 0.25 s.