Fix labeling of the navigation links in the list view. (#59370)

* Fix labeling of the navigation links in the site editor list view. * Adjust tests. * Simplify labeling by removing aria-label and using text content. * Improve logic and add test. * Adjust test. * Adjust block renaming test. * Adjust list view test. * Revert changes to useBlockDisplayTitle. * Make heading with only spaces be considered empty. Co-authored-by: afercia <[email protected]> Co-authored-by: alexstine <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: youknowriad <[email protected]>
WordPress · Mar 19, 2024 · c654171 · c654171
1 parent 9757688
commit c654171
Show file tree

Hide file tree

Showing 9 changed files with 132 additions and 103 deletions.
diff --git a/packages/block-editor/src/components/list-view/block-select-button.js b/packages/block-editor/src/components/list-view/block-select-button.js
@@ -47,7 +47,6 @@ function ListViewBlockSelectButton(
 		onDragEnd,
 		draggable,
 		isExpanded,
-		ariaLabel,
 		ariaDescribedBy,
 		updateFocusAndSelection,
 	},
@@ -249,7 +248,6 @@ function ListViewBlockSelectButton(
 				onDragEnd={ onDragEnd }
 				draggable={ draggable }
 				href={ `#block-${ clientId }` }
-				aria-label={ ariaLabel }
 				aria-describedby={ ariaDescribedBy }
 				aria-expanded={ isExpanded }
 			>

diff --git a/packages/block-editor/src/components/list-view/block.js b/packages/block-editor/src/components/list-view/block.js
@@ -21,7 +21,7 @@ import {
 	memo,
 } from '@wordpress/element';
 import { useDispatch, useSelect } from '@wordpress/data';
-import { sprintf, __ } from '@wordpress/i18n';
+import { __ } from '@wordpress/i18n';
 import { ESCAPE } from '@wordpress/keycodes';
 
 /**
@@ -35,7 +35,11 @@ import {
 } from '../block-mover/button';
 import ListViewBlockContents from './block-contents';
 import { useListViewContext } from './context';
-import { getBlockPositionDescription, focusListItem } from './utils';
+import {
+	getBlockPositionDescription,
+	getBlockPropertiesDescription,
+	focusListItem,
+} from './utils';
 import { store as blockEditorStore } from '../../store';
 import useBlockDisplayInformation from '../use-block-display-information';
 import { useBlockLock } from '../block-lock';
@@ -77,8 +81,6 @@ function ListViewBlock( {
 	const { toggleBlockHighlight } = useDispatch( blockEditorStore );
 
 	const blockInformation = useBlockDisplayInformation( clientId );
-	const blockTitle =
-		blockInformation?.name || blockInformation?.title || __( 'Untitled' );
 
 	const { block, blockName, blockEditingMode } = useSelect(
 		( select ) => {
@@ -93,6 +95,7 @@ function ListViewBlock( {
 		},
 		[ clientId ]
 	);
+
 	const allowRightClickOverrides = useSelect(
 		( select ) =>
 			select( blockEditorStore ).getSettings().allowRightClickOverrides,
@@ -107,7 +110,7 @@ function ListViewBlock( {
 		// Don't show the settings menu if block is disabled or content only.
 		blockEditingMode === 'default';
 	const instanceId = useInstanceId( ListViewBlock );
-	const descriptionId = `list-view-block-select-button__${ instanceId }`;
+	const descriptionId = `list-view-block-select-button__description-${ instanceId }`;
 
 	const {
 		expand,
@@ -247,19 +250,8 @@ function ListViewBlock( {
 		level
 	);
 
-	const blockAriaLabel = isLocked
-		? sprintf(
-				// translators: %s: The title of the block. This string indicates a link to select the locked block.
-				__( '%s (locked)' ),
-				blockTitle
-		  )
-		: blockTitle;
-
-	const settingsAriaLabel = sprintf(
-		// translators: %s: The title of the block.
-		__( 'Options for %s' ),
-		blockTitle
-	);
+	const blockPropertiesDescription =
+		getBlockPropertiesDescription( isLocked );
 
 	const hasSiblings = siblingBlockCount > 0;
 	const hasRenderedMovers = showBlockMovers && hasSiblings;
@@ -352,12 +344,11 @@ function ListViewBlock( {
 							onFocus={ onFocus }
 							isExpanded={ canEdit ? isExpanded : undefined }
 							selectedClientIds={ selectedClientIds }
-							ariaLabel={ blockAriaLabel }
 							ariaDescribedBy={ descriptionId }
 							updateFocusAndSelection={ updateFocusAndSelection }
 						/>
 						<AriaReferencedText id={ descriptionId }>
-							{ blockPositionDescription }
+							{ `${ blockPositionDescription } ${ blockPropertiesDescription }` }
 						</AriaReferencedText>
 					</div>
 				) }
@@ -405,7 +396,7 @@ function ListViewBlock( {
 							clientIds={ dropdownClientIds }
 							block={ block }
 							icon={ moreVertical }
-							label={ settingsAriaLabel }
+							label={ __( 'Actions' ) }
 							popoverProps={ {
 								anchor: settingsPopoverAnchor, // Used to position the settings at the cursor on right-click.
 							} }

diff --git a/packages/block-editor/src/components/list-view/utils.js b/packages/block-editor/src/components/list-view/utils.js
@@ -7,12 +7,15 @@ import { focus } from '@wordpress/dom';
 export const getBlockPositionDescription = ( position, siblingCount, level ) =>
 	sprintf(
 		/* translators: 1: The numerical position of the block. 2: The total number of blocks. 3. The level of nesting for the block. */
-		__( 'Block %1$d of %2$d, Level %3$d' ),
+		__( 'Block %1$d of %2$d, Level %3$d.' ),
 		position,
 		siblingCount,
 		level
 	);
 
+export const getBlockPropertiesDescription = ( isLocked ) =>
+	isLocked ? __( 'This block is locked.' ) : '';
+
 /**
  * Returns true if the client ID occurs within the block selection or multi-selection,
  * or false otherwise.

diff --git a/packages/block-library/src/heading/index.js b/packages/block-library/src/heading/index.js
@@ -30,7 +30,7 @@ export const settings = {
 		const { content, level } = attributes;
 
 		const customName = attributes?.metadata?.name;
-		const hasContent = content?.length > 0;
+		const hasContent = content?.trim().length > 0;
 
 		// In the list view, use the block's content as the label.
 		// If the content is empty, fall back to the default label.

diff --git a/packages/editor/src/components/document-outline/index.js b/packages/editor/src/components/document-outline/index.js
@@ -95,7 +95,8 @@ const computeOutlineHeadings = ( blocks = [] ) => {
 };
 
 const isEmptyHeading = ( heading ) =>
-	! heading.attributes.content || heading.attributes.content.length === 0;
+	! heading.attributes.content ||
+	heading.attributes.content.trim().length === 0;
 
 export default function DocumentOutline( {
 	onSelect,

diff --git a/test/e2e/specs/editor/blocks/navigation-list-view.spec.js b/test/e2e/specs/editor/blocks/navigation-list-view.spec.js
@@ -103,34 +103,31 @@ test.describe( 'Navigation block - List view editing', () => {
 		await expect(
 			listView
 				.getByRole( 'gridcell', {
-					name: 'Page Link',
+					name: 'Top Level Item 1',
 				} )
 				.filter( {
-					hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
+					hasText: 'Block 1 of 2, Level 1.', // proxy for filtering by description.
 				} )
-				.getByText( 'Top Level Item 1' )
 		).toBeVisible();
 
 		await expect(
 			listView
 				.getByRole( 'gridcell', {
-					name: 'Submenu',
+					name: 'Top Level Item 2',
 				} )
 				.filter( {
-					hasText: 'Block 2 of 2, Level 1', // proxy for filtering by description.
+					hasText: 'Block 2 of 2, Level 1.', // proxy for filtering by description.
 				} )
-				.getByText( 'Top Level Item 2' )
 		).toBeVisible();
 
 		await expect(
 			listView
 				.getByRole( 'gridcell', {
-					name: 'Page Link',
+					name: 'Test Submenu Item',
 				} )
 				.filter( {
-					hasText: 'Block 1 of 1, Level 2', // proxy for filtering by description.
+					hasText: 'Block 1 of 1, Level 2.', // proxy for filtering by description.
 				} )
-				.getByText( 'Test Submenu Item' )
 		).toBeVisible();
 	} );
 
@@ -241,12 +238,11 @@ test.describe( 'Navigation block - List view editing', () => {
 		await expect(
 			listView
 				.getByRole( 'gridcell', {
-					name: 'Page Link',
+					name: firstResultText,
 				} )
 				.filter( {
-					hasText: 'Block 3 of 3, Level 1', // proxy for filtering by description.
+					hasText: 'Block 3 of 3, Level 1.', // proxy for filtering by description.
 				} )
-				.getByText( firstResultText )
 		).toBeVisible();
 	} );
 
@@ -262,35 +258,45 @@ test.describe( 'Navigation block - List view editing', () => {
 			description: 'Structure for navigation menu: Test Menu',
 		} );
 
-		const submenuOptions = listView.getByRole( 'button', {
-			name: 'Options for Submenu',
-		} );
+		const subMenuItem = listView
+			.getByRole( 'gridcell', {
+				name: 'Top Level Item 2',
+			} )
+			.filter( {
+				hasText: 'Block 2 of 2, Level 1.', // proxy for filtering by description.
+			} );
+
+		// The actions menu button is a sibling of the menu item gridcell.
+		const subMenuItemActions = subMenuItem
+			.locator( '..' ) // parent selector.
+			.getByRole( 'button', {
+				name: 'Actions',
+			} );
 
-		// Open the options menu.
-		await submenuOptions.click();
+		// Open the actions menu.
+		await subMenuItemActions.click();
 
-		// usage of `page` is required because the options menu is rendered into a slot
+		// usage of `page` is required because the actions menu is rendered into a slot
 		// outside of the treegrid.
-		const removeBlockOption = page
+		const removeBlockAction = page
 			.getByRole( 'menu', {
-				name: 'Options for Submenu',
+				name: 'Actions',
 			} )
 			.getByRole( 'menuitem', {
 				name: 'Remove Top Level Item 2',
 			} );
 
-		await removeBlockOption.click();
+		await removeBlockAction.click();
 
 		// Check the menu item was removed.
 		await expect(
 			listView
 				.getByRole( 'gridcell', {
-					name: 'Submenu',
+					name: 'Top Level Item 2',
 				} )
 				.filter( {
-					hasText: 'Block 2 of 2, Level 1', // proxy for filtering by description.
+					hasText: 'Block 2 of 2, Level 1.', // proxy for filtering by description.
 				} )
-				.getByText( 'Top Level Item 2' )
 		).toBeHidden();
 	} );
 
@@ -307,12 +313,9 @@ test.describe( 'Navigation block - List view editing', () => {
 		} );
 
 		// Click on the first menu item to open its settings.
-		const firstMenuItemAnchor = listView
-			.getByRole( 'link', {
-				name: 'Page',
-				includeHidden: true,
-			} )
-			.getByText( 'Top Level Item 1' );
+		const firstMenuItemAnchor = listView.getByRole( 'link', {
+			name: 'Top Level Item 1',
+		} );
 		await firstMenuItemAnchor.click();
 
 		// Get the settings panel.
@@ -373,12 +376,11 @@ test.describe( 'Navigation block - List view editing', () => {
 		await expect(
 			listViewPanel
 				.getByRole( 'gridcell', {
-					name: 'Page Link',
+					name: 'Changed label', // new menu item text
 				} )
 				.filter( {
-					hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
+					hasText: 'Block 1 of 2, Level 1.', // proxy for filtering by description.
 				} )
-				.getByText( 'Changed label' ) // new label text
 		).toBeVisible();
 	} );
 
@@ -399,37 +401,37 @@ test.describe( 'Navigation block - List view editing', () => {
 			description: 'Structure for navigation menu: Test Menu',
 		} );
 
-		// click on options menu for the first menu item and select remove.
+		// click on actions menu for the first menu item and select remove.
 		const firstMenuItem = listView
 			.getByRole( 'gridcell', {
-				name: 'Page Link',
+				name: 'Top Level Item 1',
 			} )
 			.filter( {
-				hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
+				hasText: 'Block 1 of 2, Level 1.', // proxy for filtering by description.
 			} );
 
-		// The options menu button is a sibling of the menu item gridcell.
-		const firstItemOptions = firstMenuItem
+		// The actions menu button is a sibling of the menu item gridcell.
+		const firstItemActions = firstMenuItem
 			.locator( '..' ) // parent selector.
 			.getByRole( 'button', {
-				name: 'Options for Page Link',
+				name: 'Actions',
 			} );
 
-		// Open the options menu.
-		await firstItemOptions.click();
+		// Open the actions menu.
+		await firstItemActions.click();
 
 		// Add the submenu.
-		// usage of `page` is required because the options menu is rendered into a slot
+		// usage of `page` is required because the actions menu is rendered into a slot
 		// outside of the treegrid.
-		const addSubmenuOption = page
+		const addSubmenuAction = page
 			.getByRole( 'menu', {
-				name: 'Options for Page Link',
+				name: 'Actions',
 			} )
 			.getByRole( 'menuitem', {
 				name: 'Add submenu',
 			} );
 
-		await addSubmenuOption.click();
+		await addSubmenuAction.click();
 
 		await linkControl.searchFor( 'https://wordpress.org' );
 
@@ -439,25 +441,23 @@ test.describe( 'Navigation block - List view editing', () => {
 		await expect(
 			listView
 				.getByRole( 'gridcell', {
-					name: 'Custom Link',
+					name: 'wordpress.org',
 				} )
 				.filter( {
-					hasText: 'Block 1 of 1, Level 2', // proxy for filtering by description.
+					hasText: 'Block 1 of 1, Level 2.', // proxy for filtering by description.
 				} )
-				.getByText( 'wordpress.org' )
 		).toBeVisible();
 
 		// Check that the original item is still there but that it is now
 		// a submenu item.
 		await expect(
 			listView
 				.getByRole( 'gridcell', {
-					name: 'Submenu',
+					name: 'Top Level Item 1',
 				} )
 				.filter( {
-					hasText: 'Block 1 of 2, Level 1', // proxy for filtering by description.
+					hasText: 'Block 1 of 2, Level 1.', // proxy for filtering by description.
 				} )
-				.getByText( 'Top Level Item 1' )
 		).toBeVisible();
 	} );