Performance: Drag items via transform property to avoid layout and re-paints #33395
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 tasks
gwwar
force-pushed
the
try/transform-draggable
branch
from
45c332f
to
f42ff4b
Compare
|
Size Change: +28 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Playing around with the draggable + dropzone code, it feels like we have a few opportunities.
|
|
I did some testing and the results seem good so far. The blue drop zone indicators are noticeably quite delayed at updating position, so that might be something to improve. I think that's one of those things that needs to feel quite responsive, or the user will perceive it as slow, even if the code is well optimized. Definitely a good idea to work on this iteratively, so if we can get some noticeable improvements with the Draggable first, we can ship that straight away. |
Sounds good, I can tidy up changes to pull out the draggable changes first (will set this to ready for review when I'm done). General dragging performance is still going to be pretty slow from the existing dropzone handlers, but the difference in removing those layout shifts should be visible from browser profiling. I'll spin up a follow up draft to continue experimenting with drag intents. |
gwwar
commented
Jul 14, 2021
| y = event.clientY - 100; | ||
| // Can also be written as the affine transform: matrix( 0.5, 0, 0, 0.5, x, y ). | ||
| // Note that order matters, translate should be done before scale. | ||
| cloneWrapper.style.transform = `translate( ${ x }px, ${ y }px ) scale(0.5)`; |
There was a problem hiding this comment.
Do we have any examples where we drag an item that's more than 700px? I was wondering if we still needed to handle this case.
There was a problem hiding this comment.
I think just about every interaction uses a draggable chip now rather than a clone of an element, so this is just here for consumers of the package.
There was a problem hiding this comment.
I'm quite tempted to rip out this case. Here's what it looks like in trunk with a draggable with a 701x701px square. Scaling to 50% of the original draggable isn't going to work with larger items, so folks will likely need to come up with a similar solution to what we went with.
The positioning also looks off, cursor at top left vs center (which I can fix, but would modify current behavior).
trunk_701px.mp4
There was a problem hiding this comment.
I tried removing this case in 9667539
There was a problem hiding this comment.
Hmm, yeah. It feels like it'd be better if it had a max size instead of reducing by 50%, but maybe that's to stop the drag image from being blurry.
Removing it seems ok, we can monitor to see if there's any negative feedback.
I imagine this was for large blocks, so the use case is more than likely no longer present.
|
This one is ready for review 👍 I moved over other exploratory dropzone performance handling to #33435 |
talldan
reviewed
Jul 15, 2021
Comment on lines
99
to
102
| dragState.cursorLeft = e.clientX; | ||
| dragState.cursorTop = e.clientY; | ||
| dragState.x = nextX; | ||
| dragState.y = nextY; |
There was a problem hiding this comment.
I wonder if it might be confusing updating the object properties in this way through an object reference. 🤔
Maybe here it could call an updateDragState function that makes the change to a ref defined by useRef or something. Not sure. I recall the previous version of this code being a bit hard to follow anyhow.
Any other ideas?
There was a problem hiding this comment.
I tried removing the dragState object in 9667539 removing the extra function.
Closures are pretty common in animation/drawing handlers (since we need to keep track of what happened in the last step vs the current step), but we could try to tidy up function signatures in a larger refactor. I'd maybe move that to a seperate PR though.
talldan
reviewed
Jul 15, 2021
talldan
reviewed
Jul 15, 2021
gwwar
force-pushed
the
try/transform-draggable
branch
3 times, most recently
from
634c518
to
998ff0d
Compare
gwwar
force-pushed
the
try/transform-draggable
branch
from
998ff0d
to
9667539
Compare
talldan
approved these changes
Jul 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Our draggable component updates a drag chip by modifying
topandleftproperties. This unfortunately forces the browser to go through layout > paint> and compositing steps which is expensive. As we can see in the profiler on trunk, we're both dropping frames and seeing layout shifts.Changes in this PR update this to use
transforminstead which can skip layout and paint, to go straight to composite. We also add a throttle to reduce the number of fired drag events, and early escape if the mouse has not moved.Our drop zone handlers are also causing major performance issues when dragging quickly. This will be handled in #33435
Videos below are taken with the Armando theme, using the "alternating rows with images and headings" pattern.
Testing Instructions
Profiling In Trunk
trunk.mp4
Profiling in Branch
branch.mp4