Enable drag and drop in List View, fix performance issues #33320
Conversation
talldan
added
[Feature] Drag and Drop
| 'is-dropping-before': isDroppingBefore || isBlockMoveTarget, | ||
| 'is-dropping-after': isDroppingAfter, | ||
| 'is-dropping-to-inner-blocks': isDroppingToInnerBlocks, | ||
| 'is-dropping-before': isBlockMoveTarget, |
There was a problem hiding this comment.
TODO - remove this and use the new drop indicator for the 'Move To' option.
There was a problem hiding this comment.
Hmm, I'm going to make this a follow up too. I think Move To needs some refactoring. It currently changes the selected block to determine where it should be inserted, but it should just use the insertionPoint too. It needs changing in both the main block editor and list view.
| <ListViewContext.Provider value={ contextValue }> | ||
| <ListViewDropIndicator | ||
| listViewRef={ elementRef } | ||
| blockDropTarget={ blockDropTarget } |
There was a problem hiding this comment.
TODO - try making List View use the Insertion Point state so that the indicators appear in both the main and list view block lists.
There was a problem hiding this comment.
Tried this out, but it does slow things down a bit, will push my explorations to a separate PR.
|
Size Change: +283 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
This is ready for review now. #33327 should be merged before this though, the tests I've added depend on that. |
There was a problem hiding this comment.
What sort of initial performance targets are we aiming for? I am seeing some dropped frames while dragging in list view with the natural resources pattern.
I did a very quick check, but one hotspot is the dragover event:
I'm guessing this is coming from a library ( I didn't dig too deep), but modifying width, height, left, right props are one of the most expensive actions (basically anything that modifies getboundingclientrect) since we end up needing to go through layout > paint > and composite steps in the rendering process.
One thing we could try is seeing it's possible to use transform property for dragging which can potentially skip layout and paint steps.
See also: https://developers.google.com/web/fundamentals/performance/rendering
This change makes the drop indicator a separate popover element, which mirrors how the InsertionPoint component in the normal block list works. Only this single component now has to re-render, which improves things significantly in my tests.
One other idea might be to render all droppable indicators once while starting a drag, but toggle opacity of them as we enter over them. We'd of course need to profile to test if this helps or not.
I'll leave a some follow up comments for manual testing, as I did spot a few things.
package-lock.json
Outdated
| @@ -15704,7 +15704,7 @@ | |||
| "postcss": "^8.2.15", | |||
| "postcss-loader": "^4.2.0", | |||
| "prettier": "npm:[email protected]", | |||
| "puppeteer-core": "^9.0.0", | |||
| "puppeteer-core": "^10.0.0", | |||
There was a problem hiding this comment.
Did we mean to include package-lock updates?
There was a problem hiding this comment.
👍 I do have another PR which is handling a Puppeteer upgrade - #33327. I'll rebase this and drop the changes when that's merged.
packages/scripts/package.json
Outdated
| @@ -69,7 +69,7 @@ | |||
| "postcss": "^8.2.15", | |||
| "postcss-loader": "^4.2.0", | |||
| "prettier": "npm:[email protected]", | |||
| "puppeteer-core": "^9.0.0", | |||
| "puppeteer-core": "^10.0.0", | |||
There was a problem hiding this comment.
Did we mean to bump puppeteer in this PR?
|
Overall this looks pretty close @talldan! I did spot a few things, so may want to disable dragging in some list view instances or try to polish the behavior. ❌ The navigation block also has a list view, trying to drag in this modal can abruptly close this, since I think we're fighting with the other draggables in the post editor: Kapture.2021-07-09.at.09.24.40.mp4❌ When the persistent list view is open in the post editor, and we try dragging the modal, the wrong draggable indicators are updated: Kapture.2021-07-09.at.09.29.05.mp4✅ Persistent List View Looks Good in Post Editor ✨ Kapture.2021-07-09.at.09.30.17.mp4✅ Persistent List View Looks Good in Site Editor Kapture.2021-07-09.at.09.32.14.mp4❓ Navigation Editor 🤔 There's two ways of dragging in the Navigation Editor. One using the modal, and the other using an editor mode when we press esc
We may want some UX feedback for this. What's our preferred way of dragging? cc @jasmussen @shaunandrews maybe? Kapture.2021-07-09.at.09.36.42.mp4Kapture.2021-07-09.at.09.39.45.mp4✅ Widget Editor looks good I did notice two things: the list view isn't persistent (it's a dropdown vs sidebar), and we allow draggables from the list view to drop into the main editor (which looks like it works?) Kapture.2021-07-09.at.09.53.43.mp4Kapture.2021-07-09.at.09.50.30.mp4Things for potential follow ups: |
|
Here's a video showing that the layout shifts are coming from the draggable chip: Kapture.2021-07-09.at.10.09.08.mp4Edit: https://web.dev/cls/#animations-and-transitions
Looks like this is recommended ✨ |
|
Thanks for the amazing review @gwwar!
It's mostly anecdotal, but with the previous implementation there were two main issues:
So if that doesn't happen too much it's a good sign. I've been testing using the demo post, which has a decent number of blocks. I think as long as it's working reasonably well, it'd be great for this feature to be released in the plugin for further testing and feedback, especially at this early stage where there's a fair amount of time until the next WordPress release.
This is a really good point. AFAIK, all popovers modify
Thanks, I'll take a look at this and see if I can fix it in this PR.
Oh wow, I think it's just using the first element it finds using
I think the widget editor would definitely benefit from the persistent list view, given the widget areas automatically have nesting. Being from list view to the main block list is intentional 😄.
I'll make sure to capture these ideas. There are some ideas on #29727 that should be followed-up on too. A lot of this came quite a long time after I'd already developed the drag and drop feature for the navigation screen (#23952), but then List View was mostly removed from that screen so the feature has just lay dormant. With the persistent list view it's now very relevant again. |
| @@ -75,7 +74,6 @@ const BlockDraggable = ( { | |||
| return ( | |||
| <Draggable | |||
| cloneClassname={ cloneClassname } | |||
| elementId={ elementId } | |||
There was a problem hiding this comment.
The elementId prop seems to be dead code, Draggable ignores it when __experimentalDragComponent is defined, and BlockDraggable has that prop hard-coded just below this line.
@gwwar I tried a few different things, but couldn't reproduce this. Let me know if you have any pointers for reproducing it.
I've fixed this one. |
✅ I've verified that these two issues are now fixed. ✨ |
There was a problem hiding this comment.
I think as long as it's working reasonably well, it'd be great for this feature to be released in the plugin for further testing and feedback, especially at this early stage where there's a fair amount of time until the next WordPress release.
That sounds reasonable to me! Dragging is currently usable but not smooth on my dev laptop. Ideally we'll want to do a few browser profiling passes at it so folks can use this without needing beefy hardware.
This is a really good point. AFAIK, all popovers modify top and left, so I wonder if it's possible to try using transform. It may have been tried before, not sure. I don't think BlockDraggableChip uses popovers to this might be a good place to start using transform. I'll try that out in a separate PR.
Happy to help investigate that too. So this is coming from the popovers component? Edit: ah I think it's from draggable
gutenberg/packages/components/src/draggable/index.js
Lines 103 to 113 in dc1821f
…move duplicate ids
talldan
force-pushed
the
try/popover-drop-indicator-in-list-view
branch
from
1c92801
to
d1ab212
Compare
That's the one. 👍 Hopefully shouldn't be too complicated to modify those styles. |
|
Amazing review of an amazing PR! 👏 ❤️ |
|
I apologize if this is not the right place to share this. You can move it if necessary (or tell me to). This functionality is great, but it doesn't seem to allow you to drag a child-block to the bottom of the list at the top-level of the hierarchy unless you bring it to the very top of the list and then rearrange the top-level blocks using their Move Up/Down arrows. Here's a screen recording of what I mean. Also, it would be nice to have a wider space to drag within - ideally anywhere above and below the top and bottom of the list would be treated as a draggable section, rather than just a small sliver. I submitted similar feedback about dragging blocks within the editor too here (#32438) WMgIDuTMYl.mp4 |
Description
Fixes #25067
Fixes #29727
Enables list view drag and drop and fixes the performance issues.
List view's drag and drop has been implemented for a while, but disabled due to some performance issues were solved. The main problem is that previously the drag and drop system in List View used borders on each individual block for displaying the drop indicator. This means that when dragging over a list of block, each individual block has to re-render.
This change makes the drop indicator a separate popover element, which mirrors how the InsertionPoint component in the normal block list works. Only this single component now has to re-render, which improves things significantly in my tests.
How has this been tested?
Some things to try:
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).