fix: auto-scroll bug with multiple containers

[kswenson]

In April 2023, akantic identified an auto-scroll issue with dnd-kit in #1092 and then proposed a fix for it in #1094. In September 2023, unaware of akantic's PR, I fixed the same issue in CODAP in CODAP #901 with a different local patch that has similar effect. In comparing the two different fixes, I now believe that akantic's fix is better than the one that I came up with, but that it can still be improved upon, as demonstrated in this PR.

The code in question comes from the useRects() hook, which is responsible for maintaining/returning an array of rectangles that correspond to the bounding rectangles of an array of elements passed in as an argument. (In the context in which it is used, this array of elements is returned from a call to useScrollableAncestors().) The code in question is:

  if (elements.length > 0 && rects === defaultValue) {
    measureRects();
  }

  useIsomorphicLayoutEffect(() => {
    if (elements.length) {
      elements.forEach((element) => resizeObserver?.observe(element));
    } else {
      resizeObserver?.disconnect();
      measureRects();
    }
  }, [elements]);

where measureRects() is the dispatcher returned from a call to the useReducer() hook that is responsible for updating the state to the correct array of bounding rectangles. The current code guarantees that measureRects() will be called on transitions to/from an empty elements array. The bug occurs when, for instance, on subsequent calls to the useRects() hook the elements array changes from one non-zero length to another. In this case, measureRects() is not called and so the returned array of bounding rectangles has the wrong length. In the words of #1094, "Then, this code brings chaos," pointing to the useAutoScroller() hook which indexes into the array of rectangles.

akantic's fix was to move the call to measureRects() in the useIsomorphicLayoutEffect() hook outside of the else to the root of the effect so that it would be called whenever the hook ran, independent of whether the elements array was empty or not. This makes sense, given that any time the array of elements changes, we need to update the array of bounding rectangles.

My original fix was to change the conditional that precedes the effect to:

  if (elements.length !== rects.length) {
    measureRects();
  }

This has similar effect, in that measureRects() is called any time the length of the elements array changes, but in this case it is called at the root of the render function outside of any effects. While this fixes the issue we were encountering in which the length of the elements array was changing, a limitation of this fix is that it doesn't handle the case in which the elements array changes without changing length, which is why akantic's fix is preferable.

Neither of these earlier fixes accounts for the fact that the other purpose of the useIsomorphicLayoutEffect() hook is to connect/disconnect a resize observer to/from each of the elements in the array, however. Thus, with either of our fixes it is possible for the code to continue to observe elements that are no longer present in the elements array.

This PR implements what I believe to be an improved fix which replaces the original code with the simpler:

  useIsomorphicLayoutEffect(() => {
    resizeObserver?.disconnect();
    measureRects();
    elements.forEach((element) => resizeObserver?.observe(element));
  }, [elements]);

The transition to/from an empty elements array is no longer a special case. Any time the elements array changes, we must disconnect the resize observer from the previous elements, call measureRects() to update the current set of bounding rectangles, and make sure that the current elements are observed. Doing so consistently inside the useIsomorphicLayoutEffect() obviates the need for the separate conditional call to measureRects() outside the effect.

See also:

• Auto scroll is broken when using multiple containers #1092 -- akantic's original issue
• Fix rects measuring on elements change #1094 -- akantic's original PR
• MultipleContainers jitter from inner item when dragging to bottom of new column #1080, React 18: Autoscroll is broken with multiple horizontal drop zones which have vertical scroll #1108 -- potentially related, but I didn't investigate further
• CODAP #901 -- CODAP's original patch PR

[changeset-bot]

⚠️ No Changeset found

Latest commit: a25cc7a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR