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
a9b0ef6 to
b354e4f
Compare
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.
The press-paint change itself restores design:reduced-motion DR3 (under Reduce Motion with a fine pointer the release completes at once) and keeps DR2's 200 ms opacity fade on coarse pointers, but this head also carries the whole touch-presentation stack, so it cannot be approved as it stands. The stack's draft spec:AST-094 requirements and the additions built on them (the experimental motion and gesture subpaths, the motion/reducedMotion.stylex export, defineTheme({motion}) and the presentation="adaptive" change) need an engineering owner's decision, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), unless this pull request instead waits for its stack to land and carries only the press-paint change. Whichever way that goes, these need fixing where they live:
- The head is about 30,800 authored lines; this pull request should shrink to the four press-paint files.
- AST-094 FR8 still asks for a 0.5 s cross-fade and Reduce Motion springs, which
design:reduced-motionDR1 and DR2 forbid. .changeset/touch-motion-theme-roles.mdofferstiming.reducedMotionCrossFadeMsas a theme role thatdefineThemerefuses, and.changeset/uikit-motion-animator-curves.mdstill describes a Reduce Motion spring.
[Automated review]
| - **FR8 — Reduce Motion substitutes, never jumps.** Under | ||
| `prefers-reduced-motion: reduce`, a touch presentation MUST behave as its | ||
| UIKit counterpart does with Reduce Motion and Prefer Cross-Fade Transitions | ||
| 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 | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that | ||
| follows a finger, such as a throw, MUST keep its motion. **Proposed; pending | ||
| owner review:** this is the minimally moving form the draft `design:motion` | ||
| DR5 asks for, for touch presentations (owners: ernestt, cixzhang); the | ||
| alternative is UIKit's Reduce Motion alone, which keeps the slides of | ||
| navigation, sheets and modals. | ||
|
|
There was a problem hiding this comment.
FR8's 0.5 s navigation cross-fade and the presentation and menu springs under Reduce Motion contradict design:reduced-motion DR1 ("MUST NOT move, scale, resize, or spring its content") and DR2 ("MUST finish in 200 ms or less"). The code already follows DR2 (packages/core/src/motion/uikit/timing.ts, reducedMotionCrossFadeMs: 200). Could FR8 defer to design:reduced-motion DR1 to DR3 instead?
| '@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 theme role: registerTables.ts excludes it (DR4), and defineTheme refuses it with "is not a motion role". A real role such as timing.implicitAnimationMs would make the example accurate.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] The touch presentations' animator plays any closed-form curve (such as a spring chasing a moving target), drives a pseudo-element such as a dialog's ::backdrop, fades on a presentation's own spring under Reduce Motion (with an entrance or exit role for a slide that changes no opacity), and starts a call from rest when the browser commits it, as Core Animation does. Not exported from the package root; touch presentations reach it through the experimental subpaths (see their entry). |
There was a problem hiding this comment.
"fades on a presentation's own spring under Reduce Motion" no longer matches the animator. .changeset/reduced-motion-record.md in this head says every call becomes a 200 ms opacity-only cross-fade with nothing springing. Dropping this clause keeps the changelog consistent.
b354e4f to
5bd24e3
Compare
5bd24e3 to
bc3532a
Compare
605080b to
fa2fec6
Compare
There was a problem hiding this comment.
The press-paint change itself restores design:reduced-motion DR3 (with Reduce Motion and a fine pointer the release now ends at the lift) and keeps DR2's 200 ms opacity fade on coarse pointers, but this head still carries its whole stack, so it can't be approved as it stands. Two of the stack's public additions have no governing spec, theme motion roles (defineTheme({motion: {springs, durations, curves}})) and the astryxStylex({undefinedVariables}) option: an engineering owner needs to approve them for this exact head by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner), unless this pull request instead waits for #8045 to #8076 to land and carries only the press-paint files. Either way:
- The diff should shrink to the five press-paint files; the head is about 29,900 authored lines.
VelocityTrackeroffers four selectable filters and no stillness reset, whicharchitecture:interaction-modalityINV10 andspec:AST-100FR2 onmainrule out.springs.menuReducedMotionshould not be a theme role (design:reduced-motionDR4), and.changeset/reduced-motion-record.mdshould describe an internal change rather than an "experimental touch animator".
[Automated review]
| * header describes, `uikit` by default. | ||
| * @experimental | ||
| */ | ||
| export type VelocityStrategy = 'uikit' | 'lsq2' | 'iosFling' | 'window'; |
There was a problem hiding this comment.
architecture:interaction-modality INV10 and spec:AST-100 FR2 (now on main) allow one release filter, iOS's blend with a stillness reset that reads 0 after a still pause, and no selectable alternatives. This type offers four filters, PanGestureRecognizer takes one per pan (velocityStrategy), and the default never decays after a pause (lines 18-20). Could the tracker keep only the blend and add the stillness reset and the scroll-view release blend?
| '0.32s 0.2', | ||
| ], | ||
| [ | ||
| 'springs.menuReducedMotion', |
There was a problem hiding this comment.
springs.menuReducedMotion is listed as a retunable role, but under Reduce Motion the animator replaces every spring with the 200 ms cross-fade and nothing reads this role, and design:reduced-motion DR4 says theme values offer no way to keep springs under reduced motion. Excluding it from the springs table's roles, as timing.reducedMotionCrossFadeMs is excluded, would keep these docs accurate.
| '@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 with no experimental subpath, and no release had the reducedMotion and crossfade options or the reducedMotionCrossFadeMs role, so this note would announce a fix and removals to an API that never shipped. A [chore] Internal: note like its siblings would fit.
|
Thanks for confirming the press-paint change (ad40922, fa2fec6). This pull request is stacked on #8076, so its diff carries that stack until it lands. The description now says to review only those two commits. The other points belong to the stack:
Once #8076 lands, this rebases onto main and its diff is the five press-paint files. |
play(curves) bakes any curve given as value and velocity functions of time (a spring chasing a moving target from solveSpringTowardRamp, a fitted UIKit curve) exactly like a spring: presentation values, velocities, stop, finished and the shared clock work the same, a later animate starts from the curve's value and velocity, and under Reduce Motion a curve that moves is travel the policy replaces. Tests and a changeset of its own. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
createAnimator(element, {pseudoElement: '::backdrop'}) animates a
pseudo-element: with no inline style, a static filling animation holds
its value after a curve ends and after set; a custom property on its host
promotes it; a Reduce Motion dip borrows and hands back the
pseudo-element's own opacity, and its holds follow a node replacement.
animatorFor keeps one animator per element and pseudo-element. Tests and
a changeset of its own.
Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…ot the page's frames around it "a harness read inside a trace is not the page's long task…" also asserted that no long task started anywhere in the read's window. On CI's runner each frame of the fixture's 1,200 running rows took 51-54 ms, the page's own long tasks, and failed it; at 4x CPU locally they take 126-220 ms. The read itself was set apart correctly. The test now asserts the read's own classification, and removes the rows before the timer, click and direct-call windows, so each holds only its own work. It passes at 4x CPU, where the previous assertion failed.
…hed curve waitForAnimation's finished branch read every recorded animation, so a scroll- or view-timeline animation that reached its end (a scroller scrolled to its end) came back as finished with a NaN duration and interval. The recorded set now keeps document-timeline animations only. The scroll-timeline self-test scrolls its scroller to the end first; it returned the NaN entry before (Chromium, 3x repeat pass). The README, JSDoc and file header name watchAnimations where they list what moves the wait window and what the file exports. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…rves), pseudo-element support) as a dependency # Conflicts: # packages/core/src/motion/README.md # packages/core/src/motion/animator.ts
… motion-role animator # Conflicts: # packages/core/src/motion/animator.ts
…deselect clock; the animator changeset drops its no-component clause The press profiles' README gave a control UIButton's 0.25 s release. PRESS_PROFILES releases every surface on the list's deselect clock until a control runs its own release, as its comment says; the README now says so. Its test list still credited VelocityTracker's tests with checking each strategy against the true velocity, though one filter remains: it now names what they check. The animator's changeset said no component uses it, which goes false in the release that carries the components that do.
…removed at its end is still reported - Arming the recorder now records first and stamps strictly after the newest entry: a watch taken right after a step that changed style no longer reports that step's transition (created by the arm's own style read) as started after it. - Each entry keeps its target's ancestors as recorded, so an exit whose element the page removes at its end is still reported for the container it left, instead of a wait rejecting with nothing recorded. A self-test for each; both failed before (Chromium, 3x repeat pass). Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…he frames it credits as skipped
With a compositor record and an on-screen start, the moved criterion counted
from the first moved frame after the on-screen frame, while the gap from the
on-screen frame to it was credited as frames the screencast skipped. When the
cast skipped the frames right after the on-screen start, those credits had
no place in the denominator: four unmoved frames that fail on a full cast
("31 of 35 expected frames moved") passed as "26/30 moved and 5 skipped by
the screencast (103%)", and a clean run read 117%.
The criterion now counts the compositor's frames from the on-screen frame,
so both sides cover the same frames: the skipped cast fails like the full
one, and a clean run reads at most 100%. A busy.test.ts case pins all three.
… while a trace runs Polled inside a traced body, each look added a Layout of an anchored element in every frame (24 in the anchored fixture's steady middle). Four reads did it, each alone: Playwright's locator.evaluate (each call makes a main-thread frame), the recorder's document-wide read, an unwatched wait's element.getAnimations(), and effect.getComputedTiming(), which updates a running animation on demand and restyles its target. While a trace runs, record() from outside the recorder and arm()'s read stand down; the wait takes the element's animations from the recorder's log, judges a started one by its own times (playState only before it has a start time), reads timing through getTiming(), and reaches the element through a handle to what the locator matches at each look. The same window now lays out 0 times.
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/src/curves.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts
# Conflicts: # internal/interaction-harness/src/curves.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts
…down # Conflicts: # internal/interaction-harness/src/curves.ts
There was a problem hiding this comment.
This needs changes before it can merge: the press-paint fix itself does what design:reduced-motion asks (under Reduce Motion the release now ends at the lift on a fine pointer and stays a 200 ms opacity fade on a coarse one, with nothing changed otherwise), but the diff still carries the #8076 stack underneath it. That stack, the internal gesture and motion code, the interaction harness and the astryxStylex treeshakeCompensation default, is separate work this pull request should not land. Once #8076 is merged, please rebase onto main so the diff is just the five press-paint files, as you planned.
[Automated review]
Superseded by #8118, which carries both of its commits; closing in its favor. The branch stays.
Depends on #8076 (and its predecessors, at a71f4e5); review only the last two commits (ad40922, fa2fec6). Until #8076 lands, this diff also carries its stack.
User impact
Under Reduce Motion, the shared press paint (every press-painted surface: buttons, rows, cards, switches, tabs) still played its normal release on every pointer. It's a 200 ms ease-out fade of the overlay after the finger or pen lifts. On a fine primary pointer,
design:reduced-motionDR3 requires the change to complete at once. On a coarse pointer, DR2 allows only the system cross-fade.Problem and solution fit
[data-astryx-press="fading"]) had no Reduce Motion condition.(prefers-reduced-motion: reduce) and (not (pointer: coarse)), the arm now sets the paint's strength to 0 and runs no animation, so the paint is gone at the lift.(prefers-reduced-motion: reduce) and (pointer: coarse), the fade takesreducedMotionFadeConsts.durationand.easing, the system cross-fade, which is 200 ms.reducedMotionKeys.fadeand.instant; a computed key would change the rule's StyleX priority.Expected behavior and authority
design:reduced-motion(current) DR1–DR3. No change without Reduce Motion.Smallest restoration
The press paint's two release objects in
interactionOverlay.stylex.ts. The controller and its timer are unchanged: under the instant key the attribute still clears after the release clock, with nothing painted. No API change.Evidence
pressPaintReducedMotion.test.tsfails on feat(core): the internal touch foundation: press tracking and the animator #8076. It compiles the overlay and finds no Reduce Motion arm.animation-name: noneoutrank the release arm. Under the fade key, the system duration and curve outrank the 200 ms ease-out.:activereset.pnpm -F @astryxdesign/core build):dist/astryx.csshas the coarse arm withanimation-duration:200msandanimation-timing-function:cubic-bezier(0.42, 0, 0.58, 1), and the fine arm with--astryx-press-alpha:0andanimation-name:none; no constant placeholder is left.Scope
Testing
Restacked on feat(core): the internal touch foundation: press tracking and the animator #8076 @a71f4e51 (motion and gestures internal):
tscfor core and lab, changesets, sync-exports and check:sync clean.pnpm exec vitest run --project ui packages/core/src/utils packages/core/src/motion: 952 passed. The 15 test files that read press state through__tests__/pressState.ts(now able to skip rules inside a Reduce Motion@media): 756 passed.tsc --project packages/core/tsconfig.json --noEmit,ASTRYX_STRICT_LINT=1 eslinton the changed files: clean.pnpm check:changesets,pnpm check:sync: clean.Size: 4 files, 165 lines added, 12 removed. No new runtime import; the Reduce Motion values come from a
.stylexconstants file at build time.Stack: #8052 spec → … → #8074 gesture system → #8076 internal motion and gestures → this pull request.