Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions packages/framer-motion/cypress/integration/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,54 @@ describe("Layout animation", () => {
})
})

it(`It correctly fires layout="x" animations, only animating the x axis`, () => {
cy.visit("?test=layout&type=x")
.wait(50)
.get("#box")
.should(([$box]: any) => {
expectBbox($box, {
top: 0,
left: 0,
width: 100,
height: 200,
})
})
.trigger("click")
.wait(50)
.should(([$box]: any) => {
expectBbox($box, {
top: 100,
left: 100,
width: 200,
height: 300,
})
})
})

it(`It correctly fires layout="y" animations, only animating the y axis`, () => {
cy.visit("?test=layout&type=y")
.wait(50)
.get("#box")
.should(([$box]: any) => {
expectBbox($box, {
top: 0,
left: 0,
width: 100,
height: 200,
})
})
.trigger("click")
.wait(50)
.should(([$box]: any) => {
expectBbox($box, {
top: 50,
left: 200,
width: 300,
height: 250,
})
})
})
Comment on lines +392 to +438
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test coverage for axis constraints with layoutId (shared layout)

The PR description states the feature "Works with both layout and layoutId", but both new Cypress tests use a plain layout="x" / layout="y" element without layoutId. The code path for shared layout transitions (isShared = true) takes a different branch inside notifyLayoutUpdate, and — as noted in the logic comment above — the if (isShared) block appears to contain a bug specific to that path.

Consider adding a test case that uses layout="x" (or "y") together with layoutId to verify that the snap axis truly snaps while the animated axis transitions correctly, and to prevent regressions in the shared-layout flow.


it("Disabling crossfade works as expected", () => {
cy.visit("?test=layout-crossfade")
.wait(50)
Expand Down
6 changes: 3 additions & 3 deletions packages/motion-dom/src/layout/LayoutAnimationBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,17 @@ interface LayoutElementRecord {
}

interface LayoutAttributes {
layout?: boolean | "position" | "size" | "preserve-aspect"
layout?: boolean | "position" | "size" | "preserve-aspect" | "x" | "y"
layoutId?: string
}

type LayoutBuilderResolve = (animation: GroupAnimation) => void
type LayoutBuilderReject = (error: unknown) => void

interface ProjectionOptions {
layout?: boolean | "position" | "size" | "preserve-aspect"
layout?: boolean | "position" | "size" | "preserve-aspect" | "x" | "y"
layoutId?: string
animationType?: "size" | "position" | "both" | "preserve-aspect"
animationType?: "size" | "position" | "both" | "preserve-aspect" | "x" | "y"
transition?: Transition
crossfade?: boolean
}
Expand Down
2 changes: 1 addition & 1 deletion packages/motion-dom/src/node/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ export interface MotionNodeLayoutOptions {
*
* @public
*/
layout?: boolean | "position" | "size" | "preserve-aspect"
layout?: boolean | "position" | "size" | "preserve-aspect" | "x" | "y"

/**
* Enable shared layout transitions between different components with the same `layoutId`.
Expand Down
12 changes: 12 additions & 0 deletions packages/motion-dom/src/projection/node/create-projection-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2119,6 +2119,18 @@ function notifyLayoutUpdate(node: IProjectionNode) {
axisSnapshot.min = layout[axis].min
axisSnapshot.max = axisSnapshot.min + length
})
} else if (animationType === "x" || animationType === "y") {
const snapAxis = animationType === "x" ? "y" : "x"
const axisSnapshot = isShared
? snapshot.measuredBox[snapAxis]
: snapshot.layoutBox[snapAxis]
axisSnapshot.min = layout[snapAxis].min
axisSnapshot.max = layout[snapAxis].max

if (isShared) {
snapshot.measuredBox[snapAxis].min = layout[snapAxis].min
snapshot.measuredBox[snapAxis].max = layout[snapAxis].max
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant assignment in the shared-layout branch

When isShared is true, axisSnapshot is already a reference to snapshot.measuredBox[snapAxis] (line 2124–2126). The subsequent if (isShared) block therefore writes the exact same values back to the exact same object — it is a no-op.

The intention was likely to also zero out the snap axis on snapshot.layoutBox, which drives the layoutDelta calculation at line 2157:

calcBoxDelta(layoutDelta, layout, snapshot.layoutBox)

Because snapshot.layoutBox[snapAxis] is never updated for shared elements, layoutDelta on the snap axis will remain non-zero for layoutId-based (shared) transitions. The visual animation still appears correct because visualDelta is computed from measuredBox, but the stale layoutDelta could affect downstream didUpdate consumers and the hasLayoutChanged gate in unexpected ways.

The fix should update layoutBox in the isShared branch:

if (isShared) {
    snapshot.layoutBox[snapAxis].min = layout[snapAxis].min
    snapshot.layoutBox[snapAxis].max = layout[snapAxis].max
}

} else if (
shouldAnimatePositionOnly(animationType, snapshot.layoutBox, layout)
) {
Expand Down
2 changes: 1 addition & 1 deletion packages/motion-dom/src/projection/node/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ export interface ProjectionNodeOptions {
layoutRoot?: boolean
alwaysMeasureLayout?: boolean
onExitComplete?: VoidFunction
animationType?: "size" | "position" | "both" | "preserve-aspect"
animationType?: "size" | "position" | "both" | "preserve-aspect" | "x" | "y"
layoutId?: string
layout?: boolean | string
visualElement?: VisualElement
Expand Down