motiondivision / motiondivision/motion

[BUG] Motion component is using incorrect motion value in animation after updating injected motion value reference

Open
#2,238 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
TypeScript
Stars
33.7k
Forks
1.4k
Avg merge
1d 10h
Merged PRs (30d)
14

Description

1. Read the FAQs 👇
Done, not applicable to this issue.

2. Describe the bug

  • I have a scenario where I am injecting a motion value x1 into one motion component box1, and a transformed motion value x2 into another motion component box2
  • At a certain condition I would like to swap which of the components that receives which of the motion values. E.g. x1 will be injected into box2, while x2 is injected into box1.
  • The problem that now arises is that box2 is seemingly still subscribing to and using values from x1. Which in turn causes box2 to be positioned incorrectly.
  • This issue seems to only appear when the motion values are animated.

3. IMPORTANT: Provide a CodeSandbox reproduction of the bug
Example in CodeSandbox

4. Steps to reproduce

Steps to reproduce the behavior:

  1. Go to Example in CodeSandbox
  2. Wait approximately 3 seconds for the steps to complete
  3. See that box2 is not transformed and positioned as expected in the example with animation, which should be at x = 0
  4. See the expected positions in the example that is without animation

5. Expected behavior

I expect the boxes in the animated example to position themselves, along the x axis, in the same manner as the example without animation.

6. Video or screenshots
Not applicable, see the example.

7. Environment details
Windows 11, Chrome 114.0.5735.199 (Official Build) (64-bit)

Potential issue and potential solution

To me it seems that this issue happens because of the following events:

Problem now is that there is two subscriptions for the same key, and it is not necessarily that both subscriptions are to the same motion value.
addValue avoids this issue by checking if there is an existing value for the same key and removes it, but mount does not have a similar check.

I was planning on submitting a pull request with the following changes, but the changes caused some of the tests to fail, so I concluded that I am likely missing something in the bigger picture:

diff --git a/packages/framer-motion/src/render/VisualElement.ts b/packages/framer-motion/src/render/VisualElement.ts
index 32485e2c..5a1ca2ff 100644
--- a/packages/framer-motion/src/render/VisualElement.ts
+++ b/packages/framer-motion/src/render/VisualElement.ts
@@ -403,7 +403,11 @@ export abstract class VisualElement<
             this.removeFromVariantTree = this.parent.addVariantChild(this)
         }
 
-        this.values.forEach((value, key) => this.bindToMotionValue(key, value))
+        this.values.forEach((value, key) => {
+            // Remove existing value subscription before adding a new binding.
+            this.unbindFromMotionValue(key)
+            this.bindToMotionValue(key, value)
+        })
 
         if (!hasReducedMotionListener.current) {
             initPrefersReducedMotion()
@@ -471,6 +475,14 @@ export abstract class VisualElement<
         })
     }
 
+    private unbindFromMotionValue(key: string) {
+        const unsubscribe = this.valueSubscriptions.get(key)
+        if (unsubscribe) {
+            unsubscribe()
+            this.valueSubscriptions.delete(key)
+        }
+    }
+
     sortNodePosition(other: VisualElement<Instance>) {
         /**
          * If these nodes aren't even of the same type we can't compare their depth.
@@ -779,11 +791,7 @@ export abstract class VisualElement<
      */
     removeValue(key: string) {
         this.values.delete(key)
-        const unsubscribe = this.valueSubscriptions.get(key)
-        if (unsubscribe) {
-            unsubscribe()
-            this.valueSubscriptions.delete(key)
-        }
+        this.unbindFromMotionValue(key)
         delete this.latestValues[key]
         this.removeValueFromRenderState(key, this.renderState)
     }
Potential workaround

Update the key in motion.div when swapping the motion values, to destroy the existing motion component with the subscription to the old value, and create a fresh new one that only has a subscription to the newly injected motion value.

See the commented out key property the motion.div at SetupExample.tsx in the CodeSandbox example.

This workaround may work for some, but is not a good workaround in my case as it will recreate all child elements and cause a "flash of content" that is visible to the user.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the linked CodeSandbox reproduction, then read packages/framer-motion/src/render/VisualElement.ts around mount, addValue, bindToMotionValue, and removeValue. Compare the animated and non-animated examples, and use SetupExample.tsx to understand the motion.div value swap. Done means the animated boxes reach the same x-axis positions without recreating the component, with the relevant tests passing.

Written by the indexing model from the issue text.

Assessment

Tech stack
react, typescript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.