Do not focus new navigation block menu until loading is finished (WordPress#59801)

jeryj · draganescu · getdave · web-flow · commit 08f14e9896d2 · 2024-03-14T10:06:31.000Z
* Do not focus navigation block until loading is finished

When a new navigation is created, we place focus on the navigation block. If we place focus while loading is still happening, focus will be lost when focus the loading is finished and the block gets replaced with the new content.

* Nest loading state inside of navigation ref

Instead of replacing the navigation block after loading, only replace the inside part of it so that focus is not lost

* Add coverage for focus loss on menu creation

* select the navigatiom after import in an effect similar to the method used when creating a menu, this way the select block call is later and the focus is maintained

* remove extraneous dependency causing err on delete newly created menus

* Update test to not determine link control behavior

* No need to return await inside an async function as it will always return a Promise

Co-authored-by: Dave Smith &lt;getdavemail@gmail.com&gt;

---------

Co-authored-by: Andrei Draganescu &lt;andrei.draganescu@automattic.com&gt;
Co-authored-by: Dave Smith &lt;getdavemail@gmail.com&gt;
diff --git a/packages/block-library/src/navigation/edit/index.js b/packages/block-library/src/navigation/edit/index.js
@@ -342,16 +342,7 @@ function Navigation( {
 	const [ detectedOverlayColor, setDetectedOverlayColor ] = useState();
 
 	const onSelectClassicMenu = async ( classicMenu ) => {
-		const navMenu = await convertClassicMenu(
-			classicMenu.id,
-			classicMenu.name,
-			'draft'
-		);
-		if ( navMenu ) {
-			handleUpdateMenu( navMenu.id, {
-				focusNavigationBlock: true,
-			} );
-		}
+		return convertClassicMenu( classicMenu.id, classicMenu.name, 'draft' );
 	};
 
 	const onSelectNavigationMenu = ( menuId ) => {
@@ -402,6 +393,9 @@ function Navigation( {
 			showClassicMenuConversionNotice(
 				__( 'Classic menu imported successfully.' )
 			);
+			handleUpdateMenu( createNavigationMenuPost?.id, {
+				focusNavigationBlock: true,
+			} );
 		}
 
 		if ( classicMenuConversionStatus === CLASSIC_MENU_CONVERSION_ERROR ) {
@@ -414,6 +408,8 @@ function Navigation( {
 		classicMenuConversionError,
 		hideClassicMenuConversionNotice,
 		showClassicMenuConversionNotice,
+		createNavigationMenuPost?.id,
+		handleUpdateMenu,
 	] );
 
 	useEffect( () => {
@@ -866,50 +862,52 @@ function Navigation( {
 					</InspectorControls>
 				) }
 
-				{ isLoading && (
-					<TagName { ...blockProps }>
+				<TagName
+					{ ...blockProps }
+					aria-describedby={
+						! isPlaceholder && ! isLoading
+							? accessibleDescriptionId
+							: undefined
+					}
+				>
+					{ isLoading && (
 						<div className="wp-block-navigation__loading-indicator-container">
 							<Spinner className="wp-block-navigation__loading-indicator" />
 						</div>
-					</TagName>
-				) }
+					) }
 
-				{ ! isLoading && (
-					<TagName
-						{ ...blockProps }
-						aria-describedby={
-							! isPlaceholder
-								? accessibleDescriptionId
-								: undefined
-						}
-					>
-						<AccessibleMenuDescription
-							id={ accessibleDescriptionId }
-						/>
-						<ResponsiveWrapper
-							id={ clientId }
-							onToggle={ setResponsiveMenuVisibility }
-							hasIcon={ hasIcon }
-							icon={ icon }
-							isOpen={ isResponsiveMenuOpen }
-							isResponsive={ isResponsive }
-							isHiddenByDefault={ 'always' === overlayMenu }
-							overlayBackgroundColor={ overlayBackgroundColor }
-							overlayTextColor={ overlayTextColor }
-						>
-							{ isEntityAvailable && (
-								<NavigationInnerBlocks
-									clientId={ clientId }
-									hasCustomPlaceholder={
-										!! CustomPlaceholder
-									}
-									templateLock={ templateLock }
-									orientation={ orientation }
-								/>
-							) }
-						</ResponsiveWrapper>
-					</TagName>
-				) }
+					{ ! isLoading && (
+						<>
+							<AccessibleMenuDescription
+								id={ accessibleDescriptionId }
+							/>
+							<ResponsiveWrapper
+								id={ clientId }
+								onToggle={ setResponsiveMenuVisibility }
+								hasIcon={ hasIcon }
+								icon={ icon }
+								isOpen={ isResponsiveMenuOpen }
+								isResponsive={ isResponsive }
+								isHiddenByDefault={ 'always' === overlayMenu }
+								overlayBackgroundColor={
+									overlayBackgroundColor
+								}
+								overlayTextColor={ overlayTextColor }
+							>
+								{ isEntityAvailable && (
+									<NavigationInnerBlocks
+										clientId={ clientId }
+										hasCustomPlaceholder={
+											!! CustomPlaceholder
+										}
+										templateLock={ templateLock }
+										orientation={ orientation }
+									/>
+								) }
+							</ResponsiveWrapper>
+						</>
+					) }
+				</TagName>
 			</RecursionProvider>
 		</EntityProvider>
 	);
diff --git a/test/e2e/specs/editor/blocks/navigation-list-view.spec.js b/test/e2e/specs/editor/blocks/navigation-list-view.spec.js
@@ -543,6 +543,48 @@ test.describe( 'Navigation block - List view editing', () => {
 		// we have unmounted the list view and then remounted it).
 		await expect( linkControl.getSearchInput() ).toBeHidden();
 	} );
+
+	test( `can create a new menu without losing focus`, async ( {
+		page,
+		editor,
+		requestUtils,
+	} ) => {
+		await requestUtils.createNavigationMenu( navMenuBlocksFixture );
+
+		await editor.insertBlock( { name: 'core/navigation' } );
+
+		await editor.openDocumentSettingsSidebar();
+
+		await page.getByLabel( 'Test Menu' ).click();
+
+		await page.keyboard.press( 'ArrowUp' );
+
+		await expect(
+			page.getByRole( 'menuitem', { name: 'Create new menu' } )
+		).toBeFocused();
+
+		await page.keyboard.press( 'Enter' );
+
+		// Check that the menu was created
+		await expect(
+			page
+				.getByTestId( 'snackbar' )
+				.getByText( 'Navigation Menu successfully created.' )
+		).toBeVisible();
+		await expect(
+			page.getByText( 'This navigation menu is empty.' )
+		).toBeVisible();
+
+		// Move focus to the appender
+		await page.keyboard.press( 'ArrowDown' );
+		await expect(
+			editor.canvas
+				.getByRole( 'document', {
+					name: 'Block: Navigation',
+				} )
+				.getByLabel( 'Add block' )
+		).toBeFocused();
+	} );
 } );
 
 class LinkControl {
