adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
13.03k stars 1.13k forks source link

drag and drop useDrag hook use of useDragModality causes too many rerenders #2403

Open damenking opened 3 years ago

damenking commented 3 years ago

๐Ÿ› Bug Report

Draggable components using props from useDraggableItem hook rerender 3 times whenever the keyboard/mouse modality changes. This creates long delays when a separate input is clicked into and then typed into when over ~50 draggable items are in the dom.

๐Ÿค” Expected Behavior

Each draggable component would ideally not rerender if there is no presentational change, or at most once if it's really necessary.

๐Ÿ˜ฏ Current Behavior

useDragModality ends up added multiple event listeners to the document which is causing the component to rerender multiple times whenever the user switches from mouse to keyboard, even though there is no visiual change in the component.

๐Ÿ’ Possible Solution

Only watching for modality changes within the container managing the drag collection might be the easiest but not ideal solution.

๐Ÿ”ฆ Context

I have a list of ~120 draggable items and a form above them. Using the form is extremely slow due to the list rerendering so many times. The slowness is not very noticeable when just using the draggable list because you are usually going to be using the mouse or keyboard exclusively, however the text input fields above it cause the issue to manifest frequently.

๐Ÿ’ป Code Sample

https://codesandbox.io/s/dnd-base-cy15o Open the console and observe the number of times "rendering collection item" is logged each time modality is switched.

๐ŸŒ Your Environment

Software Version(s)
react-spectrum 3.14.1
Browser chrome
Operating System osx big sur

๐Ÿงข Your Company/Team

Adobe/Marketo

๐Ÿ•ท Tracking Issue (optional)

LFDanLu commented 3 years ago

Discussed in slack, seems to be in a similar vein to the issues we had previously encountered in TableView with https://github.com/adobe/react-spectrum/pull/1875 and https://github.com/adobe/react-spectrum/pull/2059. Would be good if we only rerender the item currently being focused when modality changes?

damenking commented 3 years ago

Seems good assuming it doesn't impact other functionality. I imagine that would radically improve the performance

LFDanLu commented 1 year ago

@damenking Just curious, have you been able to move over to using ListView now that it supports drag and drop? Hopefully that will allow you to avoid these performance problems until we pick this issue up again.

damenking commented 1 year ago

@LFDanLu I am no longer working on this so I have not tried anything since filing this issue

bwittenberg commented 6 months ago

We have the same performance problem in our components that call useDraggableItem. We are considering a patch to this hook to prevent re-rendering when modality changes, but I'm concerned it might cause problems for components that expect a re-render when the modality changes.

Is there any work planned to improve performance?

bwittenberg commented 6 months ago

The patch changes the hook to the following:

export function useInteractionModality(): Modality | null {
  setupGlobalFocusEvents();

  return useIsSSR() ? null : modality;
}

This isn't ideal because the hook isn't reactive any more, but it might be worth the tradeoff in our case so that typing doesn't lag for users.