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

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

Merged
merged 1 commit into from Nov 9, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Nov 7, 2018

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.

Show resolved Hide resolved packages/editor/src/components/block-list/block.js Outdated
Show resolved Hide resolved packages/editor/src/components/block-list/block.js Outdated
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( {

This comment has been minimized.

@aduth

aduth Nov 8, 2018

Member

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 8, 2018

Member

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.

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

This comment has been minimized.

@aduth

aduth Nov 8, 2018

Member

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?

This comment has been minimized.

@aduth

aduth Nov 8, 2018

Member

The alternative being to wrap the editable region.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 8, 2018

Member

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

This comment has been minimized.

Member

aduth commented Nov 8, 2018

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

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

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Nov 8, 2018

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

Show resolved Hide resolved packages/editor/src/components/block-list/block.js Outdated
@@ -86,6 +88,12 @@ export class BlockListBlock extends Component {
}
componentDidUpdate( prevProps ) {
if ( this.isForcingContextualToolbar && ! this.props.isTypingWithinBlock ) {

This comment has been minimized.

@aduth

aduth Nov 8, 2018

Member

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2018

Member

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.

@@ -61,6 +61,12 @@ class NavigableToolbar extends Component {
}
}
componentDidMount() {
if ( this.props.focusOnMount ) {

This comment has been minimized.

@aduth

aduth Nov 8, 2018

Member

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2018

Member

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

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

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

This comment has been minimized.

@aduth

aduth Nov 9, 2018

Member

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?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2018

Member

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.

This comment has been minimized.

@aduth

aduth Nov 9, 2018

Member

I like it 👍

@jorgefilipecosta jorgefilipecosta force-pushed the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch from 3c11419 to 2e907e4 Nov 9, 2018

@jorgefilipecosta jorgefilipecosta force-pushed the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch from 2e907e4 to 4b08bb4 Nov 9, 2018

@aduth aduth added this to the 4.3 milestone Nov 9, 2018

@aduth

aduth approved these changes Nov 9, 2018

Nice 👍

@jorgefilipecosta jorgefilipecosta merged commit a2fee33 into master Nov 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/Alt-F10-should-navigate-to-block-toolbar-regardless-of-visibility branch Nov 9, 2018

@mtias

This comment has been minimized.

Contributor

mtias commented Nov 12, 2018

Thanks!

@afercia

This comment has been minimized.

Contributor

afercia commented Nov 12, 2018

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

This comment has been minimized.

Member

aduth commented Nov 13, 2018

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.

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix: Blocks: Alt+F10 should navigate to block toolbar regardless of v…
…isibility (WordPress#11607)

## Description
Closes: WordPress#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment