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

Update the block overlay to rely on useDisabled hook #40649

Merged
merged 5 commits into from
May 6, 2022

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Apr 27, 2022

closes #40845
Related #32163 (comment)

What?

This PR kind of introduces a "disabled" state/config for a block wrapper. when it's disabled it uses the useDisabled hook to prevent editing or selecting anything inside it. Right now it's being used to replace the "Block Overlay" feature. (Reusable blocks, navigation blocks, template parts).

Why?

We plan to expand the "disabling" behavior to a lot more use-cases:

  • exploded mode
  • locked blocks

and this can be seen as the first step towards that.

How?

This is composed of two changes:

  • Update the useDisabled hook to allow "disabling" it and "enabling" it again based on a config.
  • Embeds useDisabled in useBlockProps and allow passing a isDisabled config to trigger it

Testing Instructions

1- Try navigation, reusable and template part blocks
2- See how the selection behaves (overlay..)

focusable.setAttribute( 'disabled', '' );
}
export default function useDisabled( {
isDisabled: isDisabledProp = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flag disables useDisabled. it's weird I know, I don't know if there's a better way to name this.

@github-actions
Copy link

github-actions bot commented Apr 27, 2022

Size Change: -567 B (0%)

Total Size: 1.23 MB

Filename Size Change
build/block-editor/index.min.js 150 kB -179 B (0%)
build/block-editor/style-rtl.css 15 kB -33 B (0%)
build/block-editor/style.css 15 kB -31 B (0%)
build/block-library/index.min.js 177 kB -649 B (0%)
build/components/index.min.js 227 kB +83 B (0%)
build/compose/index.min.js 11.4 kB +174 B (+2%)
build/core-data/index.min.js 14.6 kB +63 B (0%)
build/edit-navigation/index.min.js 15.8 kB -6 B (0%)
build/edit-site/index.min.js 47.4 kB +16 B (0%)
build/widgets/index.min.js 7.21 kB -5 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/annotations/index.min.js 2.77 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 487 B
build/block-directory/index.min.js 6.51 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
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 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 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 59 B
build/block-library/blocks/avatar/style.css 59 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 445 B
build/block-library/blocks/button/editor.css 445 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 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 79 B
build/block-library/blocks/categories/style.css 79 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 127 B
build/block-library/blocks/comment-template/style.css 127 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 95 B
build/block-library/blocks/comments/editor.css 95 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.53 kB
build/block-library/blocks/cover/style.css 1.53 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 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 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 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 353 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 961 B
build/block-library/blocks/gallery/editor.css 964 B
build/block-library/blocks/gallery/style-rtl.css 1.51 kB
build/block-library/blocks/gallery/style.css 1.51 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 333 B
build/block-library/blocks/group/editor.css 333 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 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 529 B
build/block-library/blocks/image/style.css 535 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 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 199 B
build/block-library/blocks/latest-posts/editor.css 198 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 493 B
build/block-library/blocks/media-text/style.css 490 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 708 B
build/block-library/blocks/navigation-link/editor.css 706 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 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 375 B
build/block-library/blocks/navigation/editor-rtl.css 2.03 kB
build/block-library/blocks/navigation/editor.css 2.04 kB
build/block-library/blocks/navigation/style-rtl.css 1.95 kB
build/block-library/blocks/navigation/style.css 1.94 kB
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 395 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/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 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 69 B
build/block-library/blocks/post-comments-form/editor.css 69 B
build/block-library/blocks/post-comments-form/style-rtl.css 521 B
build/block-library/blocks/post-comments-form/style.css 521 B
build/block-library/blocks/post-comments/editor-rtl.css 77 B
build/block-library/blocks/post-comments/editor.css 77 B
build/block-library/blocks/post-comments/style-rtl.css 527 B
build/block-library/blocks/post-comments/style.css 527 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 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/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 323 B
build/block-library/blocks/post-template/style.css 323 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 80 B
build/block-library/blocks/post-title/style.css 80 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 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 370 B
build/block-library/blocks/pullquote/style.css 370 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 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 369 B
build/block-library/blocks/query/editor.css 369 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 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 140 B
build/block-library/blocks/separator/editor.css 140 B
build/block-library/blocks/separator/style-rtl.css 233 B
build/block-library/blocks/separator/style.css 233 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 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 759 B
build/block-library/blocks/site-logo/editor.css 759 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 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 177 B
build/block-library/blocks/social-link/editor.css 177 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.37 kB
build/block-library/blocks/social-links/style.css 1.36 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 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 504 B
build/block-library/blocks/table/editor.css 504 B
build/block-library/blocks/table/style-rtl.css 625 B
build/block-library/blocks/table/style.css 625 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 226 B
build/block-library/blocks/tag-cloud/style.css 227 B
build/block-library/blocks/template-part/editor-rtl.css 149 B
build/block-library/blocks/template-part/editor.css 149 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 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 993 B
build/block-library/common.css 990 B
build/block-library/editor-rtl.css 10.2 kB
build/block-library/editor.css 10.3 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 11.5 kB
build/block-library/style.css 11.5 kB
build/block-library/theme-rtl.css 689 B
build/block-library/theme.css 694 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 47 kB
build/components/style-rtl.css 15 kB
build/components/style.css 15 kB
build/customize-widgets/index.min.js 11 kB
build/customize-widgets/style-rtl.css 1.39 kB
build/customize-widgets/style.css 1.39 kB
build/data-controls/index.min.js 663 B
build/data/index.min.js 7.66 kB
build/date/index.min.js 32 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.58 kB
build/edit-navigation/style-rtl.css 4.05 kB
build/edit-navigation/style.css 4.05 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 30.1 kB
build/edit-post/style-rtl.css 7.02 kB
build/edit-post/style.css 7.02 kB
build/edit-site/style-rtl.css 7.95 kB
build/edit-site/style.css 7.93 kB
build/edit-widgets/index.min.js 16.3 kB
build/edit-widgets/style-rtl.css 4.41 kB
build/edit-widgets/style.css 4.4 kB
build/editor/index.min.js 38.4 kB
build/editor/style-rtl.css 3.67 kB
build/editor/style.css 3.67 kB
build/element/index.min.js 4.3 kB
build/escape-html/index.min.js 548 B
build/format-library/index.min.js 6.62 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.66 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.79 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.83 kB
build/keycodes/index.min.js 1.41 kB
build/list-reusable-blocks/index.min.js 1.75 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.94 kB
build/notices/index.min.js 957 B
build/nux/index.min.js 2.1 kB
build/nux/style-rtl.css 751 B
build/nux/style.css 749 B
build/plugins/index.min.js 1.98 kB
build/preferences-persistence/index.min.js 2.16 kB
build/preferences/index.min.js 1.32 kB
build/primitives/index.min.js 949 B
build/priority-queue/index.min.js 628 B
build/react-i18n/index.min.js 704 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.69 kB
build/reusable-blocks/index.min.js 2.24 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11.2 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.52 kB
build/token-list/index.min.js 668 B
build/url/index.min.js 1.99 kB
build/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 280 B
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.07 kB

compressed-size-action

@@ -54,10 +57,14 @@ const BLOCK_ANIMATION_THRESHOLD = 200;
* the ref if one is defined.
* @param {Object} options Options for internal use only.
* @param {boolean} options.__unstableIsHtml
* @param {boolean} options.isDisabled Whether the block should be disabled.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another approach here would be a options.hasOverlay flag and bundle the call to useBlockOverlay here. I might switch to that as it avoids a lot of duplication.

@jameskoster
Copy link
Contributor

In terms of the UI/UX this PR seems to behave identically to trunk. Nothing is disabled, but I cannot directly click to select children of template parts, reusable blocks, or navigations.

@youknowriad
Copy link
Contributor Author

I was wondering if it inadvertently fixes the browser issue we had in Safari where sometimes the inner block get selected. The disabling is implemented a bit differently (two levels of disabling) so it might help.

@jameskoster
Copy link
Contributor

It's a bit better, I can't directly select Link blocks (probably because they're inside the disabled Navigation), but the Site Title remains interactive:

safari.mp4

@Mamaduka
Copy link
Member

Last night, I started testing this, and the overlay works better (compared to trunk) when there're RichText inner blocks.

PR currently breaks Reusable block edit locking. Maybe I should remove it from the trunk and start working to implement it for all blocks.

@talldan
Copy link
Contributor

talldan commented Apr 28, 2022

@Mamaduka and I did chat about this kind of approach a week or so ago, and a concern would be accessibility. Primarily keyboard navigation, but also because it changes the semantics of those elements.

In testing, keyboard navigation seems not too bad. Using the down arrow works ok, I guess because as the outer block is focused, the inner blocks become enabled and can be navigated to. Using the up arrow now possibly works differently - it'll focus the outer block because the inner blocks can't be navigated to.

I'm less sure whether the semantics would be an issue. Would need more experienced a11y feedback for that.

On another matter, there does seem to be some weird glitching happening in the PR with blocks continually unmounting/remounting. I noticed this in the dev tools:

Kapture.2022-04-28.at.13.55.57.mp4

and also in Safari when you hover over the block toolbar items, the cursor continually changes from a pointer to a hand.

And another issue is that navigation links have a slightly greyed-out style when disabled.

@youknowriad
Copy link
Contributor Author

On another matter, there does seem to be some weird glitching happening in the PR with blocks continually unmounting/remounting

I think that's not really unmouting/remounting but it's probably something to do with the "mutation observer" in useDisabled triggering too often. I'll have to check what's causing it (probably a loop in useDisabled).

@youknowriad
Copy link
Contributor Author

I fixed the loop issue, and I think it may have fixed the toolbar issue as well.

@youknowriad
Copy link
Contributor Author

youknowriad commented Apr 28, 2022

@Mamaduka I fixed the reusable block locking, it's actually a start for its support in all blocks. Makes me wonder that an e2e test to check that the locking is working would be good.

@Mamaduka
Copy link
Member

Mamaduka commented May 5, 2022

Thank you, @youknowriad. I started retesting this branch, and things look great.

Can you rebase on top of the current trunk? A few selection fixes were merged, and I just want to test against them as well.

I think this also fixed #35079.

CleanShot.2022-05-05.at.10.39.05.mp4

*/
import classnames from 'classnames';

export default function BlockContentOverlay( {
Copy link
Member

@Mamaduka Mamaduka May 5, 2022

Choose a reason for hiding this comment

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

Should we deprecate this? Directory search return single plugin - https://wpdirectory.net/search/01G29EAZQER3R2XCAGSMB7MRD5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was never stable in the first place. So I think it's fine to just remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(The result in wp-directory is a plugin that show up in all Gutenberg API searches :P I think it's bundling our packages)

Copy link
Member

@Mamaduka Mamaduka 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 the rebase, @youknowriad 🙇

It is an excellent improvement for the block content overlay and can be merged.

I agree with @talldan that the overlay method would need more experienced a11y feedback if we want to use it for edit locking. At least to make sure content is accessible for screen readers.

P.S. I noticed a minor issue with a Reusable block. When editing is locked, the insertion point is visible sometimes, and you can insert blocks.

Screenshot

CleanShot 2022-05-05 at 16 47 55

Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

In terms of design this seems like an improvement over trunk since the Navigation block is behaving as expected.

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented May 5, 2022

Thanks for working on improving this! I see two behavior issues that we were originally adamant about solving with the first overlay:

  • The popup block inserters still appear through the disabled/overlay block, allowing users to bypass this feature.
  • Instead of being able to drop anywhere in the container it forces the drop at the top level of the disabled container. Another drag-and-drop can be used after that to move within the container. So it does still work to some extent but creates a bit of a fragmented drag-and-drop flow into the container.. Whoops, apparently im bad at testing this seems to work as expected and I dont see any regression here.

Up to you whether these are blockers to fix first or not. But this is working well in my testing beyond that!

@talldan
Copy link
Contributor

talldan commented May 6, 2022

I think there's a regression here that I briefly mentioned earlier:

Using the up arrow now possibly works differently - it'll focus the outer block because the inner blocks can't be navigated to.

I'll add some more specific testing instructions, as originally this wasn't very clear.

  1. In the post editor, add three paragraph blocks with text
  2. Make the first two paragraphs part of a reusable block.
  3. Place the caret to the last paragraph
  4. Press the up arrow

Expected (In trunk): The paragraph block is selected and focused and the caret is visible in the paragraph block, pressing up again moves focus to the first paragraph block
Actual (In this PR): the reusable block is selected and focused and there's no visible caret. Pressing up again moves focus to the title.

@youknowriad
Copy link
Contributor Author

I'll add some more specific testing instructions, as originally this wasn't very clear.

Thinking about this, I actually feel the behavior in this branch is probably the one that is more correct. When navigating to a reusable block, it's arguable whether you want to land directly in the inner paragraph. In fact, it's for this exact same reason we have the overlay on click on the reusable block. that arrow navigation behavior is just the reflexion of that overlay for keyboard navigation.

@youknowriad
Copy link
Contributor Author

The popup block inserters still appear through the disabled/overlay block, allowing users to bypass this feature.

This should be fixed in 5d7c549

@youknowriad youknowriad merged commit a50c717 into trunk May 6, 2022
@youknowriad youknowriad deleted the update/block-overlay branch May 6, 2022 11:45
@github-actions github-actions bot added this to the Gutenberg 13.3 milestone May 6, 2022
@mburridge mburridge added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 9, 2022
@mburridge
Copy link
Contributor

Added label in case a dev note is needed for 6.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Dev Note Requires a developer note for a major WordPress release cycle [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useDisabled hook causes infinite loop
6 participants