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

Block editor: use React events for shortcuts (portal bubbles & contextual) #32323

Merged
merged 6 commits into from
Jul 22, 2021

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented May 28, 2021

Description

Builds on #32318.

Moves shortcut handlers from native events to React events for two reasons:

  • Native events don't bubble through portals, which requires us to hackily bubble events through the iframe.
  • The shortcuts should only work in the context of the block editor, rather than globally (on the document). This is important because you may have other things on the page beside the block editor, or multiple block editors. Shortcuts should only work if focus is within a certain context. Using React evens works nicely because they bubble both through the iframe and other portals such as the inspector.

Lastly, it removes some boilerplate for implementing a block editor, building <BlockEditorKeyboardShortcuts /> into <BlockTools />.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@github-actions
Copy link

github-actions bot commented May 28, 2021

Size Change: +10 B (0%)

Total Size: 1.07 MB

Filename Size Change
build/block-editor/index.min.js 126 kB -74 B (0%)
build/block-library/index.min.js 147 kB -2 B (0%)
build/components/index.min.js 196 kB +30 B (0%)
build/compose/index.min.js 10.2 kB +3 B (0%)
build/customize-widgets/index.min.js 10.3 kB +10 B (0%)
build/data/index.min.js 7.22 kB +1 B (0%)
build/edit-navigation/index.min.js 13.9 kB +3 B (0%)
build/edit-post/index.min.js 59.5 kB +2 B (0%)
build/edit-site/index.min.js 26 kB -3 B (0%)
build/edit-widgets/index.min.js 16.2 kB -3 B (0%)
build/editor/index.min.js 38.6 kB -12 B (0%)
build/keycodes/index.min.js 1.49 kB +55 B (+4%)
build/nux/index.min.js 2.31 kB -1 B (0%)
build/server-side-render/index.min.js 1.63 kB +1 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 1.11 kB
build/admin-manifest/index.min.js 1.41 kB
build/annotations/index.min.js 2.93 kB
build/api-fetch/index.min.js 2.44 kB
build/autop/index.min.js 2.28 kB
build/blob/index.min.js 673 B
build/block-directory/index.min.js 6.61 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 kB
build/block-editor/style-rtl.css 14 kB
build/block-editor/style.css 14 kB
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 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 112 B
build/block-library/blocks/audio/style.css 112 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 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 475 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 603 B
build/block-library/blocks/button/style.css 602 B
build/block-library/blocks/buttons/editor-rtl.css 315 B
build/block-library/blocks/buttons/editor.css 315 B
build/block-library/blocks/buttons/style-rtl.css 375 B
build/block-library/blocks/buttons/style.css 375 B
build/block-library/blocks/calendar/style-rtl.css 208 B
build/block-library/blocks/calendar/style.css 208 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 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 190 B
build/block-library/blocks/columns/editor.css 190 B
build/block-library/blocks/columns/style-rtl.css 475 B
build/block-library/blocks/columns/style.css 476 B
build/block-library/blocks/cover/editor-rtl.css 670 B
build/block-library/blocks/cover/editor.css 670 B
build/block-library/blocks/cover/style-rtl.css 1.22 kB
build/block-library/blocks/cover/style.css 1.23 kB
build/block-library/blocks/embed/editor-rtl.css 486 B
build/block-library/blocks/embed/editor.css 486 B
build/block-library/blocks/embed/style-rtl.css 401 B
build/block-library/blocks/embed/style.css 400 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 301 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 711 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 704 B
build/block-library/blocks/gallery/editor.css 705 B
build/block-library/blocks/gallery/style-rtl.css 1.06 kB
build/block-library/blocks/gallery/style.css 1.06 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 160 B
build/block-library/blocks/group/editor.css 160 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 93 B
build/block-library/blocks/group/theme.css 93 B
build/block-library/blocks/heading/editor-rtl.css 152 B
build/block-library/blocks/heading/editor.css 152 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 281 B
build/block-library/blocks/html/editor.css 281 B
build/block-library/blocks/image/editor-rtl.css 729 B
build/block-library/blocks/image/editor.css 727 B
build/block-library/blocks/image/style-rtl.css 481 B
build/block-library/blocks/image/style.css 485 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 286 B
build/block-library/blocks/latest-comments/style.css 286 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 526 B
build/block-library/blocks/latest-posts/style.css 524 B
build/block-library/blocks/list/style-rtl.css 63 B
build/block-library/blocks/list/style.css 63 B
build/block-library/blocks/media-text/editor-rtl.css 263 B
build/block-library/blocks/media-text/editor.css 264 B
build/block-library/blocks/media-text/style-rtl.css 492 B
build/block-library/blocks/media-text/style.css 489 B
build/block-library/blocks/more/editor-rtl.css 434 B
build/block-library/blocks/more/editor.css 434 B
build/block-library/blocks/navigation-link/editor-rtl.css 474 B
build/block-library/blocks/navigation-link/editor.css 473 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation/editor-rtl.css 1.69 kB
build/block-library/blocks/navigation/editor.css 1.69 kB
build/block-library/blocks/navigation/style-rtl.css 1.65 kB
build/block-library/blocks/navigation/style.css 1.66 kB
build/block-library/blocks/navigation/view.min.js 2.84 kB
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 310 B
build/block-library/blocks/page-list/editor.css 311 B
build/block-library/blocks/page-list/style-rtl.css 240 B
build/block-library/blocks/page-list/style.css 240 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 247 B
build/block-library/blocks/paragraph/style.css 248 B
build/block-library/blocks/post-author/editor-rtl.css 209 B
build/block-library/blocks/post-author/editor.css 209 B
build/block-library/blocks/post-author/style-rtl.css 183 B
build/block-library/blocks/post-author/style.css 184 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B
build/block-library/blocks/post-comments-form/style.css 140 B
build/block-library/blocks/post-comments/style-rtl.css 360 B
build/block-library/blocks/post-comments/style.css 359 B
build/block-library/blocks/post-content/editor-rtl.css 139 B
build/block-library/blocks/post-content/editor.css 139 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 338 B
build/block-library/blocks/post-featured-image/editor.css 338 B
build/block-library/blocks/post-featured-image/style-rtl.css 141 B
build/block-library/blocks/post-featured-image/style.css 141 B
build/block-library/blocks/post-template/editor-rtl.css 100 B
build/block-library/blocks/post-template/editor.css 99 B
build/block-library/blocks/post-template/style-rtl.css 379 B
build/block-library/blocks/post-template/style.css 380 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 60 B
build/block-library/blocks/post-title/style.css 60 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 362 B
build/block-library/blocks/pullquote/style.css 361 B
build/block-library/blocks/pullquote/theme-rtl.css 169 B
build/block-library/blocks/pullquote/theme.css 169 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 270 B
build/block-library/blocks/query-pagination/editor.css 262 B
build/block-library/blocks/query-pagination/style-rtl.css 168 B
build/block-library/blocks/query-pagination/style.css 168 B
build/block-library/blocks/query-title/editor-rtl.css 86 B
build/block-library/blocks/query-title/editor.css 86 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 169 B
build/block-library/blocks/quote/style.css 169 B
build/block-library/blocks/quote/theme-rtl.css 221 B
build/block-library/blocks/quote/theme.css 221 B
build/block-library/blocks/rss/editor-rtl.css 201 B
build/block-library/blocks/rss/editor.css 202 B
build/block-library/blocks/rss/style-rtl.css 290 B
build/block-library/blocks/rss/style.css 290 B
build/block-library/blocks/search/editor-rtl.css 211 B
build/block-library/blocks/search/editor.css 211 B
build/block-library/blocks/search/style-rtl.css 359 B
build/block-library/blocks/search/style.css 362 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 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 251 B
build/block-library/blocks/separator/style.css 251 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 476 B
build/block-library/blocks/shortcode/editor.css 476 B
build/block-library/blocks/site-logo/editor-rtl.css 465 B
build/block-library/blocks/site-logo/editor.css 465 B
build/block-library/blocks/site-logo/style-rtl.css 154 B
build/block-library/blocks/site-logo/style.css 154 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-title/editor-rtl.css 85 B
build/block-library/blocks/site-title/editor.css 85 B
build/block-library/blocks/social-link/editor-rtl.css 164 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 800 B
build/block-library/blocks/social-links/editor.css 799 B
build/block-library/blocks/social-links/style-rtl.css 1.34 kB
build/block-library/blocks/social-links/style.css 1.34 kB
build/block-library/blocks/spacer/editor-rtl.css 308 B
build/block-library/blocks/spacer/editor.css 308 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 478 B
build/block-library/blocks/table/editor.css 478 B
build/block-library/blocks/table/style-rtl.css 480 B
build/block-library/blocks/table/style.css 480 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 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 551 B
build/block-library/blocks/template-part/editor.css 550 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/term-description/editor-rtl.css 90 B
build/block-library/blocks/term-description/editor.css 90 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 569 B
build/block-library/blocks/video/editor.css 570 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 1.29 kB
build/block-library/common.css 1.29 kB
build/block-library/editor-rtl.css 9.83 kB
build/block-library/editor.css 9.83 kB
build/block-library/reset-rtl.css 514 B
build/block-library/reset.css 515 B
build/block-library/style-rtl.css 10.3 kB
build/block-library/style.css 10.3 kB
build/block-library/theme-rtl.css 692 B
build/block-library/theme.css 693 B
build/block-serialization-default-parser/index.min.js 1.3 kB
build/block-serialization-spec-parser/index.min.js 3.06 kB
build/blocks/index.min.js 47.2 kB
build/components/style-rtl.css 15.9 kB
build/components/style.css 15.9 kB
build/core-data/index.min.js 12.4 kB
build/customize-widgets/style-rtl.css 1.48 kB
build/customize-widgets/style.css 1.48 kB
build/data-controls/index.min.js 830 B
build/date/index.min.js 31.8 kB
build/deprecated/index.min.js 737 B
build/dom-ready/index.min.js 576 B
build/dom/index.min.js 4.78 kB
build/edit-navigation/style-rtl.css 3.12 kB
build/edit-navigation/style.css 3.12 kB
build/edit-post/classic-rtl.css 483 B
build/edit-post/classic.css 483 B
build/edit-post/style-rtl.css 7.25 kB
build/edit-post/style.css 7.24 kB
build/edit-site/style-rtl.css 5.04 kB
build/edit-site/style.css 5.03 kB
build/edit-widgets/style-rtl.css 3.99 kB
build/edit-widgets/style.css 3.99 kB
build/editor/style-rtl.css 3.88 kB
build/editor/style.css 3.88 kB
build/element/index.min.js 3.44 kB
build/escape-html/index.min.js 739 B
build/format-library/index.min.js 5.71 kB
build/format-library/style-rtl.css 668 B
build/format-library/style.css 669 B
build/hooks/index.min.js 1.76 kB
build/html-entities/index.min.js 628 B
build/i18n/index.min.js 3.73 kB
build/is-shallow-equal/index.min.js 710 B
build/keyboard-shortcuts/index.min.js 1.74 kB
build/list-reusable-blocks/index.min.js 2.07 kB
build/list-reusable-blocks/style-rtl.css 842 B
build/list-reusable-blocks/style.css 842 B
build/media-utils/index.min.js 3.08 kB
build/notices/index.min.js 1.07 kB
build/nux/style-rtl.css 745 B
build/nux/style.css 742 B
build/plugins/index.min.js 1.99 kB
build/primitives/index.min.js 1.06 kB
build/priority-queue/index.min.js 790 B
build/react-i18n/index.min.js 924 B
build/redux-routine/index.min.js 2.82 kB
build/reusable-blocks/index.min.js 2.56 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.8 kB
build/shortcode/index.min.js 1.68 kB
build/token-list/index.min.js 847 B
build/url/index.min.js 1.95 kB
build/viewport/index.min.js 1.28 kB
build/warning/index.min.js 1.16 kB
build/widgets/index.min.js 6.48 kB
build/widgets/style-rtl.css 1.04 kB
build/widgets/style.css 1.05 kB
build/wordcount/index.min.js 1.24 kB

compressed-size-action

Base automatically changed from restore/select-all-ref-callback to trunk May 28, 2021 16:41
@ellatrix ellatrix force-pushed the move/block-editor-shortcuts-synthetic-events branch from 15ac136 to 4c1f6c5 Compare May 28, 2021 16:54
draganescu
draganescu previously approved these changes Jun 2, 2021
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I tested this and it works in an identical way for the user. I think the reasons for the change in the PR's description are very good.

@draganescu draganescu dismissed their stale review June 2, 2021 11:31

wrong testing branch!

@draganescu
Copy link
Contributor

Unfortunately in Safari the shortcuts are picked up by the browser even in editor context (e.g. cmd + D opens the bookmark dialog, cmd option shift t opens the profiling in Inspector).

@ellatrix ellatrix force-pushed the move/block-editor-shortcuts-synthetic-events branch 2 times, most recently from 8f3c243 to ecdea3f Compare July 16, 2021 14:33
@ellatrix ellatrix added the [Type] Code Quality Issues or PRs that relate to code quality label Jul 16, 2021
@ellatrix ellatrix force-pushed the move/block-editor-shortcuts-synthetic-events branch from 652df47 to 7636e79 Compare July 22, 2021 11:20
@ellatrix ellatrix force-pushed the move/block-editor-shortcuts-synthetic-events branch from 7636e79 to 2d6754a Compare July 22, 2021 11:22
@ellatrix ellatrix merged commit 48df97f into trunk Jul 22, 2021
@ellatrix ellatrix deleted the move/block-editor-shortcuts-synthetic-events branch July 22, 2021 16:14
@github-actions github-actions bot added this to the Gutenberg 11.2 milestone Jul 22, 2021
@youknowriad
Copy link
Contributor

This is a big departure in terms of APIs for the shortcuts package, I think it'd have deserved more eyes a bit.

For instance

The shortcuts should only work in the context of the block editor, rather than globally (on the document).

I don't think that makes sense for all shortcuts, some of the shortcuts are edit-post shortcuts and should work no matter where we are (like saving for instance).

I also kind of like the useShortcut API personally, another option could have been to return props to add to a react element if keeping the React Event approach is mandatory.

@@ -192,66 +192,65 @@ export default function VisualEditor( { styles } ) {
}, [ isTemplateMode, themeSupportsLayout, contentSize, wideSize ] );

return (
<div
<BlockTools
Copy link
Contributor

Choose a reason for hiding this comment

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

For me if this needs to be a parent of the block editor, it needs to be documented properly in the block-editor package and the documentation we have to build third-party block editors.

That said, the naming is not great at all, shouldn't be BlockEditorWrapper or something like that? Also, if it's a wrapper, why not just bundle it in BlockEditorProvider

Copy link
Member Author

Choose a reason for hiding this comment

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

Why does it need to be higher up the tree? Agreed on the documentation, I should add some.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the question. My main motivation here is that it seems we're requiring two wrappers where we can require just one. Unless I'm not understanding properly both these components should wrap the editor canvas? Maybe more granularity is needed, but it's not clear from the user's perspective where to put each one of these components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants