Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move insertionPoint state to block-editor store/rename existing insertionPoint to insertionCue #65098

Merged
merged 25 commits into from
Sep 26, 2024

Conversation

jeryj
Copy link
Contributor

@jeryj jeryj commented Sep 5, 2024

What?

In the block editor store:

  • Change insertionPoint reducer to insertionCue (as it is where the blue line shows up not where insertion is guaranteed to happen).
  • This opens us insertionPoint to be used to manually pass an insertion point (such as when clicking an inline inserter button like the blue zoom out inserter buttons)
  • Add setInsertionPoint action
  • Adds private getInsertionPoint selector

In the editor store:

  • setIsInserterOpened dispatches setInsertionPoint to the block editor store
  • Rename editor selector getInsertionPoint to getInserter to more clearly communicate this is the state of the inserter, not solely about the insertion point.

Why?/How

Now that the inserter can remain open while interacting with the block canvas, it is expected that when you select a new block, the insertion will occur after the selected block.

The insertion point is handled by the editor package via setIsInserterOpened( value ), which can take an object to open the inserter to a certain tab panel and also provide the place where the insertion should happen. We want to change this insertion point when selecting a new block on the canvas. Block selection is handled via the block editor package. Since the block editor should not know about the editor, and we can't rely on the SELECTION_CHANGE action to clear the insertion point within the editor store, I've moved the insertion point data to be within the block editor package.

In order to clear up confusion, I've also renamed some state and selectors:

  • Block editor insertionPoint state becomes insertionCue, as this more accurately names it. It was not the insertion point, but the place where we want to show the blue line on the canvas.
  • This opens up insertionPoint to be used for manually setting an insertion point, if needed, such as when opening the inserter via one of the inline inserter buttons.
  • In the editor package, getInsertionPoint is renamed to getInserter, as this was a point of confusion. It is only the insertion point if the inserter is open and becomes stale data as soon as another interaction on the canvas happens. This is about the sate of the inserter when it is opened, and if it should still be open. getInserter is a better name, and it could still be better. getInserterPanel maybe?

How?

  • Renames state.insertionPoint to state.insertionCue in the block editor store and related action/reducers.
  • Adds state.insertionPoint to store when an insertion point is manually passed.
  • Emit the private setInsertionPoint action in the block editor store when setIsInserterOpen has an root index and insertion point set.
  • Clear insertionPoint state on SELECT_BLOCK
  • Renames private editor selector getInsertionPoint to getInserter to more accurately communicate that this is the state the inserter initialized to rather than the final insertion point.

Testing Instructions

For pattern insertion points in Zoom Out

  1. Open the Site Editor
  2. Enter zoom out mode (this isn't necessary but it makes it much easier to test) - you can enter zoom out by selecting a pattern category in the inserter or by using the 50% desktop option in the device preview dropdown)
  3. Click one on the [+] in the canvas, to set an insertion point. Notice that a wide blue line appears between the two blocks.
  4. Select another block, or clear block selection. Notice that that wide blue line disappears.
  5. Now select a pattern in the canvas. This pattern should be inserted after the newly selected block, not at the place where the old insertion point was.
  6. Create an insertion point by clicking the [+]. Hover a pattern in the inserter. You should still see the wide blue line in the canvas.
  7. Now click the pattern. It should be added to the place that the insertion point indicator was.

Default Editing

  • Use the various in-between inserters and inserters in both group and root level contexts (like a new post).
  • See if insertion always happens where you would expect it to
  • If it is unexpected, check to see if it's different than trunk

Testing Instructions for Keyboard

Screenshots or screencast

Copy link

github-actions bot commented Sep 5, 2024

Size Change: +607 B (+0.03%)

Total Size: 1.77 MB

Filename Size Change
build-module/interactivity-router/index.min.js 3 kB -6 B (-0.2%)
build/blob/index.min.js 579 B -15 B (-2.53%)
build/block-directory/index.min.js 7.26 kB +145 B (+2.04%)
build/block-directory/style-rtl.css 1.07 kB +56 B (+5.52%) 🔍
build/block-directory/style.css 1.07 kB +55 B (+5.42%) 🔍
build/block-editor/content-rtl.css 4.42 kB -49 B (-1.1%)
build/block-editor/content.css 4.41 kB -51 B (-1.14%)
build/block-editor/index.min.js 256 kB -408 B (-0.16%)
build/block-editor/style-rtl.css 15.6 kB -28 B (-0.18%)
build/block-editor/style.css 15.5 kB -28 B (-0.18%)
build/block-library/blocks/form/view.min.js 470 B -14 B (-2.89%)
build/blocks/index.min.js 52.5 kB -17 B (-0.03%)
build/components/index.min.js 226 kB +728 B (+0.32%)
build/core-data/index.min.js 73.4 kB -19 B (-0.03%)
build/dom/index.min.js 4.66 kB -1 B (-0.02%)
build/edit-post/index.min.js 13.6 kB +1 B (+0.01%)
build/edit-site/index.min.js 217 kB -13 B (-0.01%)
build/edit-site/posts-rtl.css 7.36 kB +60 B (+0.82%)
build/edit-site/posts.css 7.36 kB +62 B (+0.85%)
build/edit-site/style-rtl.css 12.6 kB +64 B (+0.51%)
build/edit-site/style.css 12.6 kB +68 B (+0.54%)
build/edit-widgets/index.min.js 17.8 kB +57 B (+0.32%)
build/editor/index.min.js 103 kB +43 B (+0.04%)
build/rich-text/index.min.js 10.1 kB -44 B (-0.44%)
build/url/index.min.js 3.9 kB -25 B (-0.64%)
build/widgets/index.min.js 7.17 kB -14 B (-0.19%)
ℹ️ View Unchanged
Filename Size
build-module/a11y/index.min.js 482 B
build-module/block-library/file/view.min.js 447 B
build-module/block-library/image/view.min.js 1.78 kB
build-module/block-library/navigation/view.min.js 1.16 kB
build-module/block-library/query/view.min.js 743 B
build-module/block-library/search/view.min.js 616 B
build-module/interactivity/debug.min.js 16.7 kB
build-module/interactivity/index.min.js 13.4 kB
build/a11y/index.min.js 952 B
build/annotations/index.min.js 2.26 kB
build/api-fetch/index.min.js 2.32 kB
build/autop/index.min.js 2.12 kB
build/block-editor/default-editor-styles-rtl.css 394 B
build/block-editor/default-editor-styles.css 394 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 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 149 B
build/block-library/blocks/audio/editor.css 151 B
build/block-library/blocks/audio/style-rtl.css 132 B
build/block-library/blocks/audio/style.css 132 B
build/block-library/blocks/audio/theme-rtl.css 134 B
build/block-library/blocks/audio/theme.css 134 B
build/block-library/blocks/avatar/editor-rtl.css 115 B
build/block-library/blocks/avatar/editor.css 115 B
build/block-library/blocks/avatar/style-rtl.css 104 B
build/block-library/blocks/avatar/style.css 104 B
build/block-library/blocks/button/editor-rtl.css 265 B
build/block-library/blocks/button/editor.css 265 B
build/block-library/blocks/button/style-rtl.css 538 B
build/block-library/blocks/button/style.css 538 B
build/block-library/blocks/buttons/editor-rtl.css 291 B
build/block-library/blocks/buttons/editor.css 291 B
build/block-library/blocks/buttons/style-rtl.css 328 B
build/block-library/blocks/buttons/style.css 328 B
build/block-library/blocks/calendar/style-rtl.css 240 B
build/block-library/blocks/calendar/style.css 240 B
build/block-library/blocks/categories/editor-rtl.css 132 B
build/block-library/blocks/categories/editor.css 131 B
build/block-library/blocks/categories/style-rtl.css 152 B
build/block-library/blocks/categories/style.css 152 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 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 122 B
build/block-library/blocks/code/theme.css 122 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 420 B
build/block-library/blocks/columns/style.css 420 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 124 B
build/block-library/blocks/comment-author-avatar/editor.css 124 B
build/block-library/blocks/comment-author-name/style-rtl.css 72 B
build/block-library/blocks/comment-author-name/style.css 72 B
build/block-library/blocks/comment-content/style-rtl.css 120 B
build/block-library/blocks/comment-content/style.css 120 B
build/block-library/blocks/comment-date/style-rtl.css 65 B
build/block-library/blocks/comment-date/style.css 65 B
build/block-library/blocks/comment-edit-link/style-rtl.css 70 B
build/block-library/blocks/comment-edit-link/style.css 70 B
build/block-library/blocks/comment-reply-link/style-rtl.css 71 B
build/block-library/blocks/comment-reply-link/style.css 71 B
build/block-library/blocks/comment-template/style-rtl.css 200 B
build/block-library/blocks/comment-template/style.css 199 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 228 B
build/block-library/blocks/comments-pagination/editor.css 217 B
build/block-library/blocks/comments-pagination/style-rtl.css 234 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 832 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 631 B
build/block-library/blocks/cover/editor-rtl.css 641 B
build/block-library/blocks/cover/editor.css 642 B
build/block-library/blocks/cover/style-rtl.css 1.62 kB
build/block-library/blocks/cover/style.css 1.6 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 86 B
build/block-library/blocks/details/style.css 86 B
build/block-library/blocks/embed/editor-rtl.css 331 B
build/block-library/blocks/embed/editor.css 331 B
build/block-library/blocks/embed/style-rtl.css 419 B
build/block-library/blocks/embed/style.css 419 B
build/block-library/blocks/embed/theme-rtl.css 133 B
build/block-library/blocks/embed/theme.css 133 B
build/block-library/blocks/file/editor-rtl.css 326 B
build/block-library/blocks/file/editor.css 326 B
build/block-library/blocks/file/style-rtl.css 278 B
build/block-library/blocks/file/style.css 279 B
build/block-library/blocks/file/view.min.js 324 B
build/block-library/blocks/footnotes/style-rtl.css 198 B
build/block-library/blocks/footnotes/style.css 197 B
build/block-library/blocks/form-input/editor-rtl.css 229 B
build/block-library/blocks/form-input/editor.css 229 B
build/block-library/blocks/form-input/style-rtl.css 357 B
build/block-library/blocks/form-input/style.css 357 B
build/block-library/blocks/form-submission-notification/editor-rtl.css 344 B
build/block-library/blocks/form-submission-notification/editor.css 341 B
build/block-library/blocks/form-submit-button/style-rtl.css 69 B
build/block-library/blocks/form-submit-button/style.css 69 B
build/block-library/blocks/freeform/editor-rtl.css 2.6 kB
build/block-library/blocks/freeform/editor.css 2.6 kB
build/block-library/blocks/gallery/editor-rtl.css 946 B
build/block-library/blocks/gallery/editor.css 951 B
build/block-library/blocks/gallery/style-rtl.css 1.83 kB
build/block-library/blocks/gallery/style.css 1.82 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 334 B
build/block-library/blocks/group/editor.css 334 B
build/block-library/blocks/group/style-rtl.css 103 B
build/block-library/blocks/group/style.css 103 B
build/block-library/blocks/group/theme-rtl.css 79 B
build/block-library/blocks/group/theme.css 79 B
build/block-library/blocks/heading/style-rtl.css 188 B
build/block-library/blocks/heading/style.css 188 B
build/block-library/blocks/html/editor-rtl.css 346 B
build/block-library/blocks/html/editor.css 347 B
build/block-library/blocks/image/editor-rtl.css 785 B
build/block-library/blocks/image/editor.css 787 B
build/block-library/blocks/image/style-rtl.css 1.59 kB
build/block-library/blocks/image/style.css 1.59 kB
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/image/view.min.js 1.65 kB
build/block-library/blocks/latest-comments/style-rtl.css 355 B
build/block-library/blocks/latest-comments/style.css 354 B
build/block-library/blocks/latest-posts/editor-rtl.css 179 B
build/block-library/blocks/latest-posts/editor.css 179 B
build/block-library/blocks/latest-posts/style-rtl.css 509 B
build/block-library/blocks/latest-posts/style.css 510 B
build/block-library/blocks/list/style-rtl.css 107 B
build/block-library/blocks/list/style.css 107 B
build/block-library/blocks/loginout/style-rtl.css 61 B
build/block-library/blocks/loginout/style.css 61 B
build/block-library/blocks/media-text/editor-rtl.css 321 B
build/block-library/blocks/media-text/editor.css 320 B
build/block-library/blocks/media-text/style-rtl.css 558 B
build/block-library/blocks/media-text/style.css 556 B
build/block-library/blocks/more/editor-rtl.css 427 B
build/block-library/blocks/more/editor.css 427 B
build/block-library/blocks/navigation-link/editor-rtl.css 644 B
build/block-library/blocks/navigation-link/editor.css 645 B
build/block-library/blocks/navigation-link/style-rtl.css 192 B
build/block-library/blocks/navigation-link/style.css 191 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 295 B
build/block-library/blocks/navigation-submenu/editor.css 294 B
build/block-library/blocks/navigation/editor-rtl.css 2.19 kB
build/block-library/blocks/navigation/editor.css 2.2 kB
build/block-library/blocks/navigation/style-rtl.css 2.25 kB
build/block-library/blocks/navigation/style.css 2.23 kB
build/block-library/blocks/navigation/view.min.js 1.03 kB
build/block-library/blocks/nextpage/editor-rtl.css 392 B
build/block-library/blocks/nextpage/editor.css 392 B
build/block-library/blocks/page-list/editor-rtl.css 378 B
build/block-library/blocks/page-list/editor.css 378 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/editor-rtl.css 236 B
build/block-library/blocks/paragraph/editor.css 236 B
build/block-library/blocks/paragraph/style-rtl.css 341 B
build/block-library/blocks/paragraph/style.css 340 B
build/block-library/blocks/post-author-biography/style-rtl.css 74 B
build/block-library/blocks/post-author-biography/style.css 74 B
build/block-library/blocks/post-author-name/style-rtl.css 69 B
build/block-library/blocks/post-author-name/style.css 69 B
build/block-library/blocks/post-author/editor-rtl.css 107 B
build/block-library/blocks/post-author/editor.css 107 B
build/block-library/blocks/post-author/style-rtl.css 188 B
build/block-library/blocks/post-author/style.css 189 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 527 B
build/block-library/blocks/post-comments-form/style.css 528 B
build/block-library/blocks/post-content/editor-rtl.css 74 B
build/block-library/blocks/post-content/editor.css 74 B
build/block-library/blocks/post-content/style-rtl.css 79 B
build/block-library/blocks/post-content/style.css 79 B
build/block-library/blocks/post-date/style-rtl.css 62 B
build/block-library/blocks/post-date/style.css 62 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 155 B
build/block-library/blocks/post-excerpt/style.css 155 B
build/block-library/blocks/post-featured-image/editor-rtl.css 729 B
build/block-library/blocks/post-featured-image/editor.css 726 B
build/block-library/blocks/post-featured-image/style-rtl.css 347 B
build/block-library/blocks/post-featured-image/style.css 347 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 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 399 B
build/block-library/blocks/post-template/style.css 398 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 70 B
build/block-library/blocks/post-time-to-read/style.css 70 B
build/block-library/blocks/post-title/style-rtl.css 162 B
build/block-library/blocks/post-title/style.css 162 B
build/block-library/blocks/preformatted/style-rtl.css 125 B
build/block-library/blocks/preformatted/style.css 125 B
build/block-library/blocks/pullquote/editor-rtl.css 134 B
build/block-library/blocks/pullquote/editor.css 134 B
build/block-library/blocks/pullquote/style-rtl.css 342 B
build/block-library/blocks/pullquote/style.css 342 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 121 B
build/block-library/blocks/query-pagination-numbers/editor.css 118 B
build/block-library/blocks/query-pagination/editor-rtl.css 154 B
build/block-library/blocks/query-pagination/editor.css 154 B
build/block-library/blocks/query-pagination/style-rtl.css 237 B
build/block-library/blocks/query-pagination/style.css 237 B
build/block-library/blocks/query-title/style-rtl.css 64 B
build/block-library/blocks/query-title/style.css 64 B
build/block-library/blocks/query/editor-rtl.css 452 B
build/block-library/blocks/query/editor.css 451 B
build/block-library/blocks/query/view.min.js 958 B
build/block-library/blocks/quote/style-rtl.css 238 B
build/block-library/blocks/quote/style.css 238 B
build/block-library/blocks/quote/theme-rtl.css 233 B
build/block-library/blocks/quote/theme.css 236 B
build/block-library/blocks/read-more/style-rtl.css 138 B
build/block-library/blocks/read-more/style.css 138 B
build/block-library/blocks/rss/editor-rtl.css 101 B
build/block-library/blocks/rss/editor.css 101 B
build/block-library/blocks/rss/style-rtl.css 288 B
build/block-library/blocks/rss/style.css 287 B
build/block-library/blocks/search/editor-rtl.css 199 B
build/block-library/blocks/search/editor.css 199 B
build/block-library/blocks/search/style-rtl.css 672 B
build/block-library/blocks/search/style.css 671 B
build/block-library/blocks/search/theme-rtl.css 113 B
build/block-library/blocks/search/theme.css 113 B
build/block-library/blocks/search/view.min.js 475 B
build/block-library/blocks/separator/editor-rtl.css 100 B
build/block-library/blocks/separator/editor.css 100 B
build/block-library/blocks/separator/style-rtl.css 248 B
build/block-library/blocks/separator/style.css 248 B
build/block-library/blocks/separator/theme-rtl.css 195 B
build/block-library/blocks/separator/theme.css 195 B
build/block-library/blocks/shortcode/editor-rtl.css 286 B
build/block-library/blocks/shortcode/editor.css 286 B
build/block-library/blocks/site-logo/editor-rtl.css 806 B
build/block-library/blocks/site-logo/editor.css 803 B
build/block-library/blocks/site-logo/style-rtl.css 218 B
build/block-library/blocks/site-logo/style.css 218 B
build/block-library/blocks/site-tagline/editor-rtl.css 87 B
build/block-library/blocks/site-tagline/editor.css 87 B
build/block-library/blocks/site-tagline/style-rtl.css 65 B
build/block-library/blocks/site-tagline/style.css 65 B
build/block-library/blocks/site-title/editor-rtl.css 85 B
build/block-library/blocks/site-title/editor.css 85 B
build/block-library/blocks/site-title/style-rtl.css 143 B
build/block-library/blocks/site-title/style.css 143 B
build/block-library/blocks/social-link/editor-rtl.css 338 B
build/block-library/blocks/social-link/editor.css 338 B
build/block-library/blocks/social-links/editor-rtl.css 757 B
build/block-library/blocks/social-links/editor.css 756 B
build/block-library/blocks/social-links/style-rtl.css 1.51 kB
build/block-library/blocks/social-links/style.css 1.5 kB
build/block-library/blocks/spacer/editor-rtl.css 346 B
build/block-library/blocks/spacer/editor.css 346 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-of-contents/style-rtl.css 83 B
build/block-library/blocks/table-of-contents/style.css 83 B
build/block-library/blocks/table/editor-rtl.css 394 B
build/block-library/blocks/table/editor.css 394 B
build/block-library/blocks/table/style-rtl.css 640 B
build/block-library/blocks/table/style.css 639 B
build/block-library/blocks/table/theme-rtl.css 152 B
build/block-library/blocks/table/theme.css 152 B
build/block-library/blocks/tag-cloud/editor-rtl.css 144 B
build/block-library/blocks/tag-cloud/editor.css 144 B
build/block-library/blocks/tag-cloud/style-rtl.css 266 B
build/block-library/blocks/tag-cloud/style.css 265 B
build/block-library/blocks/template-part/editor-rtl.css 368 B
build/block-library/blocks/template-part/editor.css 368 B
build/block-library/blocks/template-part/theme-rtl.css 113 B
build/block-library/blocks/template-part/theme.css 113 B
build/block-library/blocks/term-description/style-rtl.css 126 B
build/block-library/blocks/term-description/style.css 126 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 165 B
build/block-library/blocks/text-columns/style.css 165 B
build/block-library/blocks/verse/style-rtl.css 98 B
build/block-library/blocks/verse/style.css 98 B
build/block-library/blocks/video/editor-rtl.css 396 B
build/block-library/blocks/video/editor.css 397 B
build/block-library/blocks/video/style-rtl.css 192 B
build/block-library/blocks/video/style.css 192 B
build/block-library/blocks/video/theme-rtl.css 134 B
build/block-library/blocks/video/theme.css 134 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.1 kB
build/block-library/common.css 1.1 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 11.7 kB
build/block-library/editor.css 11.7 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 219 kB
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/style-rtl.css 14.9 kB
build/block-library/style.css 14.9 kB
build/block-library/theme-rtl.css 708 B
build/block-library/theme.css 712 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/commands/index.min.js 16.1 kB
build/commands/style-rtl.css 955 B
build/commands/style.css 952 B
build/components/style-rtl.css 12.3 kB
build/components/style.css 12.3 kB
build/compose/index.min.js 12.7 kB
build/core-commands/index.min.js 3.11 kB
build/customize-widgets/index.min.js 11 kB
build/customize-widgets/style-rtl.css 1.35 kB
build/customize-widgets/style.css 1.35 kB
build/data-controls/index.min.js 641 B
build/data/index.min.js 8.98 kB
build/date/index.min.js 18 kB
build/deprecated/index.min.js 458 B
build/dom-ready/index.min.js 325 B
build/edit-post/classic-rtl.css 578 B
build/edit-post/classic.css 580 B
build/edit-post/style-rtl.css 2.54 kB
build/edit-post/style.css 2.54 kB
build/edit-widgets/style-rtl.css 4.19 kB
build/edit-widgets/style.css 4.19 kB
build/editor/style-rtl.css 9.34 kB
build/editor/style.css 9.34 kB
build/element/index.min.js 4.83 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 8.11 kB
build/format-library/style-rtl.css 476 B
build/format-library/style.css 476 B
build/hooks/index.min.js 1.54 kB
build/html-entities/index.min.js 445 B
build/i18n/index.min.js 3.58 kB
build/is-shallow-equal/index.min.js 526 B
build/keyboard-shortcuts/index.min.js 1.31 kB
build/keycodes/index.min.js 1.46 kB
build/list-reusable-blocks/index.min.js 2.18 kB
build/list-reusable-blocks/style-rtl.css 846 B
build/list-reusable-blocks/style.css 846 B
build/media-utils/index.min.js 3.2 kB
build/notices/index.min.js 946 B
build/nux/index.min.js 1.61 kB
build/nux/style-rtl.css 749 B
build/nux/style.css 745 B
build/patterns/index.min.js 7.34 kB
build/patterns/style-rtl.css 687 B
build/patterns/style.css 685 B
build/plugins/index.min.js 1.81 kB
build/preferences-persistence/index.min.js 2.06 kB
build/preferences/index.min.js 2.9 kB
build/preferences/style-rtl.css 554 B
build/preferences/style.css 554 B
build/primitives/index.min.js 829 B
build/priority-queue/index.min.js 1.54 kB
build/private-apis/index.min.js 1.01 kB
build/react-i18n/index.min.js 630 B
build/react-refresh-entry/index.min.js 9.47 kB
build/react-refresh-runtime/index.min.js 6.76 kB
build/redux-routine/index.min.js 2.69 kB
build/reusable-blocks/index.min.js 2.55 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/router/index.min.js 1.96 kB
build/server-side-render/index.min.js 1.94 kB
build/shortcode/index.min.js 1.4 kB
build/style-engine/index.min.js 2.04 kB
build/token-list/index.min.js 581 B
build/vendors/react-dom.min.js 41.7 kB
build/vendors/react-jsx-runtime.min.js 560 B
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 965 B
build/warning/index.min.js 250 B
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.03 kB

compressed-size-action

@Mamaduka
Copy link
Member

Mamaduka commented Sep 5, 2024

I was also trying to find the right solution for this issue and came to a similar conclusion. We should probably fully revert #64048 and work on an alternative fix for #63866.

Why revert? Zoom-out mode is still experimental (behind the settings flag); experimental features should hinder stable ones.

@jeryj
Copy link
Contributor Author

jeryj commented Sep 5, 2024

@Mamaduka - I'm fine reverting #64048 as well. I've been looking at this most of the day and feel like there's a deeper issue at play.

The insertion point is bound to the editor, and gets set when the inserter opens. The idea of the inserter staying open and the insertion point changing wasn’t a thing when this was designed, because the inserter was supposed to close and reset when focus was moved out of it. Now that it stays open and you can interact with the canvas, we have this muddling of states where the insertion point can change while the inserter is open.

@jeryj
Copy link
Contributor Author

jeryj commented Sep 6, 2024

I worked more on this today to move the insertion point into the block editor package. It's working correctly for the zoom out inserters. I really dislike the naming of all the things as it's so hard to untangle which insertion point means what :/

@Mamaduka
Copy link
Member

Mamaduka commented Sep 9, 2024

cc @WordPress/gutenberg-core

Comment on lines +729 to +732
* @param {string} value.filterValue A query to filter the inserter results.
* @param {Function} value.onSelect A callback when an item is selected.
* @param {string} value.tab The tab to open in the inserter.
* @param {string} value.category The category to initialize in the inserter.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not new, just adding the documentation for things already in use.

Comment on lines +1425 to +1428
- _value.filterValue_ `string`: A query to filter the inserter results.
- _value.onSelect_ `Function`: A callback when an item is selected.
- _value.tab_ `string`: The tab to open in the inserter.
- _value.category_ `string`: The category to initialize in the inserter.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not new, just adding the documentation for things already in use.

Comment on lines 96 to 101
} else if (
insertionPoint?.insertionIndex &&
insertionPoint?.rootClientId
) {
_destinationRootClientId = insertionPoint.rootClientId;
_destinationIndex = insertionPoint.insertionIndex;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this after the insertionIndex or clientId to preserve the existing API. This lets anyone who passes these values in via the props to still have full control.

Comment on lines 369 to 375
export function setInserterInsertionPoint( value ) {
return {
type: 'SET_INSERTER_INSERTION_POINT',
value,
};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this naming, but it's at least clear 🤷🏻 I'm concerned that this assumes only one instance of the inserter, while the editor package way passes it in. That said, it's still possible to manually pass the inserter the root block and index and it will be respected.

Comment on lines +2102 to +2095
case 'SELECT_BLOCK':
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear the insertion point after a new block selection.

Copy link
Contributor

@scruffian scruffian Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, the vertical displacement is still visible after selecting a different block.

@jeryj jeryj marked this pull request as ready for review September 9, 2024 20:22
Copy link

github-actions bot commented Sep 9, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jeryj <jeryj@git.wordpress.org>
Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: scruffian <scruffian@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: ajlende <ajlende@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@jeryj jeryj self-assigned this Sep 9, 2024
@mtias mtias added the [Type] Performance Related to performance efforts label Sep 9, 2024
@jeryj jeryj changed the title Prevent inserter rerender when hovering blocks Prevent rapid inserter rerenders Sep 9, 2024
@jeryj jeryj added the [Type] Bug An existing feature does not function as intended label Sep 9, 2024
@Mamaduka
Copy link
Member

@jeryj, what do you think about reverting #64048 in a separate PR, while we continue working on a fix here?

@jeryj
Copy link
Contributor Author

jeryj commented Sep 10, 2024

@Mamaduka - yes! Of course. Here's a revert PR: #65208

@jeryj jeryj removed the [Type] Bug An existing feature does not function as intended label Sep 10, 2024
@jeryj jeryj changed the title Prevent rapid inserter rerenders Add inserterInsertionPoint state to enable updating block insertion point on block selection change Sep 10, 2024
Copy link

github-actions bot commented Sep 10, 2024

Flaky tests detected in 98cf472.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11055136565
📝 Reported issues:

@Mamaduka
Copy link
Member

Performance charts after the revert PR was merged. We should monitor "Inserter Hovering Items" results while working on the fix.

Screenshot

CleanShot 2024-09-11 at 12 20 30

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed something weird that I can't reproduce on trunk. I'm not sure if it's related to this PR nor what's going on. The testing environment is Playground.

Kapture.2024-09-11.at.16.29.35.mp4

Notice how sometimes selecting the pattern will insert it at the wrong place. And some other times hover over sections will rerender the patterns inserter.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this one. Some things I noticed.

Comment on lines 93 to 96
_destinationRootClientId = insertionPoint?.rootClientId
? insertionPoint.rootClientId
: rootClientId;
_destinationIndex = insertionPoint.insertionIndex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other conditions have comments to explain them. Could we add one here?

Comment on lines 93 to 97
setInserterIsOpened( {
rootClientId,
insertionIndex,
filterValue,
onSelect,
} );
setInserterInsertionPoint( { rootClientId, insertionIndex } );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have the editor stores setInserterIsOpened dispatch the setInserterInsertionPoint to the block editor store as part of the action. That said it might be clearer this way as the "set inserter is opened" communicates "toggling" rather than "setting an insertion point".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want to setInserterInsertionPoint without setInserterIsOpened as well - for example if you're using the quick inserter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what about backwards compat on the setInserterIsOpened API?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that I think its good to have the flexibility of calling these independently rather than tying one to the other.

Comment on lines 727 to 728
* @param {string} value.rootClientId The root client ID to insert at.
* @param {number} value.insertionIndex The index to insert at.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could continue to make this backwards compatible by dispatching the new action to the block editor store if the rootClientId or the insertionIndex are provided as part of value. You could also fire a deprecated notice in the console.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API has changed. It's a public API. We have to ensure it's backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still pass these things, they just don't do anything within our implementation of the block editor. I'll add a deprecation and fire that though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...they just don't do anything

That was the bit I was concerned about because if a public API used to do something it needs to continue to do that.

Looks like you fixed it up now 👍

Copy link
Member

@Mamaduka Mamaduka Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can’t deprecate a public API/feature and suggest to use a private API instead.

It would be non-actionable for consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would also be fine to not deprecate any of it. You can still pass it those props, it just won't do anything in our codebase 🤷🏻

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh actually we don't need to deprecate it do we? @Mamaduka is right that we can't point to a private API - great spot there.

@Mamaduka
Copy link
Member

I have a couple of surface notes:

  • Opening the inserter and setting insertion points actions are already a bit complicated. I am hesitant about introducing yet another concept in this area. I think @jeryj also noted this somewhere.
  • I would like to add e2e test coverage for regressions linked to the original PR. I'll try to do this in a separate PR and close out those issues.

Screenshot

CleanShot 2024-09-11 at 12 46 42

- old block-editor insertionPoint becomes insertionCue (for the reducer and state value)
- which opens up insertionPoint to be the real insertion point
- so we can change getInserterInsertionPoint to getInsertionPoint
- rename editor private getInsertionPoint to getInserter to more accurately describe its state
- remove unnecessary deprecation
Comment on lines +128 to +140
describe( 'setInsertionPoint', () => {
it( 'should return the SET_INSERTION_POINT action', () => {
expect(
setInsertionPoint( {
rootClientId: '',
index: '123',
} )
).toEqual( {
type: 'SET_INSERTION_POINT',
value: { rootClientId: '', index: '123' },
} );
} );
} );
Copy link
Contributor

@ajlende ajlende Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and all the other tests in this file aren't doing anything useful.

The important thing for actions is that reducers do things based on them and the selectors read out a specific value. The parts of a store shouldn't be tested in isolation like this.

It looks like you're trying to fix the breaking change that may have happened some time earlier, but updating the test and documentation isn't the way to fix it. The prior logic needs to be restored.

If the tests were for the store as a whole, maybe this would have been caught.

Also, to be clear, I'm suggesting moving the tests, documentation, and fixing the setIsInserterOpened regression described in your comment into a different PR.

Comment on lines 744 to 747
return registry.dispatch( blockEditorStore ).setInsertionPoint( {
rootClientId: value.rootClientId,
index: value.insertionIndex,
} );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two bugs here:

  1. It shouldn't return here, it should just dispatch it.
  2. setInsertionPoint isn't an available action because it's in the private-actions in the block editor store. Are we allowed to use it in this editor package if it's private in the block editor package?

Copy link
Contributor

@ajlende ajlende left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. There's more to do in #65290, but this is a good start.

@jeryj jeryj enabled auto-merge (squash) September 26, 2024 17:08
@jeryj jeryj disabled auto-merge September 26, 2024 17:55
@jeryj jeryj enabled auto-merge (squash) September 26, 2024 18:06
@jeryj jeryj merged commit 0ed82ae into trunk Sep 26, 2024
62 of 63 checks passed
@jeryj jeryj deleted the fix/insertion-point-hover branch September 26, 2024 18:37
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 26, 2024
@Mamaduka
Copy link
Member

@jeryj, I appreciate all the work done here, but I think getting broader feedback from the core team would have been nice.

@jeryj
Copy link
Contributor Author

jeryj commented Sep 26, 2024

@Mamaduka - I'd be love to have gotten more feedback on this as well. It had been open long enough that I didn't think I was going to hear anything else, and it was also blocking follow-up work on insertion point work with the zoom out view. Very happy to iterate or follow-up with any of it! Fortunately, I ended up finding a way to not change any public APIs, so everything is possible to iterate or undo.

*
* @return {Object} Action object.
*/
export function setInsertionPoint( value ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than showInsertionPoint?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so If I'm understand properly we have:

  • showInsertionPoint which is the exact location where the block will be inserted. So the true insertion point.
  • and we have setInsertionPoint which is about the top level "inserter" component. IT's about setting the "default" insertion point (if blocks are not allowed to be inserted there, we compute the true insertion point and show it using showInsertionPoint)

is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying my understanding more:

The "inserter insertion point" state was being set in the "editor" package because the "Inserter" component has a "rootClientId" and "clientId" props, so this one is controlled externally, I guess the idea is that at the same time, you can potentially render different inserters with different default insertion points. The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than showInsertionPoint?
showInsertionPoint makes the insertion cue visible. It is not code-wise related to the insertion point, just a visual indicator.

showInsertionPoint which is the exact location where the block will be inserted. So the true insertion point.

No, this only shows where the blue line will get shown (what I will now refer to as the insertion cue for clarity). We don't have a true insertion point at the moment other than passing an insertion point to useInsertionPoint (what the sidebar inserter was doing) or via the new setInsertionPoint added in this PR.

The current PR seems to move the state to "block-editor" but the props remain so it's still "externally" controlled right? So why the move to "block-editor" state.

Exactly. No props were removed and a manually passed insertion point to the inserter will still be respected. We needed to move it to the block-editor state to clear the insertion point when a block is selected.

This reverted PR demos more about why we need this. Code-wise, the inserter was built with a static insertion point in mind. Now the inserter can be open with selection on the block canvas happening, meaning the insertion point can be updated while the inserter is still open.

The problem with the PR above is that it relied on getBlockInsertionPoint which updates constantly on hover, so we had to revert it.

I wrote an overview of the Insertion API current state here: #65290

* @param {Object} state
* @return {Object} where the insertion point in the block editor is or null if none is set.
*/
export function getInsertionPoint( state ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than getBlockInsertionPoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getBlockInsertionPoint is really about the blue insertion cue that gets shown. getBlockInsertionPoint is constantly updated based on where you're hovering the canvas. Maybe originally getBlockInsertionPoint was about where the insertion would happen, but its code now is tied to where the insertion cue shows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference for you between "insertion cue" and "insertion position". If we're showing a different insertion cue than the actual insertion point, then we have problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that one is an index and the other one is an UI element?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are state for me. The UI element is just a reflexion of a state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it gets tricky when you include drag and drop. You’re showing an insertion cue at a location that may be different than what is set via selection.

If you update that point on hover via drag and drop, this is the correct behavior for the insertion cue/insertion point, but a lot of the things built off using it were not intended to be updating their insertion point that often.

So, I think insertion cue of where insertion will happen and should rely on insertion point if set, but the insertion cue should be able to show separate from the insertion point on instances like drag and drop. Hovering the canvas should not be updating the insertion point, it should be updating a cue about where you might be able to insert.

@youknowriad
Copy link
Contributor

One thing I'm not sure I understand is how is this blocking things in zoom-out?

@jeryj
Copy link
Contributor Author

jeryj commented Sep 27, 2024

One thing I'm not sure I understand is how is this blocking things in zoom-out?

We need to be able to update the insertion point with the inserter panel still open. This isn't zoom-out specific, as it's useful for the main canvas as well. We also have work on the zoom out separators that is behaving in a buggy way, and being able to get a clear insertion point will be very helpful.

@youknowriad
Copy link
Contributor

We need to be able to update the insertion point with the inserter panel still open.

Can you expand more, I'm not sure how the current change is related to that. Because the previous getInsertionPoint or the current getInserter can also change their values to adapt to that (the inserter open or closed doesn't really any impact here)

@jeryj
Copy link
Contributor Author

jeryj commented Sep 27, 2024

Can you expand more, I'm not sure how the current change is related to that. Because the previous getInsertionPoint or the current getInserter can also change their values to adapt to that (the inserter open or closed doesn't really any impact here)

You could change the insertion point via the old getInsertionPoint in the editor package, but, AFAIK, would have needed to change it based on the SELECT_BLOCK action in the block-editor package. I am not aware of an acceptable way to update the editor package via an action from the block-editor, and, regardless, it seems like insertion point information seems like data the block-editor should be in control of instead of the editor.

There very well may be better way to handle this, and I'm very open to that. The way I went with here seemed like the best first step towards clarifying and increasing flexibility of the insertion methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Performance Related to performance efforts
Development

Successfully merging this pull request may close these issues.

9 participants