Repository navigation
Conversation
… source rect createRectMorph in motion/rectMorph.ts, internal: present along a path and dismiss back onto a copy or the element, each continuing the other from the rect on screen. Rebuilt on main from the pull request's own change and ported to main's engine (createMotionAnimator). Timing follows the owner's rule for touch motion: present and dismiss take a motion, either a theme duration and easing (a tap, a key, the program) or a spring (a gesture's release, from its velocity). timedProgressCurve joins the progress helpers. architecture:react-component-runtime INV12 records the implementation gap: the size-change glide moves onto the morph next. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
/approve-spec 131ca14 |
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the rect morph's timing rule, and one fix is needed.
rectMorph.ts documents and implements a rule no current record states (a present or dismissal that a tap, a key or the program starts plays the theme's duration and easing from rest, and only a gesture's release plays a spring), which contradicts AST-094 FR31, where a touch menu grows on springs.menuOpen, and FR6, where a new target keeps the current velocity. An engineering owner either records the rule in AST-094 (amending FR31 and FR6) and approves that edit by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the morph carries the presented velocity into a timed retarget and its example and changeset follow FR31.
Blocking
- A dismissal onto a copy calls
attach(null)on a handed-in animator (rectMorph.ts:448), and neitherreset()nordispose()attaches it again, so the owner's ownsetandplaystop reaching the element; attach a handed-in animator again and add a test that the owner's write lands after a copy dismissal.
Everything else matches the current records, and the motion suites, the typecheck and the Chromium interaction spec pass.
[Automated review]
| // style write on an element that no longer paints, landing inside the | ||
| // dismissal's frames. The next present attaches the element again. | ||
| restoreElement(); | ||
| animator.attach(null); |
There was a problem hiding this comment.
Blocking. This detaches the animator even when the caller handed it in (createRectMorph(element, {animator})). reset() and dispose() never attach it again; only the next present does. After a close onto a copy, the owner's stage.set and stage.play stop reaching the element, and after dispose() they never reach it again. That breaks the JSDoc's promise that dispose leaves the animator to its owner. Attach a handed-in animator to the element again when the copy's dismissal ends and in reset(). Then test that the owner's write lands after a copy dismissal and dispose(); the existing test reads only stage.get('x').
| ) => | ||
| 'spring' in motion | ||
| ? progressCurve(resolveMotion(on, motion.spring), {velocity}) | ||
| : timedProgressCurve(motion.duration, motion.easing); |
There was a problem hiding this comment.
A timed motion drops the velocity that present and dismiss just projected, so a timed present that reverses a running dismissal starts from rest. AST-094 FR6 says a new target MUST keep the current velocity. Whether timed touch motion may start from rest is the open owner decision on this pull request. If the rule is not adopted, carry the projected velocity into the timed curve.
| * A present or a dismissal takes its timing as a motion: the theme's | ||
| * duration and easing for a motion that a tap, a key or the program starts, | ||
| * or a spring for a gesture's release, which starts at the gesture's | ||
| * velocity. Only a spring carries a velocity into the motion. | ||
| * | ||
| * Reduced Motion is the animator's: on a coarse | ||
| * pointer a present lands at rest and fades in, a dismissal fades out where | ||
| * it stands, opacity only, in 200 ms; on any other pointer the change | ||
| * completes at once. |
There was a problem hiding this comment.
Non-blocking (INV29): the file header runs 40 lines. These two paragraphs repeat the RectMorphMotion JSDoc and the Reduced Motion behavior the animator owns. Keep @input, @output, @position and SYNC:. Move the iOS quirk next to retire: "iOS Safari does not scroll a finger through an element that keeps a scale, even at 1. Clear it once the present settles."
| import type {SpringDescription} from './spring'; | ||
| import {resolveMotion} from './resolveMotion'; | ||
|
|
||
| // The paths the morph plays, where its callers have always imported them. |
There was a problem hiding this comment.
Non-blocking (INV29): no caller imports these paths from here yet, so "where its callers have always imported them" does not hold. Delete the comment; the re-export explains itself.
| // A path with no opacity of its own gives the animator's Reduced Motion | ||
| // cross-fade nothing to read, and it would start the motion on the source | ||
| // rect and dip. Such a path says which way it goes, so the cross-fade | ||
| // lands at rest on a present and fades out in place on a dismissal; a caller's own `crossfadeAs` still wins, and | ||
| // `'move'` keeps the animator's dip for an element that only moves. |
There was a problem hiding this comment.
Non-blocking (INV29): two sentences carry four topics, and one runs past 25 words. Suggested: "A path with no opacity gives the Reduced Motion cross-fade nothing to read. Treat it as an entrance on a present and an exit on a dismissal. A caller's crossfadeAs wins, and 'move' keeps the animator's dip."
…opy dismissal CodeBunny on #9769: a dismissal onto a copy detached the animator, and a handed-in one was never attached again, so its owner's writes stopped reaching the element. Only the morph's own animator lets go now. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the rect morph's timing rule, and two fixes are needed whatever that decision is.
The morph documents and implements a rule no current spec states: a present or dismissal that a tap, a key or the program starts plays the theme's duration and easing from rest, and only a gesture's release plays a spring. That contradicts AST-094 FR31, where a touch menu grows on springs.menuOpen, and FR6, where a new target keeps the current velocity. To settle it, an engineering owner either records the rule in AST-094 (amending FR31 and FR6) and approves that edit by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the morph carries the presented velocity into a timed retarget and its example and changeset follow FR31.
Blocking
timedProgressCurve(packages/core/src/motion/progress.ts:79-99) is a second copy of the timed curvesolveAnimatedPageScrollalready provides inpaging.ts, without that curve's velocity carry, so move that curve with its optional velocity intoprogress.tsand call it from the pager and the morph.- The INV12 edit needs the
## Writing rubric resultssection in the description and should end at "its own compositor translate.)" instead of the plan "it moves onto the morph next", as the knowledge-contracts writing rules require.
The handed-in animator now stays on the element through a copy dismissal, reset() and dispose(), and the motion suites, the typecheck and the Chromium interaction spec pass.
[Automated review]
| * `easing`: the curve a timed motion (a theme duration and easing) plays. | ||
| * Its velocity is in progress per second. | ||
| */ | ||
| export function timedProgressCurve( |
There was a problem hiding this comment.
Blocking (one shared path). solveAnimatedPageScroll in paging.ts already plays this curve: from 0 to 1 with the same duration and easing the two agree at every sample. It also keeps the velocity of the move it retargets ("A new target keeps the current velocity"), which this copy cannot, so the pager conforms to AST-094 FR6 where the morph drops the velocity. Move that curve, with its optional velocity, into progress.ts as the one timed progress curve and call it from paging.ts and rectMorph.ts.
| its own compositor translate. It never animates height. Its walk visits only | ||
| its own compositor translate. (Implementation gap: Core has the shared | ||
| rectangle morph, but the size-change glide still plays its own compositor | ||
| translate; it moves onto the morph next.) It never animates height. Its walk visits only |
There was a problem hiding this comment.
Blocking. The knowledge-contracts writing rules ask a record to describe the system in the present tense, not the project that produced it, so end the note at "its own compositor translate.)" and drop "it moves onto the morph next". The same rules ask the description to report ## Writing rubric results for a record change; it has none yet.
…er and the morph CodeBunny on #9769: solveAnimatedPageScroll already played this curve and keeps the velocity of the move it retargets. timedCurve in progress.ts is now that one curve; the pager and the morph's timed motion call it, so a timed present that interrupts a dismissal keeps its velocity. The INV12 note states the system only. File header and comments trimmed. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
|
/approve-spec e3a43d0 |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the rect morph's timing rule, and one fix is needed whatever that decision is.
The RectMorphMotion doc, its example and the changeset say that a present or dismissal a tap, a key or the program starts plays the theme's duration and easing, and only a gesture's release plays a spring; no current spec states that rule, and it contradicts AST-094 FR31, where a menu opened under a finger grows on springs.menuOpen, and FR4, where touch motion reads its named roles. To settle it, an engineering owner either records the rule in AST-094 (amending FR31 and FR4) and approves that edit by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the doc, the example and the changeset name the presentation's motion roles instead.
Blocking
- A timed present or dismissal that starts at rest is handed a velocity of 0 (
packages/core/src/motion/rectMorph.ts:351and:397-399), whichtimedCurvetreats as a retarget and uses to cancel the easing's starting slope, so a 250 ms present on the entrance curve is 48 px wide at 50 ms where the easing gives 114 px; pass no velocity when nothing is in flight, and test a present from rest on an easing that does not start flat, such as[0, 0, 0.58, 1], against the easing itself.
A timed retarget now keeps its velocity, the duplicated timed curve is gone, and the motion and pager suites and the typecheck pass.
[Automated review]
| const from = | ||
| exiting ?? (presenting != null && box != null ? {animator, box} : null); | ||
| let start = path; | ||
| let velocity = play.velocity ?? 0; |
There was a problem hiding this comment.
Blocking. With nothing in flight this passes a velocity of 0, and timedCurve treats any defined velocity as a retarget: it subtracts the easing's starting slope, so a timed present no longer plays the easing it is given. On timing.curves.entrance a 250 ms present from a 22 px square is 48 px wide at 50 ms where the easing gives 114 px. A dismissal from a settled present does the same (lines 397-399 project 0). Leave the velocity undefined when nothing moves, keep the projected one for an interruption, and test a present from rest on [0, 0, 0.58, 1] against the easing itself.
| * // A tap opened the menu: the theme's entrance duration and easing. | ||
| * morph.present(path, {duration: enterMs, easing: enterEasing}); |
There was a problem hiding this comment.
Pending an engineering owner's decision: AST-094 FR31 has a menu opened under a finger grow on springs.menuOpen, and FR4 has touch motion read its roles. This example (a tapped menu on the theme's entrance duration and easing) and the rule at lines 111-115 need either an AST-094 amendment or the menu's role here.
| * @position Motion foundation building block, internal to core: the one | ||
| * "grow out of a source rect, shrink back into it" that touch | ||
| * presentations share: an anchored menu, a popover, a context menu's | ||
| * lifted preview, a photo's zoom, a sub-menu's parent card |
There was a problem hiding this comment.
Non-blocking (INV29): the header runs 18 lines, past the record's limit of about 12. A one-line @position keeps it short, for example: 'Motion building block, internal to core: grow out of a source rect and shrink back into it.'
| // Leaves the element as laid out: identity, and none of the morph's inline | ||
| // properties (an opacity only when a path animated it). At rest it needs no | ||
| // compositing layer, so the values go in without promoting it. |
There was a problem hiding this comment.
Non-blocking (INV29): the first sentence restates restoreElement. Keep only the reason: 'At rest the element needs no compositing layer, so set the values without promoting it.'
| // A spring goes through `resolveMotion` on the element that plays it: a | ||
| // touch motion role resolves to its table value and reads no style, and a | ||
| // caller's bare spring comes back as given. A timed motion is the caller's |
There was a problem hiding this comment.
Non-blocking (INV29): the first sentence runs 37 words with three topics. Suggested: 'A spring goes through resolveMotion. A motion role resolves to its table value, and a bare spring comes back as given.'
| // of the layer its presentation promoted, in this task. Kept, the | ||
| // promotion would end its linger after the element's last motion: a | ||
| // style write on an element that no longer paints, landing inside the |
There was a problem hiding this comment.
Non-blocking (INV29): this sentence runs 28 words. Suggested: 'Otherwise the promotion's linger ends later, with a style write inside the dismissal's frames.'
| } | ||
| }); | ||
|
|
||
| it('presents on a theme timing: its duration and easing as given, from the velocity it is given', async () => { |
There was a problem hiding this comment.
This test recomputes its expectation with timedProgressCurve and uses [0.2, 0, 0, 1], which starts flat, so it passes whether or not the easing's start survives (component-test-sufficiency INV6). Add a present from rest on [0, 0, 0.58, 1] checked against cubicBezier(easing).
CodeBunny on #9769: with nothing in flight the morph handed timedCurve a velocity of 0, which it treats as a retarget and uses to cancel the easing's starting slope. Only a motion in flight hands its velocity on now; tests check a present from rest and a dismissal from a settled present against the easing itself. Header and comments shortened. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the rect morph's timing rule; nothing else blocks.
- The
RectMorphMotiondoc (packages/core/src/motion/rectMorph.ts:109-115), its example (:214-215, a tapped menu on the theme's entrance duration and easing) and the changeset say that a present or dismissal a tap, a key or the program starts plays the theme's duration and easing, and only a gesture's release plays a spring; no current spec states that rule, and it contradicts AST-094 FR31, where a menu opened under a finger grows onsprings.menuOpenand shrinks onsprings.menuClose, and FR4, where touch motion reads the role named for its use.
To settle it, an engineering owner either records the rule in AST-094 (amending FR31 and FR4) and approves that edit by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or the doc, the example and the changeset name the presentation's motion roles instead. The earlier /approve-spec comments name earlier heads, so the INV12 note needs that same exact-head approval.
A timed motion from rest now plays the easing it is given, and the new tests fail without the fix. Non-blocking: the description's Testing line still says the unit suite has 35 cases including a timed present that ignores a given velocity; rectMorph.test.ts has 24 cases and the one at line 276 proves the opposite.
[Automated review]
Supersedes #8329 by @vjeux, who stays the author of this commit. Stacked on #9764 (the morph paths) and based on its branch, so it retargets to
mainwhen #9764 merges. #8329's branch carries the open chain under #8909, #8340 and #8360, about 30,000 lines. This pull request is #8329's own change, rebuilt onmainand ported tomain's engine (createMotionAnimator; #8340 was absorbed intomain).Maintainer impact
Five touch presentations grow a surface out of what the finger touched and shrink it back: menus, Popover, the lifted preview, Lightbox's zoom, and a sub-menu's parent card. Without one module, each would write its own version and get interruptions and Reduced Motion right on its own.
Intended invariant
No behavior change: the module is internal, and no component imports it yet.
present(path, motion)anddismiss(path, motion, {target})take aRectMorphMotion: either{duration, easing}, the theme's tokens resolved by the caller, for a motion a tap, a key or the program starts, or{spring}for a gesture's release. Either starts at the velocity it is given, by default the presented velocity, so a new target keeps the current velocity (spec:AST-094FR6). The type steers callers to the rule. The doc example shows a tap-opened menu on the theme's entrance timing.timedCurveinprogress.tsis the curvesolveAnimatedPageScrollplayed, moved there with its retargeted velocity. The pager and the morph both call it; the pager's curve is unchanged.architecture:react-component-runtimeINV12 says the size-change glide plays through the shared rectangle morph once Core has it. Now that Core has it, INV12 records an implementation gap: the glide still plays its own compositor translate. Moving it onto the morph is a separate, later pull request.Change
motion/rectMorph.ts, internal:createRectMorphpresents along a path (lerpPath,edgeGrowPath, or a caller's own) and dismisses back onto a target the caller gives (a copy, or the element). Each interrupts the other from the rect on screen. Nothing stays inline once a present settles, and Reduced Motion is the animator's.motion/progress.ts:timedCurve(moved frompaging.ts) andtimedProgressCurve, the progress a timed motion plays, keeping a retargeted velocity.motion/paging.ts:solveAnimatedPageScrollcallstimedCurve.architecture:react-component-runtimeINV12: the implementation-gap sentence above.[chore]changeset.Evidence
createAnimator, whichmaindoesn't have.tscis clean, and the motion and pager suites pass (994 tests).Scope
Testing
pnpm exec vitest run packages/core/src/motion packages/core/src/hooks/usePager.test.ts: 994 passed.pnpm exec tsc -p packages/core --noEmit: clean. Strict ESLint on the changed files,pnpm check:knowledgeandpnpm check:changesets: pass.rectMorph.interaction.browser.spec.ts) runs in CI's interaction job.Writing rubric results
Record changed:
architecture:react-component-runtimeINV12, one implementation-gap sentence.#number,/pull/or/issues/.