Block Editor: Consistently render IgnoreNestedEvents for block append…

…er (#19135)

* Block Editor: Consistently render IgnoreNestedEvents for block appender

* Block Editor: Fix grammar in BlockListAppender comment

* Block Editor: Capture button click focus in BlockListAppender

Firefox, Safari compatibility

* Block Editor: Remove redundant IgnoreNestedEvents from InnerBlocks appender

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

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

@@ -23,49 +23,54 @@ 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).
+	//
+	// A `tabIndex` is used on the wrapping `div` element in order to force a
+	// focus event to occur when an appender `button` element is clicked. In
+	// some browsers (Firefox, Safari), button clicks do not emit a focus event,
+	// which could cause this event to propagate unexpectedly. The `tabIndex`
+	// ensures that the interaction is captured as a focus, without also adding
+	// an extra tab stop.
+	//
+	// See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
+	return (
+		<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
+			<div tabIndex={ -1 } className="block-list-appender">
+				{ appender }
+			</div>
+		</IgnoreNestedEvents>
 	);
 }
 

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

@@ -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 ) {
 			onSelect();
 		}
 	};

packages/block-editor/src/components/inner-blocks/default-block-appender.js

@@ -12,18 +12,15 @@ import { withSelect } from '@wordpress/data';
 /**
  * Internal dependencies
  */
-import IgnoreNestedEvents from '../ignore-nested-events';
 import BaseDefaultBlockAppender from '../default-block-appender';
 import withClientId from './with-client-id';
 
 export const DefaultBlockAppender = ( { clientId, lastBlockClientId } ) => {
 	return (
-		<IgnoreNestedEvents childHandledEvents={ [ 'onFocus', 'onClick', 'onKeyDown' ] }>
-			<BaseDefaultBlockAppender
-				rootClientId={ clientId }
-				lastBlockClientId={ lastBlockClientId }
-			/>
-		</IgnoreNestedEvents>
+		<BaseDefaultBlockAppender
+			rootClientId={ clientId }
+			lastBlockClientId={ lastBlockClientId }
+		/>
 	);
 };
 

packages/block-editor/src/components/inserter/index.js

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

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

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