Allow dropping an image on an empty paragraph block to create an image block

[kevin940726]

What?

Allow dropping files/blocks/HTML on an empty paragraph block.

Why?

Fix https://github.com/WordPress/gutenberg/issues/29145.

How?

I also created the dragFiles util to help test DnD in Playwright e2e tests.

Testing Instructions

1. Create a new post or edit an existing post.
2. Add an empty paragraph block (note that the default block in a newly created post is not a paragraph block, yet).
3. Drag an image to the block.
4. It should show the placeholder upon hovering.
5. Dropping the image should create an image block.

Screenshots or screencast

https://user-images.githubusercontent.com/7753001/191423167-52a86f37-a6ea-4c3f-9b71-0fdfdbf5f72c.mp4

[github-actions[bot]]

Size Change: +1.06 kB (0%)

Total Size: 1.26 MB

Filename	Size	Change
build/block-directory/index.min.js	7.08 kB	+26 B (0%)
build/block-editor/index.min.js	164 kB	+613 B (0%)
build/block-editor/style-rtl.css	15.4 kB	+1 B (0%)
build/block-editor/style.css	15.4 kB	+4 B (0%)
build/block-library/blocks/paragraph/editor-rtl.css	317 B	+143 B (+82%)	🆘
build/block-library/blocks/paragraph/editor.css	317 B	+143 B (+82%)	🆘
build/block-library/editor-rtl.css	11.1 kB	+55 B (0%)
build/block-library/editor.css	11.1 kB	+57 B (+1%)
build/block-library/index.min.js	191 kB	+560 B (0%)
build/components/index.min.js	198 kB	-65 B (0%)
build/compose/index.min.js	12.5 kB	-25 B (0%)
build/customize-widgets/style-rtl.css	1.38 kB	+5 B (0%)
build/customize-widgets/style.css	1.38 kB	+5 B (0%)
build/edit-post/index.min.js	30.8 kB	+26 B (0%)
build/edit-site/index.min.js	57.6 kB	-379 B (-1%)
build/editor/index.min.js	41.6 kB	-25 B (0%)
build/editor/style-rtl.css	3.62 kB	-39 B (-1%)
build/editor/style.css	3.61 kB	-41 B (-1%)

ℹ️ View Unchanged

Filename	Size
build/a11y/index.min.js	982 B
build/annotations/index.min.js	2.76 kB
build/api-fetch/index.min.js	2.26 kB
build/autop/index.min.js	2.14 kB
build/blob/index.min.js	475 B
build/block-directory/style-rtl.css	990 B
build/block-directory/style.css	991 B
build/block-editor/default-editor-styles-rtl.css	378 B
build/block-editor/default-editor-styles.css	378 B
build/block-library/blocks/archives/editor-rtl.css	61 B
build/block-library/blocks/archives/editor.css	60 B
build/block-library/blocks/archives/style-rtl.css	65 B
build/block-library/blocks/archives/style.css	65 B
build/block-library/blocks/audio/editor-rtl.css	150 B
build/block-library/blocks/audio/editor.css	150 B
build/block-library/blocks/audio/style-rtl.css	122 B
build/block-library/blocks/audio/style.css	122 B
build/block-library/blocks/audio/theme-rtl.css	126 B
build/block-library/blocks/audio/theme.css	126 B
build/block-library/blocks/avatar/editor-rtl.css	116 B
build/block-library/blocks/avatar/editor.css	116 B
build/block-library/blocks/avatar/style-rtl.css	84 B
build/block-library/blocks/avatar/style.css	84 B
build/block-library/blocks/block/editor-rtl.css	161 B
build/block-library/blocks/block/editor.css	161 B
build/block-library/blocks/button/editor-rtl.css	482 B
build/block-library/blocks/button/editor.css	482 B
build/block-library/blocks/button/style-rtl.css	523 B
build/block-library/blocks/button/style.css	523 B
build/block-library/blocks/buttons/editor-rtl.css	337 B
build/block-library/blocks/buttons/editor.css	337 B
build/block-library/blocks/buttons/style-rtl.css	332 B
build/block-library/blocks/buttons/style.css	332 B
build/block-library/blocks/calendar/style-rtl.css	239 B
build/block-library/blocks/calendar/style.css	239 B
build/block-library/blocks/categories/editor-rtl.css	84 B
build/block-library/blocks/categories/editor.css	83 B
build/block-library/blocks/categories/style-rtl.css	100 B
build/block-library/blocks/categories/style.css	100 B
build/block-library/blocks/code/editor-rtl.css	53 B
build/block-library/blocks/code/editor.css	53 B
build/block-library/blocks/code/style-rtl.css	103 B
build/block-library/blocks/code/style.css	103 B
build/block-library/blocks/code/theme-rtl.css	124 B
build/block-library/blocks/code/theme.css	124 B
build/block-library/blocks/columns/editor-rtl.css	108 B
build/block-library/blocks/columns/editor.css	108 B
build/block-library/blocks/columns/style-rtl.css	406 B
build/block-library/blocks/columns/style.css	406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css	125 B
build/block-library/blocks/comment-author-avatar/editor.css	125 B
build/block-library/blocks/comment-content/style-rtl.css	92 B
build/block-library/blocks/comment-content/style.css	92 B
build/block-library/blocks/comment-template/style-rtl.css	187 B
build/block-library/blocks/comment-template/style.css	185 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css	123 B
build/block-library/blocks/comments-pagination-numbers/editor.css	121 B
build/block-library/blocks/comments-pagination/editor-rtl.css	222 B
build/block-library/blocks/comments-pagination/editor.css	209 B
build/block-library/blocks/comments-pagination/style-rtl.css	235 B
build/block-library/blocks/comments-pagination/style.css	231 B
build/block-library/blocks/comments-title/editor-rtl.css	75 B
build/block-library/blocks/comments-title/editor.css	75 B
build/block-library/blocks/comments/editor-rtl.css	834 B
build/block-library/blocks/comments/editor.css	832 B
build/block-library/blocks/comments/style-rtl.css	632 B
build/block-library/blocks/comments/style.css	630 B
build/block-library/blocks/cover/editor-rtl.css	605 B
build/block-library/blocks/cover/editor.css	607 B
build/block-library/blocks/cover/style-rtl.css	1.57 kB
build/block-library/blocks/cover/style.css	1.55 kB
build/block-library/blocks/embed/editor-rtl.css	293 B
build/block-library/blocks/embed/editor.css	293 B
build/block-library/blocks/embed/style-rtl.css	410 B
build/block-library/blocks/embed/style.css	410 B
build/block-library/blocks/embed/theme-rtl.css	126 B
build/block-library/blocks/embed/theme.css	126 B
build/block-library/blocks/file/editor-rtl.css	300 B
build/block-library/blocks/file/editor.css	300 B
build/block-library/blocks/file/style-rtl.css	253 B
build/block-library/blocks/file/style.css	254 B
build/block-library/blocks/file/view.min.js	346 B
build/block-library/blocks/freeform/editor-rtl.css	2.44 kB
build/block-library/blocks/freeform/editor.css	2.44 kB
build/block-library/blocks/gallery/editor-rtl.css	948 B
build/block-library/blocks/gallery/editor.css	950 B
build/block-library/blocks/gallery/style-rtl.css	1.53 kB
build/block-library/blocks/gallery/style.css	1.53 kB
build/block-library/blocks/gallery/theme-rtl.css	108 B
build/block-library/blocks/gallery/theme.css	108 B
build/block-library/blocks/group/editor-rtl.css	337 B
build/block-library/blocks/group/editor.css	337 B
build/block-library/blocks/group/style-rtl.css	57 B
build/block-library/blocks/group/style.css	57 B
build/block-library/blocks/group/theme-rtl.css	78 B
build/block-library/blocks/group/theme.css	78 B
build/block-library/blocks/heading/style-rtl.css	76 B
build/block-library/blocks/heading/style.css	76 B
build/block-library/blocks/html/editor-rtl.css	327 B
build/block-library/blocks/html/editor.css	329 B
build/block-library/blocks/image/editor-rtl.css	876 B
build/block-library/blocks/image/editor.css	873 B
build/block-library/blocks/image/style-rtl.css	627 B
build/block-library/blocks/image/style.css	630 B
build/block-library/blocks/image/theme-rtl.css	126 B
build/block-library/blocks/image/theme.css	126 B
build/block-library/blocks/latest-comments/style-rtl.css	284 B
build/block-library/blocks/latest-comments/style.css	284 B
build/block-library/blocks/latest-posts/editor-rtl.css	213 B
build/block-library/blocks/latest-posts/editor.css	212 B
build/block-library/blocks/latest-posts/style-rtl.css	463 B
build/block-library/blocks/latest-posts/style.css	462 B
build/block-library/blocks/list/style-rtl.css	88 B
build/block-library/blocks/list/style.css	88 B
build/block-library/blocks/media-text/editor-rtl.css	266 B
build/block-library/blocks/media-text/editor.css	263 B
build/block-library/blocks/media-text/style-rtl.css	507 B
build/block-library/blocks/media-text/style.css	505 B
build/block-library/blocks/more/editor-rtl.css	431 B
build/block-library/blocks/more/editor.css	431 B
build/block-library/blocks/navigation-link/editor-rtl.css	705 B
build/block-library/blocks/navigation-link/editor.css	703 B
build/block-library/blocks/navigation-link/style-rtl.css	115 B
build/block-library/blocks/navigation-link/style.css	115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css	296 B
build/block-library/blocks/navigation-submenu/editor.css	295 B
build/block-library/blocks/navigation-submenu/view.min.js	423 B
build/block-library/blocks/navigation/editor-rtl.css	1.99 kB
build/block-library/blocks/navigation/editor.css	2 kB
build/block-library/blocks/navigation/style-rtl.css	2.17 kB
build/block-library/blocks/navigation/style.css	2.16 kB
build/block-library/blocks/navigation/view-modal.min.js	2.78 kB
build/block-library/blocks/navigation/view.min.js	443 B
build/block-library/blocks/nextpage/editor-rtl.css	395 B
build/block-library/blocks/nextpage/editor.css	395 B
build/block-library/blocks/page-list/editor-rtl.css	363 B
build/block-library/blocks/page-list/editor.css	363 B
build/block-library/blocks/page-list/style-rtl.css	175 B
build/block-library/blocks/page-list/style.css	175 B
build/block-library/blocks/paragraph/style-rtl.css	260 B
build/block-library/blocks/paragraph/style.css	260 B
build/block-library/blocks/post-author/style-rtl.css	175 B
build/block-library/blocks/post-author/style.css	176 B
build/block-library/blocks/post-comments-form/editor-rtl.css	96 B
build/block-library/blocks/post-comments-form/editor.css	96 B
build/block-library/blocks/post-comments-form/style-rtl.css	493 B
build/block-library/blocks/post-comments-form/style.css	493 B
build/block-library/blocks/post-date/style-rtl.css	61 B
build/block-library/blocks/post-date/style.css	61 B
build/block-library/blocks/post-excerpt/editor-rtl.css	73 B
build/block-library/blocks/post-excerpt/editor.css	73 B
build/block-library/blocks/post-excerpt/style-rtl.css	69 B
build/block-library/blocks/post-excerpt/style.css	69 B
build/block-library/blocks/post-featured-image/editor-rtl.css	547 B
build/block-library/blocks/post-featured-image/editor.css	545 B
build/block-library/blocks/post-featured-image/style-rtl.css	315 B
build/block-library/blocks/post-featured-image/style.css	315 B
build/block-library/blocks/post-navigation-link/style-rtl.css	153 B
build/block-library/blocks/post-navigation-link/style.css	153 B
build/block-library/blocks/post-template/editor-rtl.css	99 B
build/block-library/blocks/post-template/editor.css	98 B
build/block-library/blocks/post-template/style-rtl.css	282 B
build/block-library/blocks/post-template/style.css	282 B
build/block-library/blocks/post-terms/style-rtl.css	73 B
build/block-library/blocks/post-terms/style.css	73 B
build/block-library/blocks/post-title/style-rtl.css	100 B
build/block-library/blocks/post-title/style.css	100 B
build/block-library/blocks/preformatted/style-rtl.css	103 B
build/block-library/blocks/preformatted/style.css	103 B
build/block-library/blocks/pullquote/editor-rtl.css	135 B
build/block-library/blocks/pullquote/editor.css	135 B
build/block-library/blocks/pullquote/style-rtl.css	326 B
build/block-library/blocks/pullquote/style.css	325 B
build/block-library/blocks/pullquote/theme-rtl.css	167 B
build/block-library/blocks/pullquote/theme.css	167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css	122 B
build/block-library/blocks/query-pagination-numbers/editor.css	121 B
build/block-library/blocks/query-pagination/editor-rtl.css	221 B
build/block-library/blocks/query-pagination/editor.css	211 B
build/block-library/blocks/query-pagination/style-rtl.css	282 B
build/block-library/blocks/query-pagination/style.css	278 B
build/block-library/blocks/query-title/style-rtl.css	63 B
build/block-library/blocks/query-title/style.css	63 B
build/block-library/blocks/query/editor-rtl.css	439 B
build/block-library/blocks/query/editor.css	439 B
build/block-library/blocks/quote/style-rtl.css	213 B
build/block-library/blocks/quote/style.css	213 B
build/block-library/blocks/quote/theme-rtl.css	223 B
build/block-library/blocks/quote/theme.css	226 B
build/block-library/blocks/read-more/style-rtl.css	132 B
build/block-library/blocks/read-more/style.css	132 B
build/block-library/blocks/rss/editor-rtl.css	202 B
build/block-library/blocks/rss/editor.css	204 B
build/block-library/blocks/rss/style-rtl.css	289 B
build/block-library/blocks/rss/style.css	288 B
build/block-library/blocks/search/editor-rtl.css	165 B
build/block-library/blocks/search/editor.css	165 B
build/block-library/blocks/search/style-rtl.css	409 B
build/block-library/blocks/search/style.css	406 B
build/block-library/blocks/search/theme-rtl.css	114 B
build/block-library/blocks/search/theme.css	114 B
build/block-library/blocks/separator/editor-rtl.css	146 B
build/block-library/blocks/separator/editor.css	146 B
build/block-library/blocks/separator/style-rtl.css	234 B
build/block-library/blocks/separator/style.css	234 B
build/block-library/blocks/separator/theme-rtl.css	194 B
build/block-library/blocks/separator/theme.css	194 B
build/block-library/blocks/shortcode/editor-rtl.css	464 B
build/block-library/blocks/shortcode/editor.css	464 B
build/block-library/blocks/site-logo/editor-rtl.css	488 B
build/block-library/blocks/site-logo/editor.css	488 B
build/block-library/blocks/site-logo/style-rtl.css	203 B
build/block-library/blocks/site-logo/style.css	203 B
build/block-library/blocks/site-tagline/editor-rtl.css	86 B
build/block-library/blocks/site-tagline/editor.css	86 B
build/block-library/blocks/site-title/editor-rtl.css	84 B
build/block-library/blocks/site-title/editor.css	84 B
build/block-library/blocks/social-link/editor-rtl.css	184 B
build/block-library/blocks/social-link/editor.css	184 B
build/block-library/blocks/social-links/editor-rtl.css	674 B
build/block-library/blocks/social-links/editor.css	673 B
build/block-library/blocks/social-links/style-rtl.css	1.4 kB
build/block-library/blocks/social-links/style.css	1.39 kB
build/block-library/blocks/spacer/editor-rtl.css	322 B
build/block-library/blocks/spacer/editor.css	322 B
build/block-library/blocks/spacer/style-rtl.css	48 B
build/block-library/blocks/spacer/style.css	48 B
build/block-library/blocks/table/editor-rtl.css	494 B
build/block-library/blocks/table/editor.css	494 B
build/block-library/blocks/table/style-rtl.css	611 B
build/block-library/blocks/table/style.css	609 B
build/block-library/blocks/table/theme-rtl.css	190 B
build/block-library/blocks/table/theme.css	190 B
build/block-library/blocks/tag-cloud/style-rtl.css	239 B
build/block-library/blocks/tag-cloud/style.css	239 B
build/block-library/blocks/template-part/editor-rtl.css	235 B
build/block-library/blocks/template-part/editor.css	235 B
build/block-library/blocks/template-part/theme-rtl.css	101 B
build/block-library/blocks/template-part/theme.css	101 B
build/block-library/blocks/text-columns/editor-rtl.css	95 B
build/block-library/blocks/text-columns/editor.css	95 B
build/block-library/blocks/text-columns/style-rtl.css	166 B
build/block-library/blocks/text-columns/style.css	166 B
build/block-library/blocks/verse/style-rtl.css	87 B
build/block-library/blocks/verse/style.css	87 B
build/block-library/blocks/video/editor-rtl.css	561 B
build/block-library/blocks/video/editor.css	563 B
build/block-library/blocks/video/style-rtl.css	174 B
build/block-library/blocks/video/style.css	174 B
build/block-library/blocks/video/theme-rtl.css	126 B
build/block-library/blocks/video/theme.css	126 B
build/block-library/common-rtl.css	1.02 kB
build/block-library/common.css	1.02 kB
build/block-library/editor-elements-rtl.css	75 B
build/block-library/editor-elements.css	75 B
build/block-library/elements-rtl.css	54 B
build/block-library/elements.css	54 B
build/block-library/reset-rtl.css	478 B
build/block-library/reset.css	478 B
build/block-library/style-rtl.css	12.2 kB
build/block-library/style.css	12.2 kB
build/block-library/theme-rtl.css	719 B
build/block-library/theme.css	722 B
build/block-serialization-default-parser/index.min.js	1.1 kB
build/block-serialization-spec-parser/index.min.js	2.83 kB
build/blocks/index.min.js	49.8 kB
build/components/style-rtl.css	11.4 kB
build/components/style.css	11.5 kB
build/core-data/index.min.js	15.5 kB
build/customize-widgets/index.min.js	11.3 kB
build/data-controls/index.min.js	653 B
build/data/index.min.js	8.08 kB
build/date/index.min.js	32.1 kB
build/deprecated/index.min.js	507 B
build/dom-ready/index.min.js	324 B
build/dom/index.min.js	4.7 kB
build/edit-navigation/index.min.js	16 kB
build/edit-navigation/style-rtl.css	3.99 kB
build/edit-navigation/style.css	4 kB
build/edit-post/classic-rtl.css	546 B
build/edit-post/classic.css	547 B
build/edit-post/style-rtl.css	6.94 kB
build/edit-post/style.css	6.94 kB
build/edit-site/style-rtl.css	8.36 kB
build/edit-site/style.css	8.34 kB
build/edit-widgets/index.min.js	16.5 kB
build/edit-widgets/style-rtl.css	4.34 kB
build/edit-widgets/style.css	4.34 kB
build/element/index.min.js	4.68 kB
build/escape-html/index.min.js	537 B
build/format-library/index.min.js	6.76 kB
build/format-library/style-rtl.css	571 B
build/format-library/style.css	571 B
build/hooks/index.min.js	1.64 kB
build/html-entities/index.min.js	448 B
build/i18n/index.min.js	3.77 kB
build/is-shallow-equal/index.min.js	527 B
build/keyboard-shortcuts/index.min.js	1.78 kB
build/keycodes/index.min.js	1.83 kB
build/list-reusable-blocks/index.min.js	1.74 kB
build/list-reusable-blocks/style-rtl.css	835 B
build/list-reusable-blocks/style.css	835 B
build/media-utils/index.min.js	2.93 kB
build/notices/index.min.js	953 B
build/nux/index.min.js	2.05 kB
build/nux/style-rtl.css	732 B
build/nux/style.css	728 B
build/plugins/index.min.js	1.94 kB
build/preferences-persistence/index.min.js	2.22 kB
build/preferences/index.min.js	1.3 kB
build/primitives/index.min.js	933 B
build/priority-queue/index.min.js	1.58 kB
build/react-i18n/index.min.js	696 B
build/react-refresh-entry/index.min.js	8.44 kB
build/react-refresh-runtime/index.min.js	7.31 kB
build/redux-routine/index.min.js	2.74 kB
build/reusable-blocks/index.min.js	2.21 kB
build/reusable-blocks/style-rtl.css	256 B
build/reusable-blocks/style.css	256 B
build/rich-text/index.min.js	10.6 kB
build/server-side-render/index.min.js	1.61 kB
build/shortcode/index.min.js	1.53 kB
build/style-engine/index.min.js	1.45 kB
build/token-list/index.min.js	644 B
build/url/index.min.js	3.61 kB
build/vendors/react-dom.min.js	38.5 kB
build/vendors/react.min.js	4.34 kB
build/viewport/index.min.js	1.09 kB
build/warning/index.min.js	268 B
build/widgets/index.min.js	7.18 kB
build/widgets/style-rtl.css	1.18 kB
build/widgets/style.css	1.19 kB
build/wordcount/index.min.js	1.06 kB

_{compressed-size-action}

[mtias]

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?

[jasmussen]

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:

• We con't use any upload text for the sibling inserter, whether dropping one or multiple images. So it doesn't feel like we need any here either — in that light, it could just be the a blue box that covers the entire paragraph. We could show an icon if we need to, but it seems better to not show anything, than show cropped text/icons.
• If need be, we can show text and/or icons if there's space. Global Styles would let you set both a big and a small font size, and in some cases a paragraph might be of varying sizes. That's worth considering for the implementation as well.

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.

[kevin940726]

Thanks for trying it out! I'm currently busy helping out with other things. I'll get back to it soon.

[kevin940726]

I refactored it to use useOnBlockDrop to implement the drop-zone. Haven't yet updated the tests and cleaned up the mess but LMK what you think!

https://user-images.githubusercontent.com/7753001/190333398-6b42e10d-4365-43df-9439-147010d4d122.mp4

I'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?

[jasmussen]

I'm seeing perfect positioning, the right border radius and color, and the right width/height, and some nice animation:

Very cool, this is close! Right now it looks like it's scaling both x and y axes. Can we scale only the y axis, so it starts as a full-width line that grows thicker?

That might thread the needle! Looks good!

[mtias]

This is looking pretty good!

[jasmussen]

The main question I have is whether the empty paragraph should intercept dragged blocks as well as files, or if it should only handle files? At the moment it's handling both blocks and files, but I didn't see a discussion about that anywhere.

Good catch. I would lean towards it being okay to replace empty paragraphs with other blocks or patterns being dragged in there; presumably we can always pull that back if it turns out to be a problem in practice.

[mtias]

Handling both should be fine, as we consider an empty paragraph a drop-zone in general.

[youknowriad]

Hello folks, I didn't dive into the PR but this has a very bad impact on several performance metrics. Check https://codehealth.vercel.app

[paaljoachim]

Thank you for working on this Kai! This will be very helpful!

Testing using Twenty Twenty Two. WP 6.0.2

This is very helpful! Dragging and dropping an image into where the Paragraph block is.

I happen to do a very quick CTRL+Z as in undoing the image upload.

It went into a loop.

I waited a few minutes before refreshing/reloading the page. Then went into the revision screen to bring back the image I just CTRL+Z ed away.

It now looks like this: I will wait a few more minutes to see if the gif spinner ends by itself or if I need to again refresh the page.

A few minutes have passed. I will just save the page. Then delete the partly uploaded image.

[kevin940726]

@youknowriad Seems like the performance regression is from this line:

https://github.com/WordPress/gutenberg/blob/9580611aaa63b80a3ac490c11dfa83db52b923e5/packages/e2e-tests/specs/performance/post-editor.test.js#L160-L162

We're creating 1000 empty paragraph blocks to simulate some dummy blocks. However, since this PR, we're now also rendering 1000 dropzones for those 1000 empty paragraph blocks:

https://github.com/WordPress/gutenberg/blob/9580611aaa63b80a3ac490c11dfa83db52b923e5/packages/block-library/src/paragraph/edit.js#L118-L123

Here I made a choice to defer rendering the dropzone only when the paragraph block is empty, which I assume in practice should be a relatively rare case that someone would create dozens of empty paragraph blocks in the content. This assumption however isn't the case in the performance test.

FWIW, I'm not sure why we're not resetting the page between each test in the performance test suite? This causes some former metrics to alter the results of the latter ones. In this case, all metrics below Selecting blocks is skewed.

An obvious solution is to avoid rendering the dropzone completely when the user is not dragging. This is however not possible without some refactoring and/or requiring a <DropZoneProvider> to only listen to events once on the topmost level.

Another solution is to change the performance test to not render empty paragraph blocks 1000 times? Maybe simply adding some dummy text to the content attribute would fix this issue?

[kevin940726]

@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 https://github.com/WordPress/gutenberg/pull/42001, a naughty useEffect is not being used correctly in the image block 😅.

[youknowriad]

@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.

[kevin940726]

@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 <Popover> for each dropzone to correctly position itself on top of the paragraph block (to avoid changing the DOM structure). That's 1000 popovers, which is likely what causes the page to be unresponsive. AFAIK, that's not an issue for other dropzones (media-placeholder for instance) because they just render a div positioned relatively.

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.

[youknowriad]

do have some plans to refactor drop zones to share the same state and events globally though

For what it's worth that's how drop zones were implemented originally, and it was refactored (simplified I guess) to what it is today either by @talldan or @ellatrix I don't remember exactly

[ellatrix]

However, I guess the main bottleneck here is that we're using <Popover> for each dropzone to correctly position itself on top of the paragraph block (to avoid changing the DOM structure). That's 1000 popovers, which is likely what causes the page to be unresponsive.

Why are we rendering Popovers for empty paragraphs? A Popover should only be rendered if you're dragging over the block?

[youknowriad]

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?

[talldan]

For what it's worth that's how drop zones were implemented originally, and it was refactored (simplified I guess) to what it is today either by @talldan or @ellatrix I don't remember exactly

Yep, there was a DropZoneProvider that was removed from the editors in a PR by @ellatrix.

Worth noting though that it sounds like there's a desire to have some kind of top-level drop zone - https://github.com/WordPress/gutenberg/pull/43493#issuecomment-1250967222. 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.

If it's hard to track, maybe we can we maybe consider reverting temporarily?

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.

[youknowriad]

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.

[kevin940726]

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

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.

[youknowriad]

@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.

[ellatrix]

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 isDraggingOver flag, and only maybe then render a Popover to indicate that it's possible to drop.

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 useBlockProps or the BlockListBlock component, or through the editor.BlockListBlock filter, using isUnmodifiedDefaultBlock as a condition for attaching drag listeners.

[kevin940726]

These are the exact same tests, 1000 empty paragraphs like the actual performance test

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.

We should use the block element ref to attach a drag listener to set an isDraggingOver flag, and only maybe then render a Popover to indicate that it's possible to drop.

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 dragover event, we'll have to render the Popover on top which will then trigger dragleave of the same component. It's still possible though using some OR logic and I think it could be a good follow-up.

There's no need for a drag/drop provider? I don't follow the reasoning there.

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.

And what if we want to extend this to other empty/unmodified blocks? What if the default block is changed to another block?

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 drop-zone component in a modular way so it should not be too hard to remove it whenever needed.

using isUnmodifiedDefaultBlock as a condition for attaching drag listeners

Ah cool! I didn't even know isUnmodifiedDefaultBlock is a thing before 😅. I agree that would be interesting to try it out in a follow-up.

[youknowriad]

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)

[talldan]

@youknowriad I did try setting that to false in a PR here - Test popover animation frame performance.

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.

[kevin940726]

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 <InsertionPoint> component since they are similar in some way and we can reuse most of the logic and positioning components there.

I'll try to share more results soon.

[ellatrix]

@kevin940726 Ping me for a review

[youknowriad]

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.

@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.

[talldan]

@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 😄