Fix incorrect Reorder.Item order calculation

[obeattie]

Problem

I've been experiencing an issue with Reorder lately, where items can become "stuck" while being dragged. It's a little hard to describe in words, but here's a recording of an example from an app I am building:

As you can see here, the "Sydney" item is first dragged over "Vancouver", and then "London" without a problem. But when "Vancouver" is then dragged backwards over "London", "Vancouver" also moves, even though the user hasn't moved the cursor that far to the left yet. As the dragging continues, it becomes totally impossible to reorder the items. This is reliably reproducible in my example, but not in more trivial examples, leading to much frustration.

Cause

The underlying issue is that Reorder.Group has an incorrect view of the current order of its items on the page. To determine that order, relies on the layout measurements it is passed by its child Reorder.Items via a registerItem callback. But the code in Reorder.Item to pass the layout measurements has a race condition. The intended flow seems to be:

1. Layout measurements are taken and passed to the items' onLayoutMeasure handler
2. The handler stores the measurements in a ref
3. An effect reads the measurements from the ref on each render and invokes the registerItem callback

But there is a race condition between 1 and 3, and sometimes 3 can execute before 1. I'm not familiar enough with React's internals to understand why the two hooks execute in a non-deterministic order, but they do. And when that happens, stale layout values from the last render are passed to registerItem and the Reorder.Group gets an incorrect order.

Solution

Instead of using a ref to temporarily hold the layout information, I'm just invoking registerItem directly in the onLayoutMeasure callback.

With this patch, the issue disappears and the reordering works as you would expect:

I've also tried this fix with the example in #2101. I can reproduce that issue pretty easily with framer-motion 10.12.21, and it seems fixed with this patch. Perhaps some of the other open Reorder bugs can be attributed to this, too. (Fixes #2101).

Notes

There aren't any comments in the code to explain why the extra indirection with the ref is necessary, and the commit that introduced the code doesn't contain any helpful description either. Being unsure why that choice was made, I'm left wondering if it was meant to solve a problem I am now overlooking. But, things seem to work just as well as before with my change, and the bug is fixed too.

[nick-keller]

Awesome work, thx!

[nick-keller]

Hey @mattgperry,
This looks like a fix to an impacting issue, with minimal footprint on existing code. What is the status on this, anyway we can help?

[chuganzy]

Let me share what I found for now.

I was investigating another race condition where transitionEnd styles are applied after new animation starts and accidentally overrides the latest motion values.

It seems to be introduced in #2025. Before it lands, calling stop() (which is also invoked when new animation begins) never completes animation Promises, which are used to apply the transitionEnd styles at the end, so there was no race condition..

I did not dig into how it is related to Reorder, but rolling back it somehow seems to solve this issue.

UPDATE: I could reproduce this Reorder issue on v10.5.0 where #2025 is not landed yet, so the issue seems to be not related. Sorry for the confusion here! 😅

[obeattie]

That's great sleuthing 🕵️‍♂️

In my opinion it would still be a good thing to remove this indirection from the code anyway, regardless of what causes the execution of the two hooks to be reordered. Passing the layout measurements between two hooks via a ref seems unnecessary and makes the code brittle to an order of execution that it doesn't control.

(Which is not at all to say that tracking down the cause of what you mention is not worthwhile as well. It sounds like fixing that would have wider benefits to other APIs as well. 😀)

[chuganzy]

I agree that this PR's approach looks more robust. One thing I can come up with regarding the existing approach is for the better performance? but not quite sure..😅

While I was investigating it further, I found that Reorder got unstable after #1993 is merged (v10.2.4), and another fix is needed to make it more stable.

It happens randomly when you drag an item, stop the cursor for a while, and drop it as attached bellow:

reorder-stuck.mov

I have confirmed that this still happens on this PR, and the fix will be soon ready to submit!

UPDATE: #2264 is the one.

[obeattie]

@chuganzy: I've tried your patch from #2264. For clarity, I don't think it fixes the same issue that is described here. The inertial animation code you have modified is only invoked when an item is dropped (ie. on mouseup), but the issue I have tried to fix happens while the item is being dragged (ie. while the mouse button is down).

[chuganzy]

Ah, my bad, I was not clear enough🙏 Yes, this PR is still needed. Mine only fixes the end of the animation as you mentioned.

[nick-keller]

Hey @mattgperry,
Anything we can do to help you merge this PR?

[robertotcestari]

Any updates here?

[nick-keller]

Are there any blockers to merge this PR?

[obeattie]

I messaged the maintainer on Twitter (fine, X). The hold-up is that this PR isn't mergeable without an integration test.

I don't have time immediately to work on this, but I'll try in the next few weeks.

Alternatively others could feel free to try and write this.

[nick-keller]

Given that this PR fixes what I would consider a major bug on an important feature of the library, and that writing an integration test for this looks very tricky (I have no idea how to approach it at least) maybe we should merge this soon?

Or maybe @obeattie or @mattgperry can give it another shot?

It has been almost 3 months that the fix was proposed, it feels wasteful not to merge it <3
I know how hard it is to manage such a project, I don't want to sound pushy, I'm just not able to help as I would love to at this point...

[thewildandy]

It would be great to see this move forward, it's a shame to not see the benefit of any of the work being done on motion due to such a major feature being broken despite a solution existing. Appreciate @nick-keller for persisting with this 🙌

[chuganzy]

Hi! JFYI: I just opened a PR that cherry-picks the changes here, and adds integration test!
#2359

[mattgperry]

Merged via #2359