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
needs:design-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.
Approval of this pull request waits on a spec decision. On main, spec:AST-094 FR4 and DEC-11 keep the touch motion roles internal, while the spec:AST-108 requirements (with the spec:AST-094 DEC-15 and related amendments) that let a theme's tempo and character retune them appear here only as proposed text, in an earlier version than the one approved on #8613. An engineering owner records that decision by commenting /approve-spec 5e235e66b2a97e2e81d798820889f71d03bd8d29 on this pull request (the author may do so if they are an engineering owner); otherwise the pull request waits for #8613 to land on main and rebases onto it. Whatever the decision, two changes are needed:
- Rebase onto
mainonce #8076 and #8661 land, so the diff carries only this change; today it counts 17,872 authored lines against a 499-line own change. - Make the springs follow the knobs the CSS follows (an adaptation rule's
tempoorcharacter, and an enclosing theme's knobs, whichthemeMotionKnobsnever reads), or state in the spec that they do not.
[Automated review]
| export function themeMotionKnobs( | ||
| element: Element | null | undefined, | ||
| ): ThemeMotionKnobsAt { | ||
| const scope = element?.closest?.(THEME_SCOPE); | ||
| if (scope == null) { | ||
| return DEFAULT_KNOBS; | ||
| } | ||
| const tempoValue = Number(scope.getAttribute('data-astryx-motion-tempo')); | ||
| const tempoIsSet = | ||
| scope.hasAttribute('data-astryx-motion-tempo') && | ||
| Number.isFinite(tempoValue) && | ||
| tempoValue > 0; | ||
| const characterValue = scope.getAttribute('data-astryx-motion-character'); | ||
| const character: ThemeMotionCharacter = | ||
| characterValue === 'crisp' || characterValue === 'expressive' | ||
| ? characterValue | ||
| : 'standard'; | ||
| if (!tempoIsSet && character === 'standard') { | ||
| return DEFAULT_KNOBS; | ||
| } | ||
| return {tempo: tempoIsSet ? tempoValue : 1, tempoIsSet, character}; |
There was a problem hiding this comment.
The springs read only this scope's attributes, which Theme.tsx writes from the theme's root motion. The CSS also follows an adaptation rule's tempo or character, and inherits an enclosing theme's scaled --duration-* when the nearest theme sets no motion. In both cases the springs and the CSS diverge. For example, a when: {pointer: 'coarse'} rule with tempo: 1.3 makes the CSS 1.3× while the springs keep the root tempo. Either resolve the knobs the way the CSS does, or state the limit in AST-108 FR3 and FR5, with a test either way.
| return null; | ||
| } | ||
| const curve = solveSpring(toSpring(spring), {from, to, velocity}); | ||
| // The motion role it plays: its table value while DEC-11 holds. |
There was a problem hiding this comment.
After this change a role follows the theme's knobs (resolveMotion.ts), so "its table value while DEC-11 holds" no longer describes it. The same goes for line 137 and for README.md lines 49-50 and 72-73.
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
…SS part's settle clock
There was a problem hiding this comment.
Approval of this pull request waits on a spec decision: on main, spec:AST-094 FR4 and DEC-11 keep the touch motion roles internal, and the spec:AST-108 records (with architecture:theme-authoring-contract INV15) that let a theme's tempo, character and reducedMotion change them are proposed here without approval for this head. An engineering owner records that decision by commenting /approve-spec f2546dbdc1b0830d730cdae6793c51a5d6c3028f on this pull request (the author may do so as an engineering owner), or the records land on main first. Whatever the decision, these changes are needed:
- Make
reducedMotion: 'always'reach the components' own reduced-motion styles and script readers before the docs and changeset promise that it reduces motion for everyone, or say what it reaches today. - Write
--pre-tempo-duration-*wherever a theme writes a duration, so a nested theme's scale or explicit duration still times Spinner and Skeleton under a theme that setstempo. - Make the springs follow the knobs the CSS follows (an adaptation rule's
tempoorcharacter, and an enclosing theme's knobs), or state that limit in the spec, with a test either way. - Rebase once #8076 and #8661 land, so the diff carries only this change (17,911 authored lines today).
[Automated review]
| items: [ | ||
| "`tempo` scales every motion duration: 1 is Astryx's own, 0.75 the quickest and 1.5 the slowest (values outside are clamped).", | ||
| "`character` sets how lively motion is: `'crisp'` decelerates without overshoot, `'standard'` is Astryx's own, and `'expressive'` overshoots slightly on entrances and moves. Exits and fades never overshoot.", | ||
| "`reducedMotion: 'always'` reduces motion for everyone, as if each person had turned on Reduce Motion; `'system'`, the default, follows each person's setting. No value loosens Reduce Motion.", |
There was a problem hiding this comment.
No core component reads the data-astryx-reduced-motion mark yet: the 50 non-test core files that read prefers-reduced-motion in a style or script (Dialog, BottomSheetPanel, TopNavMegaMenu, Step among them) ignore it, so a person without the setting still gets full motion under a theme on 'always'. Only the theme's own when: {motion} adaptation rules and the internal animator honour it. Either make those styles and script readers key on the mark (as spec:AST-108 FR6 requires) before promising this, or say what 'always' reaches today. The changeset's "so motion is reduced for everyone" has the same problem.
| ): Record<string, string> { | ||
| const knobs = resolveMotionKnobs(config); | ||
| const tokens: Record<string, string> = {}; | ||
| if (knobs.tempoIsSet) { |
There was a problem hiding this comment.
--pre-tempo-duration-* is written only when this theme sets tempo, and resolveThemeValues copies an explicit duration into it only when the name already exists. Custom properties inherit, so a nested theme with its own duration scale or explicit duration but no tempo leaves Spinner and Skeleton reading the outer theme's pre-tempo value. Example: inside a tempo: 1.5 theme, a nested motion: {fast: 100, medium: 250, slow: 600, ratio: 0.75} theme's Spinner turns every 730 ms instead of its own 450 ms --duration-slow-min. Writing the pre-tempo twin wherever a theme writes a duration would fix it; a nested-theme test would pin it.
| * the element is in (`spec:AST-108` FR3): `tempo` scales a spring's duration | ||
| * and a role's duration, and `character` moves a spring's bounce by its | ||
| * role's weight (1 for entrances and presentations, 0.5 for state changes, 0 | ||
| * for exits, which never overshoot). The theme's `MotionScaleConfig` alone |
There was a problem hiding this comment.
"0 for exits, which never overshoot" no longer matches spec:AST-108 FR3 at this head, which says a weight-0 role keeps its measured bounce: springs.menuClose (damping ratio 0.85) overshoots by about 0.6% at every character. The same wording sits in theme/motionKnobs.ts:34, theme/defineTheme.ts:243, cli/assets/theme.template.ts:186 and motion.doc.mjs:159.
…rt's settle clock
There was a problem hiding this comment.
Approval of this pull request waits on one spec decision: architecture:theme-authoring-contract INV15 adds "an explicit token anywhere in the lineage still wins", which no record on main states and which lets a base theme's explicit --duration-* or --ease-* token beat a child's own duration scale. An engineering owner records that decision by commenting /approve-spec eeec68709d2d3c5de4c5138180bcce7b740a4f99 on this pull request (the author may do so as an engineering owner), or the clause can be dropped so a child's own scale keeps winning. Whatever the decision, these changes are needed:
- Make component styles and script readers honour the
reducedMotion: 'always'mark, asspec:AST-108FR6 requires, or keep'always'out of the docs and changeset until they do. - Make the springs follow the knobs the CSS follows: an adaptation rule's
tempoorcharacter, and an enclosing theme's knobs. - Write
--pre-tempo-duration-*wherever a theme writes a duration, so a nested theme still times Spinner and Skeleton under a theme that setstempo. - Rebase once #8076 and #8661 land, so the diff carries only this change (17,803 authored lines today).
[Automated review]
| export function themeMotionKnobs( | ||
| element: Element | null | undefined, | ||
| ): ThemeMotionKnobsAt { | ||
| const scope = element?.closest?.(THEME_SCOPE); |
There was a problem hiding this comment.
The springs read only the nearest theme scope's attributes, which Theme.tsx writes from the theme's root motion. An adaptation rule's tempo or character, and an enclosing theme's scaled durations, reach the CSS but not the springs: a when: {pointer: 'coarse'} rule with tempo: 1.3 makes --duration-medium 533 ms while springs.settle stays 0.5 s. spec:AST-108 FR3 lists an adaptation rule among its verified states, and FR4a needs a CSS fade and its spring to land together.
| ): Record<string, string> { | ||
| const knobs = resolveMotionKnobs(config); | ||
| const tokens: Record<string, string> = {}; | ||
| if (knobs.tempoIsSet) { |
There was a problem hiding this comment.
Only a theme that sets tempo writes --pre-tempo-duration-*, and custom properties inherit. Inside a tempo: 1.5 theme, a nested theme with motion: {fast: 100, medium: 250, slow: 600, ratio: 0.75} leaves its Spinner turning every 730 ms instead of 450 ms and its Skeleton pulsing over 550 ms instead of 335 ms (component:Spinner FR3, component:Skeleton FR4). Writing the pre-tempo twin wherever a theme writes a duration, with a nested-theme test, would fix it.
| items: [ | ||
| "`tempo` scales every motion duration: 1 is Astryx's own, 0.75 the quickest and 1.5 the slowest (values outside are clamped).", | ||
| "`character` sets how lively motion is: `'crisp'` decelerates without overshoot, `'standard'` is Astryx's own, and `'expressive'` overshoots slightly on entrances and moves. Exits and fades never overshoot.", | ||
| "`reducedMotion: 'always'` reduces motion for everyone, as if each person had turned on Reduce Motion; `'system'`, the default, follows each person's setting. No value loosens Reduce Motion.", |
There was a problem hiding this comment.
No core component reads data-astryx-reduced-motion yet: the 48 core files that key reduced motion on prefers-reduced-motion ignore it, so a person without the setting still gets full motion under an 'always' theme. spec:AST-108 FR6 requires those styles and script readers to key on the mark; until they do, this line and the changeset overstate what 'always' does.
| input.motion != null && baseMotion != null | ||
| ? {...baseMotion, ...input.motion} | ||
| : input.motion; | ||
| const lineageMotionTokens = |
There was a problem hiding this comment.
This re-applies the base's explicit --duration-* and --ease-* tokens above whatever the child generates, so a child's own duration scale no longer beats a base's explicit token, as it does today. That is the INV15 clause waiting on an owner decision; if it stays, the record's System model precedence list should say so too.
| * the element is in (`spec:AST-108` FR3): `tempo` scales a spring's duration | ||
| * and a role's duration, and `character` moves a spring's bounce by its | ||
| * role's weight (1 for entrances and presentations, 0.5 for state changes, 0 | ||
| * for exits, which never overshoot). The theme's `MotionScaleConfig` alone |
There was a problem hiding this comment.
"0 for exits, which never overshoot" contradicts spec:AST-108 FR3: a weight-zero role keeps its measured bounce, and springs.menuClose (damping ratio 0.85) overshoots by about 0.6%. The same wording is in motionKnobs.ts:33-34, defineTheme.ts:242-243, theme.template.ts:186 and motion.doc.mjs:159.
…ionAnimator The public createAnimator (spec:AST-107 FR6) takes the name; meta#8677's tests of the internal animator now import it by its own name.
…coasts every channel spec:AST-107 FR3 as merged: - Every animate plays springs.stateChange at the tempo and character of the theme the element is in (spec:AST-108 FR3), which meta#8677 brings; the new case fails with the motion module as it was before it. - decay coasts any of the five channels it is given on the fixed momentum decay (spec:AST-108 FR4): the translate axes as a throw that Reduce Motion lets continue, scale, rotate and opacity as a transition it plans like a move (design:reduced-motion DR2); opacity stops at 0 or 1. It took only x and y before. - destroy halts every channel where it shows before letting go, so nothing reads as moving after it, and every method after it does nothing; destroy removes every listener it added (FR6). The docs and the changeset say the same.
Main's golden counts 367 matches for "icon set"; this branch's public animator pages and meta#8677's motion docs each add one, so the merged count is 369. The listed results and every other field are unchanged, and the diff names no icon set and nothing experimental.
…ch the SideNav rail moves onto (#8721) Implements the records #8783 put on main: `spec:AST-105` FR2 and FR9, `architecture:react-component-runtime` INV12, `component:SideNav` FR6, `component:Resizable` FR8, `component:LayoutPanel` FR7 and AR2, and `component:Table` FR15. Resizable's and LayoutPanel's slide and LayoutPanel's focus rule (AR2: focus moves to the ResizeHandle only when a collapsing pane holds it) are built on this path in #9197 and #9202. This PR moves no focus anywhere. Behavior-module PR: `packages/core/src/motion/animatedLayout.ts` (FR9's shared edge reveal). ## User need A panel that collapses beside content, a Collapsible closed while it opens, a resizable pane sliding shut: each reveals or conceals an element from one of its edges while the content beside it moves. Before this change Core had one such reveal of its own per component. The SideNav rail clipped and rode its content in its own code. Collapsible's reveal could only open downward, and a close while it opened restarted the conceal from fully shown while the content below kept gliding, so a gap opened. Nothing a resize controller could reuse existed. #8783 made these one shared edge reveal in the animated-layout module. ## Problem and solution fit The animated-layout module now produces `spec:AST-105` FR9's edge reveal, in three forms over one ride: - **One-state** (`playLayoutChange`, `holdLayoutExit`): reveals and conceals from the block-end edge by default, or from the block-start or an inline edge it is given, mirrored right to left. Once the content on that edge's side glides, the moving edge rides that glide frame by frame (its keyframes replaced, its clock the glide's), so no gap opens. It rides the content's offset relative to the element: when the same batch also moves the element itself, such as an item above them closing in a single-open CollapsibleGroup, the edge follows only the difference. A conceal by a start edge frees the element's space on that edge's side, so the element stays where it shows while the content before it moves; a layout where that would move the element itself is not held, and the element goes at once. A reversal starts from what the element shows. Collapsible, TreeList and Table's row expansion already play this block-end form, so they gain the ride, and Collapsible and TreeList, which hold their exits, the reversal too. - **Two-state** (`holdLayoutEdge`, `playLayoutEdge`): holds the old state as FR8's swap copy, clips the larger state on the edge from the extent it shows, rides a gliding element, and raises the element above the copy while it conceals. The SideNav rail now plays through it on its inline-end edge and keeps only its labels' and icons' fades (`component:SideNav` FR6); its frames are unchanged. - **A resized element with no copy** (`reportLayoutEdge`): measured before a size change commits and played after the new size applies. A larger extent reveals from the extent shown. A smaller one holds the element at the extent it was laid out at, freeing the difference by the margin on that edge's side so what lies beyond stays where the new size puts it, then clips down to the new extent. This is the path a resize controller's pane uses (`component:Resizable` FR8 and `component:LayoutPanel` FR7 in #9197 and #9202). Reduce Motion follows `design:reduced-motion` DR2 under a coarse primary pointer and DR3 otherwise. ## Current specification `spec:AST-105` FR9 ("One shared edge reveal") and FR2, `architecture:react-component-runtime` INV12, `component:SideNav` FR6, `component:Resizable` FR8, `component:LayoutPanel` FR7 and AR2, and `component:Table` FR15, all current on main since #8783. A LayoutPanel given a hand-built `ResizableProps` gets no slide, and the hook's own `props` passed through `resizable` slide; both are #9197 and #9202's to implement. ## Public delta and caller burden None. The new functions and types stay internal: no entry point exports them (`animatedLayout.boundary.test.ts` checks this), and `useAnimatedLayout`'s contract is unchanged. The visible changes are Collapsible's and TreeList's: content closed while it opens, or opened while it closes, continues from what it shows, and its edge stays on the content after it (`[fix]` changeset). Table's row expansion reveal rides the rows after it. ## Design and alternatives - **Riding:** the edge's keyframes are sampled on the glide's own sample grid and clock, so the clip and the glide are complementary by construction. The alternative, a separate spring tuned to match, drifts whenever the glide carries a running motion. - **Resized element without a copy:** holding the old extent keeps the pane's content laid out as it was. Clipping a pane already at its new size would need its content to overflow, or a copy, which the runtime record rules out for a resize controller. - **Morph routing** of the glide stays out until the shared rect morph lands (#8329), as INV12's "until then" clause says. ## Evidence Status at this head: rebased onto main 8a03d75 after the component records moved to `docs/specs/components/`; the patch is byte-identical to 70ebcc7's. Here, build, type check, CI-strict lint, `check:knowledge`, the record-path tests (knowledge paths, check-knowledge, specs-for-file), `check:changesets` and the motion, SideNav and AppShell unit suites (1,020) pass, and `specs:for` still returns `component:SideNav` and `spec:AST-105` for the changed files. At 70ebcc7 every gate finished: `test:ui`, `check:repo`, the node suites (only the two `cache.test.mjs` lock cases that fail on main), and the three motion specs ×10 in WebKit under the CI job's command (740, with 100 Chromium-only skips) and in Chromium pinned to 2 cores at 4 workers (840). **Still running:** `test:ui`, the node suites, `check:repo` and the three motion specs ×10 in both engines at this head. Each new proof failed on main first: - `animatedLayout.edge.test.ts`: 17 of the 18 one- and two-state cases fail on main (the block-end default passes there and guards it), and the resized-element cases have no function there. - `SideNavRailMotion.test.tsx`: a new case asserts the rail plays through the module's two-state form with no clip, copy or ride of its own. It fails on main; the rail's existing cases pass unchanged with the rail moved. - `animatedLayout.interaction.browser.spec.ts` (FR9): a block-end reveal reversed into a conceal, an inline-end reveal and conceal (LTR and RTL), the two-state form, and a resized pane (start pane LTR to 48 px, start pane RTL to 0, end pane LTR to 48), each with a reversal. On every frame the moving edge stays within 1.5 px of what it rides. All fail on main: the block-end reversal jumped 40 px, the inline edge was clipped from the bottom, and the other forms don't exist there. - `animatedLayoutAdopters.interaction.browser.spec.ts`: a Collapsible closed while it opens, and a TreeList row collapsed while it expands, keep their edge on the content after them. On main each conceal restarted from fully shown: −16 px and −36 px gaps. Two more cases guard this PR's own paths. A block-start reveal and conceal in a column that stacks from its end keep the edge on the content before it, with the element staying where it shows; before the start-edge fix the conceal freed its space at its end, so the element jumped by its 80 px and an 80 px gap opened. And in a single-open CollapsibleGroup switching from a taller item to a shorter one below it, the opening item's edge stays on the next item on every frame; before the ride was taken relative to the element, the content showed 31 px over the next item. How the proofs read a ride: both sides on the module's clock, the edge from its clip animation's keyframes and what it rides from its layout position plus its glide's keyframes, at the animations' current time. Chromium also asserts that its evaluated clip and rect equal those (under 0.5 px). In WebKit the two run a little apart, and that skew is reported per test, not hidden: a compositor-run `translate` is evaluated a little ahead of the frame and, on a late first frame, a main-thread clip reads one frame behind. Under the CI WebKit job's command (`LP_NUM_THREADS=4`), the summed skew per frame reached 0.7 to 3.4 px depending on the test. The reversals pause and seek every animation to 120 ms, and the adopters' wait for the component to commit the close before playing on, so neither a late first frame nor a component committing in a later task moves the moment they test. ## Scope - [x] One capability is added (FR9's shared edge reveal), with its adopters in this PR's scope (the SideNav rail; Collapsible, TreeList and Table's row expansion keeping their form). - [x] Unrelated fixes, cleanup, and policy changes were removed or split. Resizable's and LayoutPanel's adoption is #9197 and #9202. - [x] Consumer docs, focused tests, and migration evidence ship with the feature. - [x] Public text and artifacts contain no internal Meta context. Overlaps: #8677 also edits `motion/animatedLayout.ts` (the theme's motion knobs), and #3890 edits `SideNav/SideNav.tsx`. This PR leaves `SideNav.tsx` unchanged. ## Testing ``` pnpm exec vitest run --project ui packages/core/src/motion packages/core/src/SideNav packages/core/src/AppShell packages/core/src/Collapsible packages/core/src/TreeList packages/core/src/Table pnpm build && pnpm storybook:build pnpm test:interaction packages/core/src/motion/__tests__/animatedLayout.interaction.browser.spec.ts packages/core/src/motion/__tests__/animatedLayoutAdopters.interaction.browser.spec.ts packages/core/src/SideNav/__tests__/SideNavRailMotion.interaction.browser.spec.ts --project chromium ``` For WebKit, use the CI WebKit job's container command (`internal/interaction-harness/scripts/webkit-container.mjs`) with podman `--cpus=4`, `CI=1`, `LP_NUM_THREADS=4` and 1 worker.
6e150ba to
9359e05
Compare
vjeux
changed the title
github-actions
Bot
removed
needs:design-review
There was a problem hiding this comment.
Changes requested. Springs and the layout glide now follow a theme's motion.tempo as spec:AST-108 FR3 describes, and the new unit tests fail on main and pass here. These need fixing first:
- The glide's spring follows
tempounder every pointer, so a mouse user's SideNav rail collapse or AppShell content glide is retimed (resolveMotion.ts:131-138,animatedLayout.ts:179), and this desktop behavior change needs an owner decision: gate the scaling to touch input, or cite the decision that covers it. - Switch's release replays the raw
switchControl.lift,switchControl.settleandminimumLiftMs(useSwitchTouch.ts:894-896and:965-966), so at tempo 1.5 the press lifts on a 970 ms spring but the release settles in 1,125 ms instead of 1,687 ms; resolve the three against the thumb. - A theme with only a duration scale, nested inside a tempo theme, keeps its own durations but plays its springs at the outer tempo, against
spec:AST-108FR5 and the changeset's own sentence, so a layer that writes durations without a tempo should also write--_astryx-motion-tempo: 1, with a test ondefineThemethemes (nested, and under an adaptation rule). themeTempo(themeTempo.ts:34-39) copies the tempo read inpressFadeMs(utils/pressFeedback.ts:158-161), sopressFadeMsshould callthemeTempoand leave one reader.defineTheme'smotionJSDoc, the theme template, the author-a-theme doc and thethemeMotion.tsheader still say the springs don't readtempo; update them asmotion.doc.mjswas.
The inline comments also carry two non-blocking wording suggestions. CI has not run on this draft yet.
[Automated review]
| if (role == null || role.kind === 'curve' || FIXED_ROLES.has(role.id)) { | ||
| return value; | ||
| } | ||
| const tempo = themeTempo(element); |
There was a problem hiding this comment.
This runs under every pointer, so a theme that sets tempo also retimes the glide for mouse users: SideNav's rail collapse and AppShell's content took 1,102 ms instead of 735 ms at tempo 1.5 with a mouse. That desktop behavior change needs an owner decision: gate the scaling to touch input, or cite the decision that covers it in the description.
There was a problem hiding this comment.
Kept on purpose, per the current records. AST-108 FR1 has tempo scale every motion duration, and FR3 scales every AST-094 FR4 role's spring. The glide plays the FR4 role springs.settle under every pointer (AST-105 FR4, FR5), and AST-107 FR3 says animate's spring follows the theme's tempo. The CSS tokens already follow tempo with a mouse, so the glide now keeps step with them. The description names this, and the browser spec now runs the glide with a mouse too (0.75, 1 and 1.5 times as long).
There was a problem hiding this comment.
Fair point — the description now cites the records behind this (AST-108 FR1/FR3, AST-105 FR4/FR5, AST-107 FR3), states the glide follows tempo with a mouse too, and the browser spec covers the glide with a mouse at 0.75×, 1× and 1.5×. That addresses the finding as written; the next full review will re-evaluate it against the updated description and spec.
| const text = view | ||
| .getComputedStyle(element) | ||
| .getPropertyValue(TEMPO_PROPERTY) | ||
| .trim(); | ||
| const tempo = text === '' ? Number.NaN : Number(text); | ||
| return Number.isFinite(tempo) && tempo > 0 ? tempo : 1; |
There was a problem hiding this comment.
pressFadeMs in utils/pressFeedback.ts (lines 158-161) already reads --_astryx-motion-tempo with this exact parse (empty to NaN, positive and finite, else 1). Have pressFadeMs call themeTempo(element) so the press release fade and the springs share one reader and cannot drift.
There was a problem hiding this comment.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Motion: a theme's `tempo` now times the springs of touch motion as well as its CSS durations. Each spring keeps its shape and takes `tempo` times as long, and so does the animated-layout glide. Finger tracking and busy loops stay as measured, a theme with only a duration scale leaves the springs at tempo 1, and the springs don't follow `character` yet. The motion doc says so. |
There was a problem hiding this comment.
"A theme with only a duration scale leaves the springs at tempo 1" holds only for a theme on its own. Nested inside a theme that sets tempo, it inherits --_astryx-motion-tempo. Its durations then follow its own scale, but its springs play the outer tempo (in Chromium, springs.settle 0.75 s next to its own 200 ms --duration-medium), against spec:AST-108 FR5. A layer that writes durations without a tempo of its own could write --_astryx-motion-tempo: 1, as resolveThemeValues.ts already does for the pre-tempo copies.
There was a problem hiding this comment.
Fixed in 6755a6a. A layer whose durations come from a duration scale alone now writes --_astryx-motion-tempo: 1 (resolveThemeValues.ts). It's tested on defineTheme themes and adaptation rules, and in the browser: a duration-scale theme nested in a tempo 1.5 theme glides at 1×. The changeset sentence now says so.
| { | ||
| type: 'prose', | ||
| text: "A theme sets how motion feels with two knobs in `defineTheme`'s `motion`. They drive the CSS motion tokens; the springs of touch motion don't read them yet. Every theme follows each person's Reduce Motion setting; no theme loosens it.", | ||
| text: "A theme sets how motion feels with two knobs in `defineTheme`'s `motion`. They drive the CSS motion tokens, and `tempo` also times the springs of touch motion; those springs don't follow `character` yet. Every theme follows each person's Reduce Motion setting; no theme loosens it.", |
There was a problem hiding this comment.
These still say the opposite: defineTheme.ts:296-297 ("touch motion's springs don't read it yet"), packages/cli/assets/theme.template.ts:184-185, packages/cli/assets/docs/author-a-theme.doc.mjs:44 and packages/core/src/theme/themeMotion.ts:16 ("No spring reads the knobs yet"). Please update them to match this sentence.
There was a problem hiding this comment.
Fixed in 6755a6a: defineTheme.ts, the theme template, author-a-theme.doc.mjs and the themeMotion.ts header now say the springs follow tempo but not character yet.
| * glide, the changed element and the indicator. Motion roles are internal | ||
| * (`spec:AST-094` DEC-11): no theme, scope or custom property changes it. | ||
| * The glide plays it at the tempo of the theme `area` is in. |
There was a problem hiding this comment.
Non-blocking: "no theme, scope or custom property changes it" now sits right before "The glide plays it at the tempo of the theme area is in." Suggest: "No page style sets the role. The glide plays it at the tempo of the theme that area is in."
| * with it | ||
| * | ||
| * Every touch motion passes through here, so the role a value plays is named | ||
| * in one place. A role plays its table value: the per-role tables stay |
There was a problem hiding this comment.
Non-blocking: "A role plays its table value" now contradicts lines 19-22, and the header is 25 lines. Suggest: "Nothing reads a role's own CSS variable, and no page or theme style sets a role."
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8686.
|
/approve-spec 9359e05 |
9359e05 to
6755a6a
Compare
There was a problem hiding this comment.
Changes requested. The springs, duration roles and layout glide now follow a theme's motion.tempo as spec:AST-108 FR3 describes, and last round's Switch release and nested duration-scale fixes check out. Two blocking findings remain:
- The glide's spring follows
tempounder every pointer (packages/core/src/motion/resolveMotion.ts:131-138, read byanimatedLayout.ts:145-148), so a mouse user's SideNav rail collapse, AppShell content and Collapsible glides are retimed (735 ms to 1,102 ms at tempo 1.5), and this desktop behavior change needs an owner decision: gate the scaling to touch input, or cite the decision that covers it. themeTempo(packages/core/src/motion/themeTempo.ts:34-39) copies the tempo read inpressFadeMs(packages/core/src/utils/pressFeedback.ts:158-161), so havepressFadeMscallthemeTempo(element)and keep one reader.- Non-blocking:
themeTempo.ts:12-13andthemeMotion.ts:84-88no longer match the duration-scale tempo write. - Miss:
animator.ts:159-161,animator.ts:2732,motion/README.md:59-60and the theme authoring contract's known-gap line still say a role plays its table value or that no spring reads the knobs (non-blocking).
CI has not run on this head yet.
[Automated review]
| if (role == null || role.kind === 'curve' || FIXED_ROLES.has(role.id)) { | ||
| return value; | ||
| } | ||
| const tempo = themeTempo(element); |
There was a problem hiding this comment.
This still scales the glide's spring under every pointer. With a mouse, Collapsible's glide takes 735 ms on the base and 1,102 ms at tempo 1.5 here, and SideNav's rail collapse and AppShell's content glide take the same path. The cited records are not an owner decision for this desktop behavior change. Gate the scaling to touch input, or cite the decision that covers it in the description.
There was a problem hiding this comment.
The decision that covers it is spec:AST-105 DEC-2 (decider cixzhang, 2026-11-04): "a theme that sets character or tempo sees its glides change with the rest of its motion, under every pointer." Its FR4 times the glide on --ease-move over --duration-medium-min, which follow tempo with a mouse too. Moving the glide from springs.settle to those tokens is its own pull request; until then the glide here keeps step with them at each tempo. The description now cites DEC-2.
There was a problem hiding this comment.
That resolves it — DEC-2 is exactly the owner decision this finding asked for, and the description now cites it verbatim, including “under every pointer.” The FR4 point also closes the consistency gap for now: the glide keeps step with --ease-move / --duration-medium-min, which already follow tempo with a mouse, and moving the glide onto those tokens is tracked as its own pull request. The next full review will re-evaluate the finding against the updated description.
| const text = view | ||
| .getComputedStyle(element) | ||
| .getPropertyValue(TEMPO_PROPERTY) | ||
| .trim(); | ||
| const tempo = text === '' ? Number.NaN : Number(text); | ||
| return Number.isFinite(tempo) && tempo > 0 ? tempo : 1; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Done in e058f49: pressFadeMs now reads the tempo through themeTempo(element) and keeps its Reduce Motion cap, so there is one reader.
| * A theme that sets `tempo` writes it, clamped, to the private custom | ||
| * property `--_astryx-motion-tempo` beside its duration tokens. An | ||
| * adaptation rule writes it where its condition holds, and a nested theme | ||
| * that sets no tempo inherits it. A duration scale alone writes none, so its |
There was a problem hiding this comment.
Non-blocking: resolveThemeValues.ts:334-335 now writes --_astryx-motion-tempo: 1 for a duration scale. Suggest: "A layer whose durations come from a duration scale writes a tempo of 1, so its springs keep its own pace."
6755a6a to
e058f49
Compare
There was a problem hiding this comment.
Approval waits on one owner decision about desktop motion; nothing else blocks.
- The layout glide still plays
springs.settle, and this change stretches it bytempounder every pointer (packages/core/src/motion/animatedLayout.ts:145-148), so mouse users' SideNav, AppShell and Collapsible glides are retimed (735 ms becomes 1,102 ms at tempo 1.5);spec:AST-105DEC-2 has glides followtempoon the curve tokens and never as a spring, so please cite the owner decision that covers this interim spring on desktop, with when it was given, or gate the glide's tempo scaling to touch input. - Non-blocking:
packages/core/src/theme/themeMotion.ts:84-88still says only a theme with atempowrites--_astryx-motion-tempoand only the press release fade reads it.
CI has not run on this head yet.
[Automated review]
| registerSpringsTable(); | ||
| const value = motionRole(ROLE_ID)?.value; | ||
| return isSpring(value) ? value : SETTLE_SPRING; | ||
| return resolveMotion(area, isSpring(value) ? value : SETTLE_SPRING); |
There was a problem hiding this comment.
This stretches the glide's springs.settle by the area's tempo under every pointer, so mouse users' SideNav, AppShell and Collapsible glides are retimed (735 ms becomes 1,102 ms at tempo 1.5). spec:AST-105 DEC-2 has glides follow tempo on --ease-move over --duration-medium-min, never as a spring, so please cite the owner decision that covers this interim spring on desktop, with when it was given, or gate the scaling to touch input.
There was a problem hiding this comment.
The owner's decision for this interim, given 2026-11-04 at 16:24 UTC: "Until the glide moves to the curve tokens (AST-105 DEC-2), its interim springs.settle follows tempo under every pointer." At tempo 1 nothing changes on desktop. The description now quotes it.
There was a problem hiding this comment.
That resolves it — the description now quotes the interim decision with its timestamp (2026-11-04, 16:24 UTC), covering the springs.settle glide following tempo under every pointer until it moves to the DEC-2 curve tokens, and notes that at tempo 1 nothing changes on desktop. That is exactly what the finding asked for. The next full review will re-evaluate the finding against the updated description.
A theme's `tempo` reached the CSS motion tokens but not the touch springs, so a slowed theme played its fades and its springs at two speeds. resolveMotion, the one door every touch spring and role duration passes through, now reads the tempo of the theme the element is in: the private property the theme writes beside its duration tokens. A spring keeps its damping ratio and takes tempo times as long, and a duration role scales. The layout glide resolves springs.settle through it at the area, under every pointer, as the CSS tokens already do. Finger tracking, ProgressBar's busy loop and curves stay as measured (spec:AST-108 FR4). The springs don't follow `character` yet; that part waits on its record. Switch's release settles on a curve it builds itself, so it now resolves its lift and settle springs and its minimum lift at the thumb, as its lift does. A theme layer whose durations come from a duration scale alone writes a tempo of 1, so a theme nested in one with a tempo keeps its springs at the pace of its own durations (FR5). Specs that set a tempo on the document now set it on the component, below the story's theme root. A `Core/Theme motion tempo` story shows three themes side by side for a feel check on a touch screen; it and a nested duration-scale story are the browser proofs' fixtures.
e058f49 to
50e47ea
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft from a fork, so once it is marked ready for review, a maintainer adds the ci:run label so CI runs.
A theme's tempo now times the role springs, the role durations and the layout glide, as spec:AST-108 FR3–FR5, spec:AST-105 FR4–FR5 and spec:AST-107 FR3 describe, and the glide's retiming under a mouse follows the owner's interim decision quoted in the description. There are no blocking findings.
Non-blocking:
docs/architecture/theme-authoring-contract.mdstill lists "no spring reads the motion knobs yet" as a known gap; that line needs updating once this lands.- The file headers of
resolveMotion.ts(25 lines) andthemeTempo.ts(18 lines) run past the guide of about 12 lines inarchitecture:knowledge-contractsINV29.
[Automated review]
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft from a fork, so once it is marked ready for review, a maintainer adds the ci:run label so CI runs.
A theme's tempo now times the role springs, the role durations and the layout glide, as spec:AST-108 FR3–FR5, spec:AST-105 FR5 and DEC-2, and spec:AST-107 FR3 describe, and the glide's retiming under a mouse follows the owner's interim decision quoted in the description. There are no blocking findings.
Non-blocking:
docs/architecture/theme-authoring-contract.mdstill lists "no spring reads the motion knobs yet" as a known gap; that line needs updating once this lands.- The file headers of
resolveMotion.ts(25 lines) andthemeTempo.ts(18 lines) run past the guide of about 12 lines inarchitecture:knowledge-contractsINV29. - The description cites
spec:AST-105FR4 for the glide's spring, but onmainFR4 puts the glide on the curve tokens, so the spring is that record's current-state gap rather than its requirement.
[Automated review]
Stack: #8677 → #8686
The theme's
characterstays out of this pull request: its spring tables wait on #9704. This one istempoonly.User need
A theme author sets
motion.tempoto slow down or speed up their product's motion. Today only the CSS transitions follow it. The touch springs keep Astryx's own timing: a sheet's settle, a switch's travel, a slider's release and the animated-layout glide. So with a tempo of 1.5, a screen's fades take 1.5 times as long while its sheet still settles at tempo 1.Problem and solution fit
Every touch spring and role duration goes through
resolveMotion(element, role), and that function passed each role through unchanged. It now reads the tempo of the themeelementis in and stretches the role:tempotimes as long.springs.settlethrough it, at the area.Current specification
spec:AST-108FR3 (current): "a spring's duration scales bytempo".spec:AST-094FR45 (current): "The theme'stemposcales the spring's duration".characteroffsets aren't here. The spec owner keeps them parked until spec(AST-108): the fluid character's motion tables #9704, which redefines them, lands.Public delta and caller burden
motion: {tempo: 1.5}now also plays every touch spring and role duration inside it 1.5 times as long, the glide included.--_astryx-motion-tempoonly when it setstempo, and the reader treats a missing value as 1.springs.fingerTrackingand Switch's drag tracking), ProgressBar's reduced-motion pulse, curves, and anything that isn't a role (momentum, the rubber band, paging, press clocks and thresholds).characteror a few explicit durations writes none.tempobut notcharacteryet: the motion doc,defineTheme'smotionJSDoc, the theme template and the author-a-theme doc.Design and alternatives
tempowith a mouse too, asspec:AST-105DEC-2 decides (cixzhang, 2026-11-04): "a theme that setscharacterortemposees its glides change with the rest of its motion, under every pointer." The owner's decision for the interim spring (2026-11-04, 16:24 UTC): "Until the glide moves to the curve tokens (AST-105 DEC-2), its interimsprings.settlefollows tempo under every pointer." At tempo 1 nothing changes on desktop. AST-108 FR1 hastemposcale every motion duration, and FR3 scales every AST-094 FR4 role's spring. The glide plays the FR4 rolesprings.settleunder every pointer (AST-105 FR4, FR5), and AST-107 FR3 saysanimate's spring follows the theme's tempo. The CSS tokens already followtempowith a mouse, so the glide now keeps step with them.--duration-exit, which the tempo already scales, so it needs no reader of its own.Evidence
themeTempo.interaction.browser.spec.ts, Chromium and WebKit on the phone profile:motion.tempo, the glide lasts 0.75, 1 and 1.5 times as long.resolveMotion.tempo.test.ts:themeTempo.test.tsreads the theme's value, and falls back to 1 when there is none or it isn't a positive number.animatedLayout.test.tschecks the glide at the area's tempo.useSwitchTouch.test.ts: a release right after a press waits the minimum lift and settles, all exactly 1.5 and 0.75 times as long at those tempos.themeMotion.test.ts: a theme and an adaptation rule with a duration scale alone write a tempo of 1; a child of a tempo theme keeps its base's tempo.35f25756c9), the Switch case reads a ratio of 1.0 and both theme cases find no tempo.a5b1282f64): 6 unit cases fail andthemeTempo.test.tshas no module to load. The browser scope case is 25 px off; finger tracking passes on both, as it should.Scope
Testing
Gate at
50e47ea46d(rebased ontomainafter the shared press release fade moved to--duration-exit), side by side withmainon one box:pnpm test:ui20,636 vs 20,622, 0 failed;pnpm test:node16,561 passed.main: in a same-moment pair of five runs each,mainfailed 5 and this change failed 4.Earlier, at
6755a6a5a8: the tempo spec three times, Chromium 15/15 and WebKit 15/15; ProgressBar's move and pulse specs 26/26.Feel check: open the
Core/Theme motion tempostory ("Three tempos") on a phone. In each row, toggle the switch, drag the slider, slide the segments, and open "Shipping details". The 1.5 row should feel half again as slow as the middle row, and the 0.75 row quicker. Each finger drag still tracks the finger.