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

Conversation

@aduth
Copy link
Member

aduth commented Dec 13, 2019

Fixes #18928
Related: #18379

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

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 13, 2019

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

@@ -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 ) {

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Dec 16, 2019

Contributor

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

This comment has been minimized.

Copy link
@draganescu

draganescu Dec 16, 2019

Contributor

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

This comment has been minimized.

Copy link
@aduth

aduth Dec 16, 2019

Author Member

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

This comment has been minimized.

Copy link
@aduth

aduth Dec 16, 2019

Author Member

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.

This comment has been minimized.

Copy link
@aduth

aduth Dec 16, 2019

Author Member

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.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Dec 19, 2019

Contributor

It's working fine for me now 🎉

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

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Dec 16, 2019

Contributor

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.

This comment has been minimized.

Copy link
@aduth

aduth Dec 16, 2019

Author Member

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.

This comment has been minimized.

Copy link
@aduth

aduth Dec 16, 2019

Author Member

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.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Dec 19, 2019

Contributor

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.

aduth added 2 commits Dec 16, 2019
…pender

Handled now via ancestor BlockListAppender in which this component would be rendered
Copy link
Contributor

tellthemachines left a comment

Looks good!

@@ -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 ) {

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Dec 19, 2019

Contributor

It's working fine for me now 🎉

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

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Dec 19, 2019

Contributor

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.

Copy link
Contributor

draganescu left a comment

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!

@draganescu draganescu merged commit 1970f29 into master Dec 19, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@draganescu draganescu deleted the fix/18928-block-ancestor-focus branch Dec 19, 2019
@aduth aduth mentioned this pull request Jan 3, 2020
6 of 6 tasks complete
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.