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: Consistently render IgnoreNestedEvents for block appender #19135

Merged
merged 4 commits into from
Dec 19, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 30 additions & 34 deletions packages/block-editor/src/components/block-list-appender/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,49 +23,45 @@ function BlockListAppender( {
isLocked,
renderAppender: CustomAppender,
} ) {
if ( isLocked ) {
if ( isLocked || CustomAppender === false ) {
return null;
}

// If a render prop has been provided
// use it to render the appender.
let appender;
if ( CustomAppender ) {
return (
<div className="block-list-appender">
<CustomAppender />
</div>
);
}

// a false value means, don't render any appender.
if ( CustomAppender === false ) {
return null;
}

// Render the default block appender when renderAppender has not been
// provided and the context supports use of the default appender.
if ( canInsertDefaultBlock ) {
return (
<div className="block-list-appender">
<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
<DefaultBlockAppender
rootClientId={ rootClientId }
lastBlockClientId={ last( blockClientIds ) }
/>
</IgnoreNestedEvents>
</div>
// Prefer custom render prop if provided.
appender = <CustomAppender />;
} else if ( canInsertDefaultBlock ) {
// Render the default block appender when renderAppender has not been
// provided and the context supports use of the default appender.
appender = (
<DefaultBlockAppender
rootClientId={ rootClientId }
lastBlockClientId={ last( blockClientIds ) }
/>
);
}

// Fallback in the case no renderAppender has been provided and the
// default block can't be inserted.
return (
<div className="block-list-appender">
} else {
// Fallback in the case no renderAppender has been provided and the
// default block can't be inserted.
appender = (
<ButtonBlockAppender
rootClientId={ rootClientId }
className="block-list-appender__toggle"
/>
</div>
);
}

// IgnoreNestedEvents is used to treat interactions within the appender as
// subject to the same conditions as those which occur within nested blocks.
// Notably, this effectively prevents event bubbling to block ancestors
// which can otherwise interfere with the intended behavior of the appender
// (e.g. focus handler on the ancestor block).
return (
<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
<div className="block-list-appender">
{ appender }
</div>
</IgnoreNestedEvents>
);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ function BlockListBlock( {
* (via `setFocus`), typically if there is no focusable input in the block.
*/
const onFocus = () => {
if ( ! isSelected && ! isAncestorOfSelectedBlock && ! isPartOfMultiSelection ) {
if ( ! isSelected && ! isPartOfMultiSelection ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a crucial part to fixing the bug 😅 , but this change has caused the original nav item appender bug to resurface in Firefox (though not in Chrome):
ezgif com-optimize
cc @draganescu

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, Firefox is the issue here and why the whole original attempt was so convoluted :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, reading this and #18379, I might expect this could be related a Firefox+macOS specific treatment of click events in buttons in which the clicks do not cause focus:

https://gist.github.com/cvrebert/68659d0333a578d75372
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a few related handlings of this in Gutenberg elsewhere:

// While ideally it would be enough to capture the
// bubbling focus event from the Inserter, due to the
// characteristics of click focusing of `button`s in
// Firefox and Safari, it is not reliable.
//
// See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
tabIndex={ -1 }

// The appender "button" is in-fact a text field so as to support
// transitions by WritingFlow occurring by arrow key press. WritingFlow
// only supports tab transitions into text fields and to the block focus
// boundary.
//
// See: https://github.com/WordPress/gutenberg/issues/4829#issuecomment-374213658
//
// If it were ever to be made to be a proper `button` element, it is
// important to note that `onFocus` alone would not be sufficient to
// capture click events, notably in Firefox.
//
// See: https://gist.github.com/cvrebert/68659d0333a578d75372

/**
* Handles a mousedown or mouseup event to respectively assign and
* unassign a flag for preventing blur check on button elements. Some
* browsers, namely Firefox and Safari, do not emit a focus event on
* button elements when clicked, while others do. The logic here
* intends to normalize this as treating click on buttons as focus.
*
* @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
*
* @param {MouseEvent} event Event for mousedown or mouseup.
*/
normalizeButtonFocus( event ) {
const { type, target } = event;
const isInteractionEnd = includes( [ 'mouseup', 'touchend' ], type );
if ( isInteractionEnd ) {
this.preventBlurCheck = false;
} else if ( isFocusNormalizedButton( target ) ) {
this.preventBlurCheck = true;
}
}

Similarly, I'm thinking it could be enough to add a tabIndex={ -1 } to the div wrapper, which could allow the focus event to occur when the appender is clicked, while not adding an extra tab stop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly, I'm thinking it could be enough to add a tabIndex={ -1 } to the div wrapper, which could allow the focus event to occur when the appender is clicked, while not adding an extra tab stop.

This seems to work in my testing. See 9091e97, which includes the test and an explanatory inline comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's working fine for me now 🎉

onSelect();
}
};
Expand Down
4 changes: 0 additions & 4 deletions packages/block-editor/src/components/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ const defaultRenderToggle = ( { onToggle, disabled, isOpen, blockTitle, hasSingl
icon="insert"
label={ label }
labelPosition="bottom"
onMouseDown={ ( event ) => {
event.preventDefault();
event.currentTarget.focus();
} }
onClick={ onToggle }
className="block-editor-inserter__toggle"
aria-haspopup={ ! hasSingleBlockType ? 'true' : false }
Expand Down
20 changes: 19 additions & 1 deletion packages/e2e-tests/specs/editor/various/writing-flow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {
insertBlock,
} from '@wordpress/e2e-test-utils';

const getActiveBlockName = async () => page.evaluate( () => wp.data.select( 'core/block-editor' ).getSelectedBlock().name );

describe( 'Writing Flow', () => {
beforeEach( async () => {
await createNewPost();
Expand All @@ -20,7 +22,11 @@ describe( 'Writing Flow', () => {
// not be necessary for interactions, and exist as a stop-gap solution
// where rendering delays in slower CPU can cause intermittent failure.

let activeElementText;
// Assertions are made both against the active DOM element and the
// editor state, in order to protect against potential disparities.
//
// See: https://github.com/WordPress/gutenberg/issues/18928
let activeElementText, activeBlockName;

// Add demo content
await clickBlockAppender();
Expand Down Expand Up @@ -54,13 +60,19 @@ describe( 'Writing Flow', () => {

// Arrow up into nested context focuses last text input
await page.keyboard.press( 'ArrowUp' );
activeBlockName = await getActiveBlockName();
expect( activeBlockName ).toBe( 'core/paragraph' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it wouldn't be best to verify instead that the block toolbar is visible at this point. From a user's perspective, that's what really matters, whereas the block being set as active in our data store is more of an implementation detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if it wouldn't be best to verify instead that the block toolbar is visible at this point. From a user's perspective, that's what really matters, whereas the block being set as active in our data store is more of an implementation detail.

That's a fair expectation we should want in these tests, I agree. The selected block name was more easily accessible and generally representative of what would be expected to be shown in the UI for the toolbar, but I'll see about making the assertion more targeted to the toolbar.

Copy link
Member Author

Choose a reason for hiding this comment

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

In closer inspection, I don't think there's anything of the toolbar markup which can serve as a sufficient indication that the toolbar is showing for a paragraph block:

image

I expect this is in-part due to the fact that core block icons are hard-coded, and not part of some keyed collection against which we can reference:

https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/paragraph/icon.js

The only options I could see here is either adding more markup to the toolbar for the sake of the tests, or comparing against this hard-coded SVG value.

For the purposes of this test, I'm inclined to think we can be content with the current assertion against block editor state. If it proves to fail us in the future, it could be worth revisiting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, we'll have to revisit if activeBlockName at some point gets divorced from toolbar visibility. Might be worth leaving a comment in the test, to make it clear the expectation is that the toolbar is visible at that point.

activeElementText = await page.evaluate( () => document.activeElement.textContent );
expect( activeElementText ).toBe( '2nd col' );

// Arrow up in inner blocks should navigate through (1) column wrapper,
// (2) text fields.
await page.keyboard.press( 'ArrowUp' );
activeBlockName = await getActiveBlockName();
expect( activeBlockName ).toBe( 'core/column' );
await page.keyboard.press( 'ArrowUp' );
activeBlockName = await getActiveBlockName();
expect( activeBlockName ).toBe( 'core/paragraph' );
activeElementText = await page.evaluate( () => document.activeElement.textContent );
expect( activeElementText ).toBe( '1st col' );

Expand All @@ -72,15 +84,21 @@ describe( 'Writing Flow', () => {
document.activeElement.getAttribute( 'data-type' )
) );
expect( activeElementBlockType ).toBe( 'core/column' );
activeBlockName = await getActiveBlockName();
expect( activeBlockName ).toBe( 'core/column' );
await page.keyboard.press( 'ArrowUp' );
activeElementBlockType = await page.evaluate( () => (
document.activeElement.getAttribute( 'data-type' )
) );
expect( activeElementBlockType ).toBe( 'core/columns' );
activeBlockName = await getActiveBlockName();
expect( activeBlockName ).toBe( 'core/columns' );

// Arrow up from focused (columns) block wrapper exits nested context
// to prior text input.
await page.keyboard.press( 'ArrowUp' );
activeBlockName = await getActiveBlockName();
expect( activeBlockName ).toBe( 'core/paragraph' );
activeElementText = await page.evaluate( () => document.activeElement.textContent );
expect( activeElementText ).toBe( 'First paragraph' );

Expand Down