Performance: only trigger dropzone handling when a user intends to drop an item#33435
Performance: only trigger dropzone handling when a user intends to drop an item#33435
Conversation
|
Size Change: +111 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
gwwar
force-pushed
the
try/dropzone-drag-intent
branch
2 times, most recently
from
374dd17 to
43aa735
Compare
|
I did a little side exploration (with extremely rough code), to see what would happen if we added one set of global drag handlers to the document, instead of per drop zone in 9eee304 I'm going to defer on this approach for now, since the chance of regression is a bit too high, but I think it might be worth revisiting if we're still seeing some bad performance characteristics with drag in the future. A few things I discovered:
global_experiment.mp4 |
|
Ran out of time, but I did a very basic distance check in 4adbcf7 which looks promising. Things to investigate next week would be:
basic.distance.check.mp4 |
gwwar
force-pushed
the
try/dropzone-drag-intent
branch
from
4adbcf7 to
a445056
Compare
gwwar
added
[Type] Performance
|
This one is ready for review 👍 |
Hmm, looks like that's unhappy in CI, but works locally for me. I'll try and see if other tuning options work out a bit better. |
|
Updated the summary with the new profiling timings with the 33ms sampling. |
| lastDragX = event.clientX; | ||
| lastDragY = event.clientY; | ||
| lastTime = time; | ||
| } |
There was a problem hiding this comment.
What would be the difference with lodash's throttle? If we keep this type of throttling, any change we can abstract this out of this hook?
There was a problem hiding this comment.
What would be the difference with lodash's throttle?
Great question! So using some of the nice visualizations from: https://css-tricks.com/debouncing-throttling-explained-examples/
Debounce
Debounce turns a number of raw events into a single (usually trailing) event call for some interval. This is really great for things like search inputs/API calls. Say one API call to search for "cats" instead of four API requests for "c", "ca", "cat", "cats" (which might resolve in incorrect order).
Throttle
With throttle we don’t allow to our function to execute more than once every X milliseconds. This usually works well with things involving animation, since we'd like to see all the inbetween work at a consistent rate. For example, imagine if a scroll handler skipped all the way to the top or bottom using debounce without animating the inbetween frames, it'd be jarring.
Heuristic - Is the user dragging slowly?
In this case, I'm using a pretty basic heuristic of is the user dragging slowly? If they are, we can guess that they intend to drop an item soon and so we should show drag inserters/hints (with all the fancy expensive animations that come with that). If not we can skip work.
What's tricky is tuning the sensitivity. If it's too sensitive we'll show the drag inserters too often (and slow down dragging), if it's not sensitive enough users will have trouble dropping items into dropzones.
A great way of visualizing this is with an old jQuery plugin hover intent that uses mouse velocity to trigger if a hover event should fire or not https://briancherne.github.io/jquery-hoverIntent/:
See how moving the mouse back and forth in the first row can queue up a lot of events, while the second row is closer to what a user would expect for interactive behavior?
hoverIntent.mp4
If we keep this type of throttling, any change we can abstract this out of this hook?
We can certainly pull out a mouse velocity/user intent piece utility with sensitivity/timing options, but I'd maybe hold off until we have a second example. It looks like it's somewhat rare for us to need to tune raw event handling like this.
If you know that we already have a user intent/mouse velocity library in place already, I'm happy to update the implementation too.
There was a problem hiding this comment.
I'm not worried about reusability, I'm more worried about polluting useDropZone with timing specific things. I just have the feeling that we're making the hook less pure, which is only meant to act as a thin wrapper around the native drag and drop API.
There was a problem hiding this comment.
Sure, I'll see if I we can tidy things a bit more without affecting cost too much
There was a problem hiding this comment.
This is a great comment https://github.com/WordPress/gutenberg/pull/33435/files#r676775855 that I think deserves to be an inline documentation or an architecture doc.
Is this behavior similar to the drag and scroll behavior we have where we also compute mouse velocity? Could this be considered a second use-case for the same utility.
There was a problem hiding this comment.
I think deserves to be an inline documentation or an architecture doc.
Sure, happy to add one. Maybe a subsection for common strategies in https://github.com/WordPress/gutenberg/blob/trunk/docs/explanations/architecture/performance.md ?
Is this behavior similar to the drag and scroll behavior we have where we also compute mouse velocity? Could this be considered a second use-case for the same utility.
If we're computing mouse velocity elsewhere, we can potentially reuse it for this handler. @youknowriad do you have a link to the file for where this happens? I can try incorporating it here or in a follow up.
There was a problem hiding this comment.
Sure, happy to add one. Maybe a subsection for common strategies in https://github.com/WordPress/gutenberg/blob/trunk/docs/explanations/architecture/performance.md ?
Sounds good to me.
If we're computing mouse velocity elsewhere, we can potentially reuse it for this handler. @youknowriad do you have a link to the file for where this happens? I can try incorporating it here or in a follow up.
I'm not very familiar with this hook but I think this is where we're kind of doing similar things:
The idea of that hook is to scroll the container as we get to the edge with the mouse but there's something about velocity as well, if you're moving fast, it scrolls faster.
There was a problem hiding this comment.
I still think this is worth separating for clarity, ideally just a wrapper function similar to throttle and debounce that the implementor of the drop zone can add around the callbacks. useDropZone is otherwise a simple utility around the native drag and drop API to make it easier to work with, it shouldn't have any opinions about the rest imho.
| //first frame | ||
| lastTime = time; | ||
| lastDragX = event.clientX; | ||
| lastDragY = event.clientY; |
There was a problem hiding this comment.
Should these be set on drag start?
|
|
||
| return await page.mouse.dragAndDrop( draggablePoint, targetPoint ); | ||
| return await page.mouse.dragAndDrop( draggablePoint, targetPoint, { | ||
| delay: 100, |
There was a problem hiding this comment.
Where is this 100 coming from?
There was a problem hiding this comment.
This E2E was passing locally but not in CI, the 100ms is pretty arbitrary, but we could try other smaller values.
gwwar
force-pushed
the
try/dropzone-drag-intent
branch
from
e22b2f3 to
2bfbe23
Compare
|
Going to test a few other sensitivity values |
gwwar
force-pushed
the
try/dropzone-drag-intent
branch
from
b280bb0 to
914e188
Compare
|
@talldan much appreciated if you could see if drag performance feels as responsive (or better) in the persistent list view. |
There was a problem hiding this comment.
Are they trying to move the item quickly, someplace else or are they ready to drop an item into a drop zone? If they're not ready to drop, we can skip work.
I think this aspect makes it feel less responsive. For example, if I'm dragging in the demo post in List View, and move my cursor quite quickly, here's what I observe:
- In trunk, the blue drop indicator follows my cursor much more closely, which makes it feel responsive (even if it is more computationally expensive)
- In this branch, the blue drop indicator stays in place momentarily and only updates a few times before my cursor reaches its destination.
This is one of those interesting cases where UX intersects performance. Is the right behavior for the drop indicator to update so much? Probably not. There might be some interesting experimentation here - like when the user is dragging quickly the indicator could just follow the cursor (a simple calculation), and only when the cursor settles in a position could it snap to the right block position (the complex calculation). I don't know exactly how that would feel but it'd be interesting to try.
Thinking as well about how things work in video games, they usually have a fixed rendering loop and a variable 'update' loop working separately, so you get fixed animations frame rates with the 60fps goal, but the business logic (physics and other game logic) could be working slower or faster than that. A new loop is only triggered when the previous one has completed. I think that could manifest itself as more of a locking mechanism, where onDragOverRef.current is only called if previous work has completed.
Sorry, this turned into quite a long review without many conclusions!
| const distance = | ||
| Math.abs( | ||
| event.clientY - dragTimingRef.current.dragY | ||
| ) + | ||
| Math.abs( | ||
| event.clientX - dragTimingRef.current.dragX | ||
| ); |
There was a problem hiding this comment.
I think distance should be something like Math.sqrt( Math.pow( x1 - x2, 2 ) + Math.pow( y1 - y2, 2 ) ).
(https://paulrohan.medium.com/euclidean-distance-and-normalization-of-a-vector-76f7a97abd9)
| } | ||
| dragTimingRef.current.dragX = event.clientX; | ||
| dragTimingRef.current.dragY = event.clientY; | ||
| dragTimingRef.current.timestamp = time; |
There was a problem hiding this comment.
I wonder if the timestamp should perhaps only be updated if the onDragOverRef.current callback has been called. As it stands, this will throttle future calls even if nothing happened the last time around because the user didn't drag far enough to trigger the callback.
The alternative, having it within the if statement, means that <16ms could have passed since the last onDragOver event, but it's really >16ms since the last time we did any actual work, so the callback can be triggered.
| Math.abs( | ||
| event.clientX - dragTimingRef.current.dragX | ||
| ); | ||
| if ( distance < 10 ) { |
There was a problem hiding this comment.
I guess the way this stands this is really a speed calculation. The cursor has to travel at slower than 10pixels per ~16ms to trigger an update.
Nothing to do here, just noting my lightbulb moment 😄
🤔 @talldan One thing we can try first is provide better drag hints (vs the 1px line) in a separate PR. This would give us better UX and would make this a bit more obvious as we try and tune drag performance a bit.
💯 For example, I'm quite aware of how slow/sluggish that drag chip is on trunk, to the point where I normally don't try to perform any editing actions with drag. At the same time it'll be pretty frustrating for folks if dropping items into an area isn't responsive enough (eg we had a false negative that they were trying to drag an item somewhere else quickly.)
Work/render loops can be quite useful in complicated apps. It's possible for us to also use requestAnimationFrame here if we like results. (As an aside a single global drag handler that calls the right child callbacks is much faster, but also is a big reactor). |
|
Going to close this one out for now in favor of some new drag interaction experiments |
4 participants
Background
Our drop zone handlers are also causing major performance issues when dragging quickly. We have dragging listeners on every component that is a drop zone. These events are currently not throttled and when dragging quickly we can queue up quite a number of events to resolve. We will also trigger a number of unneeded animations. This slows things down quite a bit.
In this PR we try and infer what the user intends to do. Are they trying to move the item quickly, someplace else or are they ready to drop an item into a drop zone? If they're not ready to drop, we can skip work.
I profiled using the Armando theme, and the "alternating rows with images and headings" pattern.
Here are some videos of the type of actions I was performing while profiling. Note that screenrecording + profiling also impacts performance, so I took separate profiling screencaps above.
Profiling In Trunk
before.mp4
Branch
after-16.mp4
Testing Instructions
Follow Up
To improve drag performance further I think there's two areas of interest I've spotted: