Allow dropping an image on an empty paragraph block to create an image block #42722
Conversation
kevin940726
added
[Feature] Drag and Drop
|
Size Change: +1.06 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
add/drop-image-on-empty-paragraph
branch
from
ebec345 to
0d367fb
Compare
kevin940726
force-pushed
the
add/drop-image-on-empty-paragraph
branch
3 times, most recently
from
2a95bfc to
339a6f5
Compare
| * @param x The X coordinate. | ||
| * @param y The Y coordinate. | ||
| */ | ||
| dragTo: async ( x: number, y: number ) => { |
There was a problem hiding this comment.
Ideally, we should allow page.mouse.move() and page.mouse.up() to map to these methods automatically. But unfortunately, it's not currently possible in Playwright.
There was a problem hiding this comment.
These work fine Puppeteer, can move the tests to Puppeteer for now?
There was a problem hiding this comment.
I managed to make it work in Playwright too so I don't think it's necessary to move it to Puppeteer for now :).
There was a problem hiding this comment.
FWIW, I don't think this is the right path, but it fixes the issue, and it's always better than nothing 😅.
Ideally, we should be able to reuse useBlockDropZone or useOnBlockDrop in the block library, either that or handle all the dropping logic in the block editor package.
It'd be good to hear a proposal for that.
I wonder if there should be a basic transform system for this, similar to how the files transform already works. Maybe specifying a from and a to for a file transform allows dragging over a block and not just before and after it.
I don't think it's bad to start with something basic though and refactor it later. It gets the feature out there for testing before maturing it into a better API.
On another note, I noticed a small issue at the moment. It's questionable if it's worth solving. When you start a new post there isn't actually a paragraph block in the post.
There's 'Type / to choose a block', but this is the default block appender masquerading as a paragraph, the paragraph is only created when that appender receives focus. I think it's done like this to avoid adding empty paragraphs into new posts (if the user decides to save without adding any content), but the problem is that the default block appender doesn't handle file drops the same way a paragraph does.
|
Thanks, this is a much better experience! I think it might be nice to not trigger the sibling inserter before an empty paragraph or empty media placeholder, since the effect is the same and it just adds a bit of cognitive overhead and precision issues. @jasmussen @ellatrix would you mind checking this one when you have a moment? |
|
This is a really nice PR, and really improves the experience from what's shipping, where it feels weird that you can drop an image above and below a paragraph, but not on a paragraph:
I'm seeing some positioning issues, however, running TT3. The dropzone appears to hover above the empty paragraph, and moves still further up the longer down the page it is:
I would echo Matías that if we can get this one working well, there isn't a need for a sibling inserter before and after an empty paragraph. As also briefly shown in the GIF above, the fact that those are present also, seems to interfere with the positioning of elements, so the code could potentially be simpler. Finally on the visuals, and these aren't strong opinions so in principle we could land this as is, but nevertheless some thoughts:
A small note, it looks like you're testing with TT1. While the code should work with all themes, it's worth noting that TT1 comes with a great deal of custom editor CSS that tweaks margins and paddings, so it's important to also test in something like TT3 or Empty Theme, to make sure the code doesn't get tailored to one theme or the other. I'll spin up a separate PR adding a 2px border radius to the dropzone, I just noticed we're missing that. |
|
Thanks for trying it out! I'm currently busy helping out with other things. I'll get back to it soon. |
kevin940726
force-pushed
the
add/drop-image-on-empty-paragraph
branch
from
339a6f5 to
1da5edd
Compare
|
I refactored it to use Kapture.2022-09-15.at.14.07.50.mp4I'm using the empty theme in the video above, but the paragraph block is a bit too small. The drop zone seems weird, I wonder if we should enlarge it a little bit? |
kevin940726
force-pushed
the
add/drop-image-on-empty-paragraph
branch
from
1da5edd to
18f1b0f
Compare
|
@paaljoachim That issue seems to be an existing one before this PR. If you drop an image between blocks using the "in-between inserter" and click "Undo", it will also enter an infinite-loading state. I guess behind the scenes it's the same issue as in #42001, a naughty |
|
@kevin940726 That's interesting, so just the fact that we render drop zones is causing all these issues? Seems like we might have a serious problem in that component directly because even if we render 1000 drop zones, only a small number of these components is actually "synchronous" (AsyncModeProvider). I agree that the test is not representative but I think the problem is still an important one to solve, because a lot of blocks render drop zones (media, all container blocks), so while empty paragraphs is not common, having a lot of drop zones is. |
|
@youknowriad I don't think that's a new problem introduced in this PR though, if rendering dropzones is costly then it should be like that for a while. However, I guess the main bottleneck here is that we're using I do have some plans to refactor drop zones to share the same state and events globally though. Maybe that would help improve the performance a bit in the long term. |
Why are we rendering Popovers for empty paragraphs? A Popover should only be rendered if you're dragging over the block? |
| className="wp-block-paragraph__drop-zone" | ||
| ref={ popoverRef } | ||
| > | ||
| { isDragging ? ( |
There was a problem hiding this comment.
Why render the parent element (Popover) without any content when isDragging is false?
There was a problem hiding this comment.
I think it's the element the dropzone references, and that's used to set isDragging. So it's a bit of a chicken/egg situation.
Possibly the dropzone could reference the <p> element.
There was a problem hiding this comment.
Yes, useDropZone returns a ref callback so we need to bind it somewhere.
Possibly the dropzone could reference the
<p>element.
But then we'll be risking binding all those drag-and-drop listeners to the <p> element, regardless if it needs to render the dropzone or not, so it's more or less the same.
However, now that I think about it, maybe we only have to bind the returned ref callback to the <p> element when there's no content. I think that could work 🤔 .
A more long-term solution might be to bind the events that set isDragging to a global element, e.g. the root block list or the document itself. So that we don't have to think about these details when implementing DnD.
|
I tried doing some performance profiling, here's the result I have for the commit before this PR
And here's the same actions (block selection) performed in the commit of the PR
You can easily see that there's something constantly running in the browser, there's no idle time at all. This slows down everything. I think it doesn't matter if the issue is a pre-existing component, it's important enough to address it. If it's hard to track, maybe we can we maybe consider reverting temporarily? |
| @@ -155,6 +155,7 @@ export { | |||
| export { default as __experimentalBlockPatternsList } from './block-patterns-list'; | |||
| export { default as __experimentalPublishDateTimePicker } from './publish-date-time-picker'; | |||
| export { default as __experimentalInspectorPopoverHeader } from './inspector-popover-header'; | |||
| export { default as __experimentalUseOnBlockDrop } from './use-on-block-drop'; | |||
There was a problem hiding this comment.
Should we mark this unstable/internal? I don't think this is an API that we want to expose in this shape.
There was a problem hiding this comment.
Sure. I don't mind changing it to __internal or any other naming. I only use __experimental here because that's a more established pattern.
Yep, there was a Worth noting though that it sounds like there's a desire to have some kind of top-level drop zone - #43493 (comment). Maybe instead of a dedicated Provider, there could be a way to check for drag over or drag start of parent drop zones. That would allow conditional rendering of the Popover. Worth noting that the block list is already a drop zone, so this is something that is already possible.
Feels a bit early to do that. It'd be good to explore whether there are any quick fixes first. I also think it's worth looking at that performance test again, and considering whether it should be testing a post full of content, as that's the more common use case. |
Indeed, we can do the test with "large-post.html" if needed but I also noticed that even with full paragraphs, it seems there's some work that is triggered constantly on idle time of the browser less than with empty paragraphs but existent as opposed to before. |
I don't think the scales of these two screenshots are the same? The duration of "before" image is ~24s while the duration of the "after" image is only ~6s. The screenshots difference might not be super representative here. Could you share the profiling files instead? Or maybe even Playwright trace file if it helps? From my testing, I can't seem to notice any big performance regression after applying this PR, assuming there are a limited amount of empty paragraphs on the post. |
|
@kevin940726 These are the exact same tests, 1000 empty paragraphs like the actual performance test, the problem with the one with this branch is that it contains so much data that the browser "cut it" and didn't display the whole graph for me. And the lag is very noticeable in my machine. @mcsf can also confirm as we did this test together this morning. |
|
It doesn't seem necessary to me to render a Popover if we're not showing anything visually to the user. We should use the block element ref to attach a drag listener to set an There's no need for a drag/drop provider? I don't follow the reasoning there. Regardless of the performance impact, I think adding all this logic to the paragraph block isn't the right approach. It's making the paragraph even more complex. And what if we want to extend this to other empty/unmodified blocks? What if the default block is changed to another block? In my opinion, this functionality should be added either internally in |
I agree that there's room for improvement, and I already have some plans and proposals for refactoring the drag-and-drop feature from the ground up. But I don't think an edge case like 1000 empty paragraphs is major enough for us to revert this PR, maybe I'm missing something though. It'd be nice if there's any quick win that we can do to improve it at the time.
The problem is that the drop zone has to be rendered on top of the paragraph block. So that once we hover on the paragraph and trigger the
Not necessarily a provider, but instead binding some common events at the top-most element only once to solve some awkward issues. I think this has been repeated in the past in some issues already. I think it's worth revisiting the use cases and experimenting with them.
I can't think of any use case for that for now, but maybe I'm missing the bigger picture. I think a big change like that deserves a follow-up. I tried to build the
Ah cool! I didn't even know |
|
The performance issue here is actually related to this https://github.com/WordPress/gutenberg/pull/43617/files#r985479949 (Turning this off "solves" the performance issue) |
|
@youknowriad I did try setting that to The performance test isn't showing any difference - https://github.com/WordPress/gutenberg/actions/runs/3156955991/jobs/5137248278#step:5:101. Maybe I made a mistake. |
|
A heads-up that I'm currently trying to improve this in a follow-up with a much bigger scope. I'm trying to integrate it inside the I'll try to share more results soon. |
|
@kevin940726 Ping me for a review |
@talldan I didn't run the test so could be that I'm wrong in terms of "numbers" but when profiling, the browser is back to a decent state (idle time, no constant work being done). Your PR is going to be a great starting point to debug this properly :) I'll tell you if I find anything that stand out. |
Yeah, that's exactly what I saw too, I was very surprised when there were no differences in the PR 😄 |
|
Tried comparing the performance graphs, it seems to be that there's no "obvious" thing, it's just that we're rendering a lot more sync things. In other words, rendering the "Popover" component is intrinsically too slow. Maybe because it uses DOM functions like retrieving DOM rect... maybe there's some caching that we can do there 🤔 |
|
Ok, so I went a rabbit whole here and I found something very interesting. So it turns out we have a potentially an important performance issue with how bubblesVirtually slot/fill is implemented. Basically, every time a fill gets rendered (which happens for instance when you select a block (fill for the toolbar)), all components calling This is shown clearly in the impact this PR had but potentially, it could be already an issue in other situations as well. I'm going to see if I can come up with a fix and see what impact it has on the numbers. |
What?
Allow dropping files/blocks/HTML on an empty paragraph block.
Why?
Fix #29145.
How?
I also created the
dragFilesutil to help test DnD in Playwright e2e tests.Testing Instructions
Screenshots or screencast
Kapture.2022-09-21.at.13.37.28.mp4