Block Editor: Consistently render IgnoreNestedEvents for block appender by aduth · Pull Request #19135 · WordPress/gutenberg

aduth · 2019-12-13T20:30:15Z

This pull request seeks to consistently apply the behavior of IgnoreNestedEvents for the block appender component. Previously, the wrapper would have applied to the rendering of the default block appender, and was intended to prevent conflicted interactions between the appender and an ancestor block's focus handlers. Since this was only applied to the default appender and not custom or button appenders, those same conflicts could occur in the latter cases (see #18329). With these changes, IgnoreNestedEvents is now used to wrap each possible rendering of the block appender. This also serves as a simplification of the component to unify the render logic.

This also updates the associated writing-flow.test.js end-to-end test. Previously, there were assertions which were made to try to verify the intended behavior. However, those assertions relied on the DOM as the source of truth. For the scenario described in #18928, the conflicting focus behavior was such that the active DOM element would be the correct node (the column), but the UI would reflect the incorrect block toolbar (the paragraph), because the selected block in block editor state was prevented from being updated. As of these changes, assertions are made against both the DOM and block editor state to verify correctness.

Testing Instructions:

Repeat steps to reproduce from #18928, verifying that you can ArrowUp from the first block within a column to navigate to the parent Column block.

As described in #18379 (comment), verify that in a Cover block, you can click the cover block background to select the parent block.

Verify that the end-to-end test included as part of #18379 still passes:

npm run test-e2e packages/e2e-tests/specs/editor/blocks/navigation.test.js

Verify that the enhanced end-to-end test for writing flow still passes:

npm run test-e2e packages/e2e-tests/specs/editor/various/writing-flow.test.js

ZebulanStanphill · 2019-12-13T21:30:26Z

I tried out the branch and everything seems to be working! 😄👍

tellthemachines · 2019-12-16T05:23:27Z

 	 */
 	const onFocus = () => {
-		if ( ! isSelected && ! isAncestorOfSelectedBlock && ! isPartOfMultiSelection ) {
+		if ( ! isSelected && ! isPartOfMultiSelection ) {


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

cc @draganescu

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

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

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

gutenberg/packages/block-editor/src/components/block-list/insertion-point.js

Lines 54 to 60 in 74269e6

// 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 }

gutenberg/packages/block-editor/src/components/default-block-appender/index.js

Lines 38 to 49 in 74269e6

// 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

gutenberg/packages/components/src/higher-order/with-focus-outside/index.js

Lines 100 to 121 in 74269e6

/**

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

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.

It's working fine for me now 🎉

tellthemachines · 2019-12-16T05:30:22Z

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


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.

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.

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:

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.

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.

Firefox, Safari compatibility

…pender Handled now via ancestor BlockListAppender in which this component would be rendered

tellthemachines

Looks good!

tellthemachines · 2019-12-19T01:07:26Z

 	 */
 	const onFocus = () => {
-		if ( ! isSelected && ! isAncestorOfSelectedBlock && ! isPartOfMultiSelection ) {
+		if ( ! isSelected && ! isPartOfMultiSelection ) {


It's working fine for me now 🎉

tellthemachines · 2019-12-19T01:11:53Z

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


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.

draganescu

I have tested this as well in both FF and Chrome for the problems which the previous PR tried to solve:

one click appending of navigation item
not deselecting a block when the flow block appender was clicked
maintain one click append for the group block
fix the new arrow navigation issues and parent block click selection problem whcih the previous PR introduced.

Everything worked great!

Block Editor: Consistently render IgnoreNestedEvents for block appender

ac00564

aduth requested review from ajitbohra, ellatrix, nerrad, ntwb, talldan and youknowriad as code owners December 13, 2019 20:30

This was referenced Dec 13, 2019

Prevent focus steal on clicking the inserter #18379

Merged

Using the up arrow key to navigate to parent blocks no longer works properly #18928

Closed

Block Editor: Fix grammar in BlockListAppender comment

cd494a7

tellthemachines reviewed Dec 16, 2019

View reviewed changes

aduth added 2 commits December 16, 2019 09:15

Block Editor: Capture button click focus in BlockListAppender

9091e97

Firefox, Safari compatibility

Block Editor: Remove redundant IgnoreNestedEvents from InnerBlocks ap…

3b2132f

…pender Handled now via ancestor BlockListAppender in which this component would be rendered

tellthemachines approved these changes Dec 19, 2019

View reviewed changes

draganescu approved these changes Dec 19, 2019

View reviewed changes

draganescu merged commit 1970f29 into master Dec 19, 2019

draganescu deleted the fix/18928-block-ancestor-focus branch December 19, 2019 15:51

draganescu mentioned this pull request Dec 19, 2019

The appender in the group block is missing icon #19246

Closed

aduth mentioned this pull request Jan 3, 2020

Move navigation and selection logic to WritingFlow #19397

Merged

6 tasks

youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020

	// 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;
	}
	}

Conversation

aduth commented Dec 13, 2019

Uh oh!

ZebulanStanphill commented Dec 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tellthemachines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

draganescu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants