Repository navigation
fix(core): an element whose transform leaves it in place still glides - #8780
Conversation
The animated layout leaves an element the app moves itself to take its place at once: its own transform, translate, rotate or scale, or a running animation of one. It counted values that leave the element in place as such a move, so a component that keeps a transform for a gesture or a press jumped instead of gliding: a resting Button (scale(1), computed as matrix(1, 0, 0, 1, 0, 0)), a pressed one (scale(0.98), which then jumped onto the button gliding beside it), and a row with swipe actions at rest (translate3d(0, 0, 0)). A value that changes nothing, or only scales the element in place, is no move now; a glide's translate composes with it. A row a swipe holds open, or anything translated, rotated or skewed, is still the app's. Red first: the followers that keep a transform leaving them in place glide; the module before it left them all where they landed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
…the List updates change
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent 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 RegressionStatus: No visual change across 6 compared shot(s). Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: counting identity transforms as in place fixes the resting Buttons and swipe rows, but a follower with its own scale now glides by its shift divided by that scale, because the glide's scale correction (scaleOf, applied at packages/core/src/motion/animatedLayout.ts:896-899) reads the element's own scale as if an ancestor applied it. In Chromium, on an 80 px downward shift, a scale(0.98) follower starts 1.63 px above its old place and a scale(0.5) follower starts 80 px above it, which contradicts spec:AST-105 FR3. Please take that correction from the follower's containing box (or divide its own scale out) and add a browser proof with a self-scaled follower, since the jsdom unit test always sees a scale of 1. A smaller gap: staysInPlace treats a none keyframe as a move, so a follower whose own transition runs from transform: none to a scale still jumps.
[Automated review]
| if (property === 'scale') { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Admitting every scale sends self-scaled followers to the glide, where the unchanged scaleOf (line 896) divides their travel by their own scale as if an ancestor applied it. In Chromium, on an 80 px downward shift, a scale(0.5) follower starts 80 px above its old place and a pressed Button (scale(0.98)) 1.63 px above it. Taking the correction from the follower's containing box, or dividing its own scale out, keeps the glide starting at its old place.
| // or as written. | ||
| const functions = [...value.matchAll(/([a-z0-9]+)\(([^)]*)\)/gi)]; | ||
| return ( | ||
| functions.length > 0 && |
There was a problem hiding this comment.
none reaches here from an app animation's keyframes (lines 313-316) and returns false, so a follower whose own transition runs from transform: none to a scale still counts as moved; rotate: none does the same at lines 240-241. Treating none as in place for every property would match the static check at line 294.
…ection A follower whose own scale or transform leaves it in place now glides, but the glide's scale correction read that scale as an ancestor's: an element's measured size against its layout size includes its own `scale` and `transform`, while a glide's `translate` applies first, in the parent's space, and does not go through them. On an 80 px shift a `scale(0.98)` follower started 1.63 px above its old place and a `scale: 0.5` one 80 px above it. The correction now divides the follower's own scale out, so only its ancestors' scale is corrected. A keyframe with no transform (`none`) now counts as in place too, so a follower whose own transition runs from `transform: none` to a scale (a press starting) glides. Red first: the browser case (a `scale(0.98)` follower, a `scale: 0.5` follower and one mid-press start at their old places) fails on the change before this one in Chromium and WebKit, each half on its own; the unit case for the `none` keyframe fails too.
|
Thanks, both fixed in c0cadbe:
|
…e fix: a real conflict in animatedLayout.ts # Conflicts: # packages/core/src/motion/animatedLayout.ts
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
|
Posted by Cindy's agent on her behalf. Thanks, the fix looks right. #8734 (the in-place swap) just merged and touches the same lines in |
… fix, as asked on the review
|
/approve-spec b44dc18 |
There was a problem hiding this comment.
Changes requested: counting identity and scale-only transforms as in place now carries resting and pressed Buttons and resting swipe rows with the glide, but ownScaleOf keeps the sign of a negative scale property (packages/core/src/motion/animatedLayout.ts:816-821), so a follower at scale: 1 -1 or scale: -1 starts its glide on the far side of its new place (in Chromium, at 0px 80px instead of 0px -80px on an 80 px downward shift), which contradicts spec:AST-105 FR3. Taking the magnitude there (x *= Math.abs(sx); y *= Math.abs(sy);, as the matrix path already does) fixes it, and a scale: -1 follower in the browser case at packages/core/src/motion/__tests__/animatedLayout.interaction.browser.spec.ts:372 would pin it.
[Automated review]
| const [sx = 1, sy = sx] = numbersIn(scale); | ||
| x *= sx; | ||
| y *= sy; |
There was a problem hiding this comment.
A negative scale keeps its sign here, unlike the Math.hypot path below, so scaleOf returns -1 on a flipped axis and the glide's start offset (dy / scale.y, line 986) changes sign: in Chromium a scale: 1 -1 follower starts at 0px 80px on an 80 px downward shift instead of 0px -80px. x *= Math.abs(sx); y *= Math.abs(sy); fixes it; with that change the flipped followers start at their old place and the module's browser spec still passes.
The glide's own-scale correction kept the sign of a negative `scale` property, so a follower at `scale: -1` or `scale: 1 -1` started its glide on the far side of its new place (at 0px 80px instead of 0px -80px on an 80 px downward shift). A negative scale flips the element in place and its measured box stays positive, so the correction now takes the scale's magnitude, as the transform-matrix path already does. Red first: the browser case's `scale: -1` follower started at +80 px on the change before this one, in Chromium and WebKit.
|
Thanks, fixed in ad3f5e1: the own-scale correction now takes the scale's magnitude, as the matrix path already did, so a flipped follower glides the right way. I added |
|
/approve-spec ad3f5e1 |
There was a problem hiding this comment.
Changes requested: counting identity and scale-only transforms as in place now carries resting and pressed Buttons, resting swipe rows and flipped followers with the glide from their old place, but staysInPlace reads an unresolved var() in an app's own Web Animation keyframes as zero (packages/core/src/motion/animatedLayout.ts:260 and :300). A follower running translate: var(--from), translateY(var(--from)) or rotate(var(--angle)) keyframes is therefore glided, and the glide's translate masks the app's own animation (in Chromium a 120 px slide-in shows the glide's -35px at 150 ms instead of the app's +105px), contrary to spec:AST-105's rule that elements the app moves itself take their final place at once. Counting a value that still holds var() as the app's (for example if (value.includes('var(')) return false; after the scale branch) and adding a unit case with a var() keyframe would fix it.
[Automated review]
| return true; | ||
| } | ||
| if (property === 'translate') { | ||
| return numbersIn(value).every(n => n === 0); |
There was a problem hiding this comment.
A Web Animation reports its keyframes as written, so an app's translate: var(--from) keyframe reaches this branch with no digits, [].every(...) is true, and the keyframe reads as zero. Line 300 does the same for translateY(var(--from)), rotate(var(--angle)) and translate3d(var(--x, 0px), 0, 0) (only the fallback's 0 is read). In Chromium such a follower now gets the glide, whose translate masks the app's own slide: at 150 ms it sits at -35px instead of the app's +105px. Counting a value that still holds var() as the app's, for example if (value.includes('var(')) return false; after the scale branch, fixes both.
A follower running its own Web Animation whose keyframes hold an unresolved custom property (`translate: var(--from)`, `translateY(var(--from))`, `rotate(var(--angle))`) read as moving nothing, since its value had no numbers, so the glide's `translate` took over from the app's own animation (in Chromium a 120 px slide-in showed the glide's -35px at 150 ms instead of the app's +105px). A value that still holds `var()` is unknown here, so it now counts as the app moving the element, which takes its final place at once (spec:AST-105). A scale stays in place whatever its value. Red first: the unit case (three var() followers left to the app, a var() scale gliding) fails on the change before this one.
|
Thanks, fixed in 415fe17: a keyframe value that still holds |
There was a problem hiding this comment.
Recommending approval: resting and pressed Buttons and resting swipe rows now glide from their old place with the content after a Collapsible or TreeList change, and a follower whose own Web Animation uses var() keyframes is again left to the app, which settles the last requested change. In Chromium, Back, a pressed Continue and a resting swipe row start at the plain rows' 0px -80px offset, app animations with var() keyframes play unmasked, and the new unit and browser cases fail without the change. Nothing more is needed from you before a maintainer's decision.
[Automated review]
…dates base: a real conflict in the animated layout module # Conflicts: # packages/core/src/motion/animatedLayout.test.ts # packages/core/src/motion/animatedLayout.ts
…8723) into the List updates change # Conflicts: # packages/core/src/motion/animatedLayout.boundary.test.ts
User impact
With an
animatedLayouthandle (#8568), the content after an opening Collapsible, an expanding TreeList row or a changing List glides to its new place. Buttons in that content jumped instead, and so did every row with swipe actions. The button you just pressed jumped onto the one gliding beside it.Problem and solution fit
What led to it: this is part of a program to make Astryx's list components behave like iOS tables on phones. Building List's row updates on the handle (
component:ListFR12, #8106), the browser proofs showed the rows and a caption gliding while the Buttons after the list jumped, and a swipe list's rows didn't glide at all. I first offered this fix as a branch on #8634 (comment); #8634 has landed without it, so it comes here on its own.Why: the module leaves an element the app moves itself to take its new place at once, judged by the element's own
transform,translate,rotateandscaleand their running animations. It counted values that leave the element in place as moves:scale(1)for its press, computed asmatrix(1, 0, 0, 1, 0, 0);scale(0.98);translate3d(0, 0, 0).Options:
scale(0.98)) then jumps onto the one gliding beside it (identity values only, this change).translatecomposes with it.Expected behavior and authority
spec:AST-105FR3: the content after a change glides. Its Coverage leaves out only elements the app moves itself.Smallest restoration
movedByAppignores a value that leaves the element in place. Anything translated, rotated or skewed, a swiped-open row included, stays the app's. No API change.It merges cleanly today with the open #8723, #8734, #8639 and #8656; if one of them lands first and conflicts, I'll rebase this, never the other way around.
Evidence
rotate(2deg)andtranslate: 4px 0take their place at once), and a swiped row and a pressed-and-moved one in the new test.animatedLayout.test.ts, "glides a follower whose transform leaves it in place".Scope
Testing
What was run: the module's unit tests, the motion, Collapsible and TreeList unit tests, typecheck, and the module's browser specs in Chromium (56) and WebKit (56). On #8106's branch, the List browser proofs with every button and swipe rows fail without this change (3 of 9) and pass with it. All pass here; the new test failed first.