Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AnimatePresence won't unmount fastly changing content #1569

Closed
wants to merge 9 commits into from

Conversation

JaeSeoKim
Copy link
Contributor

@JaeSeoKim JaeSeoKim commented Jun 3, 2022

Description

exiting object is not enclosed for useRef() as shown below

-   const exiting = new Set<ComponentKey>()
+   const exiting = useRef(new Set<ComponentKey>()).current

Because the object exiting is a new object each time it is re-rendered, the onExit event validates the Defer re-rendering untill all exiting child have indented left incorrectly.

const onExit = () => {
    allChildren.delete(key)
    exiting.delete(key)

    // Remove this child from the present children
    const removeIndex = presentChildren.current.findIndex(
        (presentChild) => presentChild.key === key
    )
    presentChildren.current.splice(removeIndex, 1)

    // Defer re-rendering until all exiting children have indeed left
    if (!exiting.size) {
        presentChildren.current = filteredChildren

        if (isMounted.current === false) return

        forceRender()
        onExitComplete && onExitComplete()
    }
}

2022-06-11 update

A bug occurred because the onExit function is newly defined each time when a fastly outgoing elements unmount instantly.

To solve this problem, solved it by memoizing exitingChild.

// If the component was exiting, reuse the previous component to preserve state
let extingChild = exitingChildren.get(key)
if (extingChild) return extingChild

const onExit = () => {
    ...
}

extingChild = (
    <PresenceChild
        key={key}
        isPresent={false}
        onExitComplete={onExit}
        custom={custom}
        presenceAffectsLayout={presenceAffectsLayout}
    >
        {child}
    </PresenceChild>
)
exitingChildren.set(key, extingChild)
return extingChild

But when I solved this problem, I saw a different problem.

The rendering array will have a strange change position.

Generally, if a change is made from ["A", "D", "E", "F", "B", "C"] to ["1", "A", "2", "B", "3", "C"] during animation,
it is expected to be ["1" ,"A" ,"D" ,"E" ,"F" ,"2" ,"B" ,"3" ,"C"]

It does not work as intended and is output in the form ["1", "D", "E", "F", "A", "2", "B", "3", "C"].

// unnatural position
const insertionIndex = presentKeys.indexOf(key)

const onExit = () => {
  ...
}

// If the current array is smaller than "insertionIndex", it will not be inserted in the intended location
childrenToRender.splice(
    insertionIndex,
    0,
    <PresenceChild
      ...
    </PresenceChild>
)

If a change occurs in the rendering array, insert the chunk where the change occurred in the previous location.

 presentChildren  ->  children
     [A]                 [1]
     [D]                 [A]
     [E]                 [2]
     [F]                 [B]
     [B]                 [3]
     [C]                 [C]

  init ->  animate -> Exit Complete

             [1]        [1]     <--- targetChunk - 1
   [A]       [A]        [A]     <--- preservingKey
   [D]       [D]
   [E]       [E]                <--- presentChunk - 1
   [F]       [F]
             [2]        [2]     <--- targetChunk - 2
   [B]       [B]        [B]     <--- preservingKey
             [3]        [3]     <--- targetChunk - 3
   [C]       [C]        [C]     <--- preservingKey

Demo

original
2022-06-11.1.52.25.mov
fixed:
2022-06-11.1.36.41.mov
original
2022-06-11.1.54.32.mov
fixed:
2022-06-11.1.37.11.mov

fixed: #907
fixed: #1439
fixed: #1534

@JaeSeoKim

This comment was marked as outdated.

@mattgperry
Copy link
Collaborator

Hey thanks for taking a look at this! However I think this approach replaces the bug with another - fastly outgoing elements unmount instantly, rather than waiting for their animation to finish.

@JaeSeoKim JaeSeoKim force-pushed the fix/animatepresence branch from 4229490 to 27c2997 Compare June 10, 2022 13:22
@JaeSeoKim
Copy link
Contributor Author

@mattgperry I fixed it again for that problem. Can I review it again?

@mattgperry
Copy link
Collaborator

@JaeSeoKim thanks for taking another look at this. Are these sandboxes updates with the latest fixes?

@JaeSeoKim
Copy link
Contributor Author

@mattgperry I updated the sandbox samples.

I would like to contribute after hearing feedback on additional problems or improvements, but I think it will be difficult to contribute additionally as I will join the army from the 27th due to the military service obligation of the Republic of Korea.

@JaeSeoKim
Copy link
Contributor Author

@mattgperry Now, I can work after work. Is there anything that needs to be updated or improved?(Even though the development environment is not good 😂)

@JaeSeoKim JaeSeoKim force-pushed the fix/animatepresence branch from 8ff751d to 4348966 Compare August 17, 2022 14:49
@JaeSeoKim
Copy link
Contributor Author

@avkvak
Copy link

avkvak commented Sep 2, 2022

@JaeSeoKim Thank you for this fix!
Is there any chances to review/merge it?

@JaeSeoKim JaeSeoKim closed this Sep 26, 2022
@utileetdulce
Copy link

@JaeSeoKim Why did you close this pull request?
@maintainers, what are your plans concerning this issue ?

@utileetdulce
Copy link

@mattgperry
I have tested @JaeSeoKim commit 29c4a5bd and even wrote a test but don't understand what you meant with your comment concerning missing animations. Shall I create a pull request including the test i wrote or can you explain under what conditions "fastly outgoing elements unmount instantly, rather than waiting for their animation to finish"?

@JaeSeoKim
Copy link
Contributor Author

JaeSeoKim commented Nov 6, 2022

@utileetdulce I don't have enough time to work because of my mandatory military service in South Korea. and two fixes are included in one PR and i couldn't write the test codes, so i closed it with the intention of uploading it again later.

A problem with commit 29c4a5b occurs when you wrap PresenceChild for the exiting component.

Because onExit objects are newly declared at every rendering, React detects changes in Props and treats them as different components. This means that the previously rendered onExit is operated by componentdidmount and is immediately deleted from the rendering tree. So I wrote a code that memorizes and solves the component through allChildren.

https://github.com/framer/motion/blob/main/packages/framer-motion/src/components/AnimatePresence/index.tsx#L180-L224

@rafaelrcamargo
Copy link

Hey there,

I was actually having the same issue with AnimatePresence not removing elements properly when multiple elements were removed in a short amount of time. However, I was able to fix the issue by using the usePresence hook from the Framer Motion documentation.

The usePresence hook provides a way to animate elements that are entering and exiting a component. It works by keeping track of the presence of the element using a boolean value, and then triggering an animation when the element enters or exits the component.

By using usePresence in conjunction with AnimatePresence, I was able to ensure that elements were properly removed even when multiple elements were removed in a short amount of time.

I highly recommend checking out the Framer Motion documentation on usePresence for more information and examples: https://www.framer.com/motion/animate-presence/#usepresence

Hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants