Fix: Blocks: Alt+F10 should navigate to block toolbar regardless of visibility #11607

jorgefilipecosta · 2018-11-07T22:44:18Z

Description

Closes: #10907
When pressing alt + f10 if the contextual toolbar was not visible because the user was typing, we did not focus the block contextual toolbar, and we focused the header instead.

This PR addresses this problem.
To solve the problem, the PR performs the following actions:
Adds a prop to NavigableToolbar that enables it to focus when mounting.
Adds an event handler for pressing alt+f10 key combination on BlockListBlock and state flag the indicates that the handling is in progress.

How has this been tested?

I checked the unified toolbar mode was not enabled.
I added a paragraph, I wrote something until the toolbar disappears I pressed alt+f10, and I verified the block toolbar appeared with the first item focused.
I repeated the test with other blocks, e.g., writing on the image caption.

packages/editor/src/components/block-list/block.js

aduth · 2018-11-08T16:43:04Z

packages/editor/src/components/block-list/block.js

+		if ( this.state.isHandlingAltF10 && ! this.props.isTypingWithinBlock ) {
+			// When the component updates and the user is not typing within the block
+			// we know that handling alt+f10 keypress is finished (the necessary child blocks are rendered).
+			this.setState( {


Would it be easy enough to use an instance variable this.isForcingContextualToolbar? This would avoid the need for a second re-render.

We can use an instance variable the catch is that we need to force a re-render when changing the variable to true. I updated to use this approach.

aduth · 2018-11-08T16:45:05Z

packages/editor/src/components/block-list/block.js

+								/>
+							) }
+							{ ! shouldShowContextualToolbar && contextualSidebarMayAppear && isSelected && (
+								<KeyboardShortcuts


An impact of how we render this is that pressing Alt+F10 while in the block inspector will cause focus to be moved into the contextual toolbar. Is that expected?

The alternative being to wrap the editable region.

An impact of how we render this is that pressing Alt+F10 while in the block inspector will cause focus to be moved into the contextual toolbar. Is that expected?

That is already the case in master. When we are on the block inspector the toolbar will always be shown normally without the forcing as we are not typing so in this PR we are not changing any behavior for that case. I personally find this behavior acceptable as the block inspector is part of the block.

aduth · 2018-11-08T16:46:04Z

Probably conflicts with #10699, though addressing different (similar) issues.

jorgefilipecosta · 2018-11-08T20:34:52Z

Hi @aduth thank you for your review. I think I addressed your feedback.

packages/editor/src/components/block-list/block.js

aduth · 2018-11-08T21:10:59Z

packages/editor/src/components/block-list/block.js

@@ -86,6 +88,12 @@ export class BlockListBlock extends Component {
 	}

 	componentDidUpdate( prevProps ) {
+		if ( this.isForcingContextualToolbar && ! this.props.isTypingWithinBlock ) {


Do we even need this condition? Or can it be assumed that since the render will occur following the assignment of the instance property that it's safe to immediately unset it in componentDidUpdate?

What is it about moving focus to the toolbar that causes this.props.isTypingWithinBlock to become false? It doesn't seem like something which could be strictly depended upon.

Do we even need this condition?

We don't need this condition and the code was updated. It is safe to immediately unset it.

What is it about moving focus to the toolbar that causes this.props.isTypingWithinBlock to become false?

The expectation I had was that if the RichText loses focus, a stop typing action is dispatched.

aduth · 2018-11-08T21:13:57Z

packages/editor/src/components/navigable-toolbar/index.js

@@ -61,6 +61,12 @@ class NavigableToolbar extends Component {
 		}
 	}

+	componentDidMount() {
+		if ( this.props.focusOnMount ) {


If it's part of the public interface, we should include a note in the CHANGELOG.md file.

Ideally also in README.md for the component, but it doesn't exist (until #10699).

I added a note to the CHANGELOG.md.

Regarding the README.md given that it is already being added in #10699 my plan is to add a commit in #10699 after merging this PR updating its description to include the new property (provided it is not merged before if that is the case I will add the commit here).

aduth · 2018-11-09T16:57:43Z

packages/editor/src/components/block-list/block.js

@@ -403,7 +416,8 @@ export class BlockListBlock extends Component {
 					// We render block movers and block settings to keep them tabbale even if hidden
 					const shouldRenderMovers = ! isFocusMode && ( isSelected || hoverArea === 'left' ) && ! showEmptyBlockSideInserter && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock;
 					const shouldShowBreadcrumb = ! isFocusMode && isHovered && ! isEmptyDefaultBlock;
-					const shouldShowContextualToolbar = ! hasFixedToolbar && ! showSideInserter && ( ( isSelected && ( ! isTypingWithinBlock || isCaretWithinFormattedText ) ) || isFirstMultiSelected );
+					const contextualToolbarMayAppear = ! hasFixedToolbar && ! showSideInserter;


I'm wondering if this really ought to be a condition including showSideInserter, or if it's enough to render KeyboardShortcuts with direct consideration of ! hasFixedToolbar. Thoughts?

Hi @aduth,
I added a new condition, that specifies in which cases we should render KeyboardShortcuts more explicitly.
We should take into account the isEmptyDefaultBlock flag, if true we should not use the shortcut as in this case the block has no toolbar, and we should continue to focus the header toolbar.
Let me know you prefer this condition; it should be equivalent to the previous one, just in a different format.

I like it 👍

…isibility

aduth

Nice 👍

mtias · 2018-11-12T14:39:22Z

Thanks!

afercia · 2018-11-12T18:37:46Z

Seems to me there's now a warning:

Warning: React does not recognize the `focusOnMount` prop on a DOM element. ...
    in div (created by NavigableContainer)
    ...

/Cc @jorgefilipecosta

aduth · 2018-11-13T16:12:14Z

Seems to me there's now a warning:

Warning: React does not recognize the `focusOnMount` prop on a DOM element. ...
    in div (created by NavigableContainer)
    ...

/Cc @jorgefilipecosta

Issue at #11769. Pull request at #11804.

aduth reviewed Nov 8, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch 2 times, most recently from 31f3ff0 to c095313 Compare November 8, 2018 19:42

mtias mentioned this pull request Nov 8, 2018

Steps to make keyboard navigation usable #11581

Closed

aduth reviewed Nov 8, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch 3 times, most recently from 018d019 to 3c11419 Compare November 9, 2018 11:47

aduth reviewed Nov 9, 2018

View reviewed changes

jorgefilipecosta force-pushed the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch from 3c11419 to 2e907e4 Compare November 9, 2018 17:20

Fix: Blocks: Alt+F10 should navigate to block toolbar regardless of v…

4b08bb4

…isibility

jorgefilipecosta force-pushed the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch from 2e907e4 to 4b08bb4 Compare November 9, 2018 17:24

aduth added this to the 4.3 milestone Nov 9, 2018

aduth approved these changes Nov 9, 2018

View reviewed changes

jorgefilipecosta merged commit a2fee33 into master Nov 9, 2018

jorgefilipecosta deleted the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch November 9, 2018 17:57

aduth mentioned this pull request Nov 19, 2018

Try to make the inline toolbars navigable toolbars and make Alt+F10 work #6302

Closed

aduth mentioned this pull request Mar 8, 2019

Editor: Add KeybindScope complement to NavigableToolbar #10699

Closed

This was referenced Dec 29, 2020

[Snyk] Fix for 1 vulnerabilities wpscholar/gutenberg#15

Open

[Snyk] Security upgrade webpack from 4.44.2 to 5.0.0 wpscholar/gutenberg#16

Open

This was referenced Mar 15, 2021

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 wpscholar/gutenberg#20

Open

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 wpscholar/gutenberg#21

Open

This was referenced Nov 5, 2022

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 Verdinjoshua26/gutenberg#56

Open

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#57

Open

This was referenced Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#59

Open

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 Verdinjoshua26/gutenberg#60

Open

This was referenced Jan 9, 2023

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 Verdinjoshua26/gutenberg#62

Open

[Snyk] Security upgrade webpack from 4.46.0 to 5.0.0 Verdinjoshua26/gutenberg#63

Open

This was referenced Nov 27, 2023

[Snyk] Fix for 5 vulnerabilities Verdinjoshua26/gutenberg#93

Open

[Snyk] Security upgrade webpack from 4.47.0 to 5.0.0 Verdinjoshua26/gutenberg#94

Open

This was referenced May 13, 2024

[Snyk] Security upgrade webpack from 4.47.0 to 5.0.0 Verdinjoshua26/gutenberg#119

Open

[Snyk] Fix for 2 vulnerabilities Verdinjoshua26/gutenberg#121

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Blocks: Alt+F10 should navigate to block toolbar regardless of visibility #11607

Fix: Blocks: Alt+F10 should navigate to block toolbar regardless of visibility #11607

jorgefilipecosta commented Nov 7, 2018 •

edited

Loading

aduth Nov 8, 2018

jorgefilipecosta Nov 8, 2018

aduth Nov 8, 2018

aduth Nov 8, 2018

jorgefilipecosta Nov 8, 2018

aduth commented Nov 8, 2018

jorgefilipecosta commented Nov 8, 2018

aduth Nov 8, 2018

jorgefilipecosta Nov 9, 2018 •

edited

Loading

aduth Nov 8, 2018

jorgefilipecosta Nov 9, 2018

aduth Nov 9, 2018

jorgefilipecosta Nov 9, 2018

aduth Nov 9, 2018

aduth left a comment

mtias commented Nov 12, 2018

afercia commented Nov 12, 2018

aduth commented Nov 13, 2018

Fix: Blocks: Alt+F10 should navigate to block toolbar regardless of visibility #11607

Fix: Blocks: Alt+F10 should navigate to block toolbar regardless of visibility #11607

Conversation

jorgefilipecosta commented Nov 7, 2018 • edited Loading

Description

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Nov 8, 2018

jorgefilipecosta commented Nov 8, 2018

Choose a reason for hiding this comment

jorgefilipecosta Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

mtias commented Nov 12, 2018

afercia commented Nov 12, 2018

aduth commented Nov 13, 2018

jorgefilipecosta commented Nov 7, 2018 •

edited

Loading

jorgefilipecosta Nov 9, 2018 •

edited

Loading