From f145e76241c32c368066cffdcc64d08c58bf1ea9 Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Mon, 26 Feb 2024 15:28:21 +0100 Subject: [PATCH 1/9] Fix labeling of the navigation links in the site editor list view. --- .../src/components/list-view/block.js | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/packages/block-editor/src/components/list-view/block.js b/packages/block-editor/src/components/list-view/block.js index 0ee7d6b4f3da8c..851caee324e183 100644 --- a/packages/block-editor/src/components/list-view/block.js +++ b/packages/block-editor/src/components/list-view/block.js @@ -77,8 +77,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 +91,13 @@ function ListViewBlock( { }, [ clientId ] ); + + const listViewItemLabel = + block.attributes?.label || // Visible label of a block item e.g. for Navigation link block. + blockInformation?.name || // Custom Block name for user-renamed blocks: attributes?.metadata?.name. + blockInformation?.title || // Synced pattern name or Block type name e.g. 'Paragraph': resusableTitle || blockType.title; + __( 'Untitled' ); // Fallback title. + const allowRightClickOverrides = useSelect( ( select ) => select( blockEditorStore ).getSettings().allowRightClickOverrides, @@ -251,15 +256,9 @@ function ListViewBlock( { ? sprintf( // translators: %s: The title of the block. This string indicates a link to select the locked block. __( '%s (locked)' ), - blockTitle + listViewItemLabel ) - : blockTitle; - - const settingsAriaLabel = sprintf( - // translators: %s: The title of the block. - __( 'Options for %s' ), - blockTitle - ); + : listViewItemLabel; const hasSiblings = siblingBlockCount > 0; const hasRenderedMovers = showBlockMovers && hasSiblings; @@ -405,7 +404,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. } } From 8d94b330e4da1462bbf536a73fdea2b9c04fbb40 Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Tue, 27 Feb 2024 15:12:54 +0100 Subject: [PATCH 2/9] Adjust tests. --- .../blocks/navigation-list-view.spec.js | 84 +++++++++---------- .../editor/various/block-renaming.spec.js | 29 ++++--- .../specs/editor/various/list-view.spec.js | 35 ++++++-- .../site-editor/navigation-editor.spec.js | 13 +-- 4 files changed, 95 insertions(+), 66 deletions(-) diff --git a/test/e2e/specs/editor/blocks/navigation-list-view.spec.js b/test/e2e/specs/editor/blocks/navigation-list-view.spec.js index c97ea3f1b17b51..681d3afd351b0c 100644 --- 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. } ) - .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. } ) - .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. } ) - .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. } ) - .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. } ) - .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. } ) - .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. } ); - // 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,12 +441,11 @@ 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. } ) - .getByText( 'wordpress.org' ) ).toBeVisible(); // Check that the original item is still there but that it is now @@ -452,12 +453,11 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) - .getByText( 'Top Level Item 1' ) ).toBeVisible(); } ); diff --git a/test/e2e/specs/editor/various/block-renaming.spec.js b/test/e2e/specs/editor/various/block-renaming.spec.js index a106d794e2e60b..0411d196fea4a8 100644 --- a/test/e2e/specs/editor/various/block-renaming.spec.js +++ b/test/e2e/specs/editor/various/block-renaming.spec.js @@ -178,15 +178,15 @@ test.describe( 'Block Renaming', () => { // Select via keyboard. await pageUtils.pressKeys( 'primary+a' ); - const blockOptionsTrigger = listView.getByRole( 'button', { - name: 'Options for No Rename Support Block', + const blockActionsTrigger = listView.getByRole( 'button', { + name: 'Actions', } ); - await blockOptionsTrigger.click(); + await blockActionsTrigger.click(); const renameMenuItem = page .getByRole( 'menu', { - name: 'Options for No Rename Support Block', + name: 'Actions', } ) .getByRole( 'menuitem', { name: 'Rename', @@ -196,7 +196,7 @@ test.describe( 'Block Renaming', () => { await expect( renameMenuItem ).toBeHidden(); } ); - test( 'displays Rename option in related menu when block is not selected', async ( { + test( 'displays Rename action in related menu when block is not selected', async ( { editor, page, pageUtils, @@ -222,16 +222,25 @@ test.describe( 'Block Renaming', () => { } ) .click(); - // Trigger options menu for the Heading (not the selected block). - const blockOptionsTrigger = listView.getByRole( 'button', { - name: 'Options for Heading', + // Trigger actions menu for the Heading (not the selected block). + const headingItem = listView.getByRole( 'gridcell', { + name: 'Heading', } ); - await blockOptionsTrigger.click(); + // The actions menu button is a sibling of the menu item gridcell. + const headingItemActions = headingItem + .locator( '..' ) // parent selector. + .getByRole( 'button', { + name: 'Actions', + } ); + + await headingItemActions.click(); + // usage of `page` is required because the actions menu is rendered + // into a slot outside of the treegrid. const renameMenuItem = page .getByRole( 'menu', { - name: 'Options for Heading', + name: 'Actions', } ) .getByRole( 'menuitem', { name: 'Rename', diff --git a/test/e2e/specs/editor/various/list-view.spec.js b/test/e2e/specs/editor/various/list-view.spec.js index cb15c12c84b490..ef26df266657a1 100644 --- a/test/e2e/specs/editor/various/list-view.spec.js +++ b/test/e2e/specs/editor/various/list-view.spec.js @@ -271,14 +271,32 @@ test.describe( 'List View', () => { // Navigate the right column to image block options button via Home key. await page.keyboard.press( 'ArrowRight' ); await page.keyboard.press( 'Home' ); + + const imageItem = listView.getByRole( 'gridcell', { + name: 'Image', + } ); + await expect( - listView.getByRole( 'button', { name: 'Options for Image' } ) + imageItem + .locator( '..' ) // parent selector. + .getByRole( 'button', { + name: 'Actions', + } ) ).toBeFocused(); // Navigate the right column to group block options button. await page.keyboard.press( 'End' ); + + const groupItem = listView.getByRole( 'gridcell', { + name: 'Group', + } ); + await expect( - listView.getByRole( 'button', { name: 'Options for Group' } ) + groupItem + .locator( '..' ) // parent selector. + .getByRole( 'button', { + name: 'Actions', + } ) ).toBeFocused(); } ); @@ -918,11 +936,12 @@ test.describe( 'List View', () => { const listView = await listViewUtils.openListView(); await listView - .getByRole( 'button', { name: 'Options for Heading' } ) + .getByRole( 'button', { name: 'Actions' } ) + .first() .click(); await page - .getByRole( 'menu', { name: 'Options for Heading' } ) + .getByRole( 'menu', { name: 'Actions' } ) .getByRole( 'menuitem', { name: 'Duplicate' } ) .click(); await expect @@ -938,11 +957,11 @@ test.describe( 'List View', () => { await page.keyboard.press( 'Shift+ArrowUp' ); await listView - .getByRole( 'button', { name: 'Options for Heading' } ) + .getByRole( 'button', { name: 'Actions' } ) .first() .click(); await page - .getByRole( 'menu', { name: 'Options for Heading' } ) + .getByRole( 'menu', { name: 'Actions' } ) .getByRole( 'menuitem', { name: 'Delete' } ) .click(); await expect @@ -960,9 +979,9 @@ test.describe( 'List View', () => { .filter( { has: page.getByRole( 'gridcell', { name: 'File' } ), } ) - .getByRole( 'button', { name: 'Options for File' } ); + .getByRole( 'button', { name: 'Actions' } ); const optionsForFileMenu = page.getByRole( 'menu', { - name: 'Options for File', + name: 'Actions', } ); await expect( optionsForFileToggle, diff --git a/test/e2e/specs/site-editor/navigation-editor.spec.js b/test/e2e/specs/site-editor/navigation-editor.spec.js index 0761f35535ba8e..ca839405089816 100644 --- a/test/e2e/specs/site-editor/navigation-editor.spec.js +++ b/test/e2e/specs/site-editor/navigation-editor.spec.js @@ -20,7 +20,7 @@ test.describe( 'Editing Navigation Menus', () => { editor, } ) => { await test.step( 'Check Navigation block is present and locked', async () => { - // create a Navigation Menu called "Test Menu" using the REST API helpers + // Create a Navigation Menu called "Primary Menu" using the REST API helpers. const createdMenu = await requestUtils.createNavigationMenu( { title: 'Primary Menu', content: @@ -61,12 +61,13 @@ test.describe( 'Editing Navigation Menus', () => { // The Navigation block should be present and locked. await expect( navBlockNode ).toBeVisible(); - // The block should have no options menu. + // The block should have no actions menu. await expect( - listView.getByRole( 'button', { - name: 'Options for Navigation', - exact: true, - } ) + navBlockNode + .locator( '..' ) // parent selector. + .getByRole( 'button', { + name: 'Actions', + } ) ).toBeHidden(); // Select the Navigation block. From 7dcc5088b85b86bee9e174886bcbf942db271d07 Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Mon, 4 Mar 2024 15:59:47 +0100 Subject: [PATCH 3/9] Simplify labeling by removing aria-label and using text content. --- .../block-title/use-block-display-title.js | 5 ++-- .../list-view/block-select-button.js | 2 -- .../src/components/list-view/block.js | 28 +++++++------------ .../src/components/list-view/utils.js | 5 +++- .../blocks/navigation-list-view.spec.js | 20 ++++++------- 5 files changed, 27 insertions(+), 33 deletions(-) diff --git a/packages/block-editor/src/components/block-title/use-block-display-title.js b/packages/block-editor/src/components/block-title/use-block-display-title.js index a51b336554a2a7..eab7d96558e19f 100644 --- a/packages/block-editor/src/components/block-title/use-block-display-title.js +++ b/packages/block-editor/src/components/block-title/use-block-display-title.js @@ -6,6 +6,7 @@ import { __experimentalGetBlockLabel as getBlockLabel, store as blocksStore, } from '@wordpress/blocks'; +import { __ } from '@wordpress/i18n'; /** * Internal dependencies @@ -64,8 +65,8 @@ export default function useBlockDisplayTitle( { [ clientId, context ] ); - if ( ! blockTitle ) { - return null; + if ( ! blockTitle || blockTitle.trim() === '' ) { + return __( 'Untitled' ); } if ( 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 index 6b9de943ea0bf2..7fc5f91012f87e 100644 --- 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 index 851caee324e183..bc21e40d7e8d82 100644 --- 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'; @@ -92,12 +96,6 @@ function ListViewBlock( { [ clientId ] ); - const listViewItemLabel = - block.attributes?.label || // Visible label of a block item e.g. for Navigation link block. - blockInformation?.name || // Custom Block name for user-renamed blocks: attributes?.metadata?.name. - blockInformation?.title || // Synced pattern name or Block type name e.g. 'Paragraph': resusableTitle || blockType.title; - __( 'Untitled' ); // Fallback title. - const allowRightClickOverrides = useSelect( ( select ) => select( blockEditorStore ).getSettings().allowRightClickOverrides, @@ -112,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, @@ -252,13 +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)' ), - listViewItemLabel - ) - : listViewItemLabel; + const blockPropertiesDescription = + getBlockPropertiesDescription( isLocked ); const hasSiblings = siblingBlockCount > 0; const hasRenderedMovers = showBlockMovers && hasSiblings; @@ -351,12 +344,11 @@ function ListViewBlock( { onFocus={ onFocus } isExpanded={ canEdit ? isExpanded : undefined } selectedClientIds={ selectedClientIds } - ariaLabel={ blockAriaLabel } ariaDescribedBy={ descriptionId } updateFocusAndSelection={ updateFocusAndSelection } /> - { blockPositionDescription } + { `${ blockPositionDescription } ${ blockPropertiesDescription }` } ) } diff --git a/packages/block-editor/src/components/list-view/utils.js b/packages/block-editor/src/components/list-view/utils.js index c91376b0472116..dde1d39567f587 100644 --- 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/test/e2e/specs/editor/blocks/navigation-list-view.spec.js b/test/e2e/specs/editor/blocks/navigation-list-view.spec.js index 681d3afd351b0c..7689ebf07f375d 100644 --- a/test/e2e/specs/editor/blocks/navigation-list-view.spec.js +++ b/test/e2e/specs/editor/blocks/navigation-list-view.spec.js @@ -106,7 +106,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeVisible(); @@ -116,7 +116,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeVisible(); @@ -126,7 +126,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeVisible(); } ); @@ -241,7 +241,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeVisible(); } ); @@ -263,7 +263,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ); // The actions menu button is a sibling of the menu item gridcell. @@ -295,7 +295,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeHidden(); } ); @@ -379,7 +379,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeVisible(); } ); @@ -407,7 +407,7 @@ test.describe( 'Navigation block - List view editing', () => { 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 actions menu button is a sibling of the menu item gridcell. @@ -444,7 +444,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeVisible(); @@ -456,7 +456,7 @@ test.describe( 'Navigation block - List view editing', () => { 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. } ) ).toBeVisible(); } ); From 6060b9375508b69c5b518f2ec74496b44a7acca7 Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Wed, 6 Mar 2024 15:36:15 +0100 Subject: [PATCH 4/9] Improve logic and add test. --- .../src/components/block-title/test/index.js | 18 ++++++++++++++++++ .../block-title/use-block-display-title.js | 10 +++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/packages/block-editor/src/components/block-title/test/index.js b/packages/block-editor/src/components/block-title/test/index.js index 8a4d3c2f52fd7e..edc073b52e9f66 100644 --- a/packages/block-editor/src/components/block-title/test/index.js +++ b/packages/block-editor/src/components/block-title/test/index.js @@ -19,6 +19,9 @@ const blockTypeMap = { 'name-with-label': { title: 'Block With Label' }, 'name-with-custom-label': { title: 'Block With Custom Label' }, 'name-with-long-label': { title: 'Block With Long Label' }, + 'name-with-only-spaces-label': { + title: 'Block With Label With Only Spaces', + }, 'reusable-block': { title: 'Reusable Block' }, }; @@ -27,6 +30,7 @@ const blockLabelMap = { 'Block With Long Label': 'This is a longer label than typical for blocks to have.', 'Block With Custom Label': 'A Custom Label like a Block Variation Label', + 'Block With Label With Only Spaces': ' ', 'Reusable Block': 'Reuse me!', }; @@ -97,6 +101,20 @@ describe( 'BlockTitle', () => { expect( screen.getByText( 'Test Label' ) ).toBeVisible(); } ); + it( 'renders "Untitled" if label is made of only spaces', () => { + useSelect.mockImplementation( ( mapSelect ) => + mapSelect( () => ( { + getBlockName: () => 'name-with-only-spaces-label', + getBlockType: ( name ) => blockTypeMap[ name ], + getBlockAttributes: () => null, + } ) ) + ); + + render( ); + + expect( screen.getByText( 'Untitled' ) ).toBeVisible(); + } ); + it( 'should prioritize reusable block title over title', () => { useSelect.mockImplementation( ( mapSelect ) => mapSelect( () => ( { diff --git a/packages/block-editor/src/components/block-title/use-block-display-title.js b/packages/block-editor/src/components/block-title/use-block-display-title.js index eab7d96558e19f..c92ee795f932fc 100644 --- a/packages/block-editor/src/components/block-title/use-block-display-title.js +++ b/packages/block-editor/src/components/block-title/use-block-display-title.js @@ -55,7 +55,11 @@ export default function useBlockDisplayTitle( { const label = getBlockLabel( blockType, attributes, context ); // If the label is defined we prioritize it over a possible block variation title match. if ( label !== blockType.title ) { - return label; + // Users can enter and save a label made of only spaces e.g. for + // navigation links. In that case we provide a fallback label. + // We do want to trim leading and trailing spaces anyways. + const trimmedLabel = label.trim(); + return trimmedLabel === '' ? __( 'Untitled' ) : trimmedLabel; } const match = getActiveBlockVariation( blockName, attributes ); @@ -65,8 +69,8 @@ export default function useBlockDisplayTitle( { [ clientId, context ] ); - if ( ! blockTitle || blockTitle.trim() === '' ) { - return __( 'Untitled' ); + if ( ! blockTitle ) { + return null; } if ( From 147c89aae23bfa5820b4ac1d9b2024ca95ef887b Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Thu, 7 Mar 2024 10:29:02 +0100 Subject: [PATCH 5/9] Adjust test. --- test/e2e/specs/site-editor/navigation-editor.spec.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/e2e/specs/site-editor/navigation-editor.spec.js b/test/e2e/specs/site-editor/navigation-editor.spec.js index ca839405089816..5cb3560149381f 100644 --- a/test/e2e/specs/site-editor/navigation-editor.spec.js +++ b/test/e2e/specs/site-editor/navigation-editor.spec.js @@ -54,13 +54,20 @@ test.describe( 'Editing Navigation Menus', () => { await expect( listView ).toBeVisible(); const navBlockNode = listView.getByRole( 'link', { - name: 'Navigation (locked)', + name: 'Navigation', exact: true, } ); - // The Navigation block should be present and locked. + // The Navigation block should be present. await expect( navBlockNode ).toBeVisible(); + // The Navigation block description should contain the locked state information. + const navBlockNodeDescriptionId = + await navBlockNode.getAttribute( 'aria-describedby' ); + await expect( + listView.locator( `id=${ navBlockNodeDescriptionId }` ) + ).toHaveText( /This block is locked./ ); + // The block should have no actions menu. await expect( navBlockNode From 99edf894d3b9fb3faf800a9ba0de49ba51e05b82 Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Thu, 7 Mar 2024 15:11:25 +0100 Subject: [PATCH 6/9] Adjust block renaming test. --- test/e2e/specs/editor/various/block-renaming.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/specs/editor/various/block-renaming.spec.js b/test/e2e/specs/editor/various/block-renaming.spec.js index 0411d196fea4a8..93924faf84e26c 100644 --- a/test/e2e/specs/editor/various/block-renaming.spec.js +++ b/test/e2e/specs/editor/various/block-renaming.spec.js @@ -224,7 +224,7 @@ test.describe( 'Block Renaming', () => { // Trigger actions menu for the Heading (not the selected block). const headingItem = listView.getByRole( 'gridcell', { - name: 'Heading', + name: 'Untitled', } ); // The actions menu button is a sibling of the menu item gridcell. From 6f8821f744b95a500ef5c71983dbfe457630f288 Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Fri, 8 Mar 2024 12:59:30 +0100 Subject: [PATCH 7/9] Adjust list view test. --- test/e2e/specs/editor/various/list-view.spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/e2e/specs/editor/various/list-view.spec.js b/test/e2e/specs/editor/various/list-view.spec.js index ef26df266657a1..2f9574848dbfea 100644 --- a/test/e2e/specs/editor/various/list-view.spec.js +++ b/test/e2e/specs/editor/various/list-view.spec.js @@ -58,7 +58,7 @@ test.describe( 'List View', () => { exact: true, } ); const headingBlockItem = listView.getByRole( 'gridcell', { - name: 'Heading', + name: 'Untitled', exact: true, } ); @@ -697,7 +697,7 @@ test.describe( 'List View', () => { await page.keyboard.press( 'ArrowRight' ); // Move focus and select the Heading block. await listView - .getByRole( 'gridcell', { name: 'Heading', exact: true } ) + .getByRole( 'gridcell', { name: 'Untitled', exact: true } ) .dblclick(); // Select both inner blocks in the column. await page.keyboard.press( 'Shift+ArrowDown' ); @@ -795,7 +795,7 @@ test.describe( 'List View', () => { } ); // Click on the Heading block to select it. await listView - .getByRole( 'gridcell', { name: 'Heading', exact: true } ) + .getByRole( 'gridcell', { name: 'Untitled', exact: true } ) .click(); await listView .getByRole( 'gridcell', { name: 'File' } ) From c8384f7f8d48692ad15596336fdff9d69e0ec9a3 Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Mon, 18 Mar 2024 13:28:10 +0100 Subject: [PATCH 8/9] Revert changes to useBlockDisplayTitle. --- .../src/components/block-title/test/index.js | 18 ------------------ .../block-title/use-block-display-title.js | 7 +------ .../editor/various/block-renaming.spec.js | 2 +- .../e2e/specs/editor/various/list-view.spec.js | 6 +++--- 4 files changed, 5 insertions(+), 28 deletions(-) diff --git a/packages/block-editor/src/components/block-title/test/index.js b/packages/block-editor/src/components/block-title/test/index.js index edc073b52e9f66..8a4d3c2f52fd7e 100644 --- a/packages/block-editor/src/components/block-title/test/index.js +++ b/packages/block-editor/src/components/block-title/test/index.js @@ -19,9 +19,6 @@ const blockTypeMap = { 'name-with-label': { title: 'Block With Label' }, 'name-with-custom-label': { title: 'Block With Custom Label' }, 'name-with-long-label': { title: 'Block With Long Label' }, - 'name-with-only-spaces-label': { - title: 'Block With Label With Only Spaces', - }, 'reusable-block': { title: 'Reusable Block' }, }; @@ -30,7 +27,6 @@ const blockLabelMap = { 'Block With Long Label': 'This is a longer label than typical for blocks to have.', 'Block With Custom Label': 'A Custom Label like a Block Variation Label', - 'Block With Label With Only Spaces': ' ', 'Reusable Block': 'Reuse me!', }; @@ -101,20 +97,6 @@ describe( 'BlockTitle', () => { expect( screen.getByText( 'Test Label' ) ).toBeVisible(); } ); - it( 'renders "Untitled" if label is made of only spaces', () => { - useSelect.mockImplementation( ( mapSelect ) => - mapSelect( () => ( { - getBlockName: () => 'name-with-only-spaces-label', - getBlockType: ( name ) => blockTypeMap[ name ], - getBlockAttributes: () => null, - } ) ) - ); - - render( ); - - expect( screen.getByText( 'Untitled' ) ).toBeVisible(); - } ); - it( 'should prioritize reusable block title over title', () => { useSelect.mockImplementation( ( mapSelect ) => mapSelect( () => ( { diff --git a/packages/block-editor/src/components/block-title/use-block-display-title.js b/packages/block-editor/src/components/block-title/use-block-display-title.js index c92ee795f932fc..a51b336554a2a7 100644 --- a/packages/block-editor/src/components/block-title/use-block-display-title.js +++ b/packages/block-editor/src/components/block-title/use-block-display-title.js @@ -6,7 +6,6 @@ import { __experimentalGetBlockLabel as getBlockLabel, store as blocksStore, } from '@wordpress/blocks'; -import { __ } from '@wordpress/i18n'; /** * Internal dependencies @@ -55,11 +54,7 @@ export default function useBlockDisplayTitle( { const label = getBlockLabel( blockType, attributes, context ); // If the label is defined we prioritize it over a possible block variation title match. if ( label !== blockType.title ) { - // Users can enter and save a label made of only spaces e.g. for - // navigation links. In that case we provide a fallback label. - // We do want to trim leading and trailing spaces anyways. - const trimmedLabel = label.trim(); - return trimmedLabel === '' ? __( 'Untitled' ) : trimmedLabel; + return label; } const match = getActiveBlockVariation( blockName, attributes ); diff --git a/test/e2e/specs/editor/various/block-renaming.spec.js b/test/e2e/specs/editor/various/block-renaming.spec.js index 93924faf84e26c..0411d196fea4a8 100644 --- a/test/e2e/specs/editor/various/block-renaming.spec.js +++ b/test/e2e/specs/editor/various/block-renaming.spec.js @@ -224,7 +224,7 @@ test.describe( 'Block Renaming', () => { // Trigger actions menu for the Heading (not the selected block). const headingItem = listView.getByRole( 'gridcell', { - name: 'Untitled', + name: 'Heading', } ); // The actions menu button is a sibling of the menu item gridcell. diff --git a/test/e2e/specs/editor/various/list-view.spec.js b/test/e2e/specs/editor/various/list-view.spec.js index 2f9574848dbfea..ef26df266657a1 100644 --- a/test/e2e/specs/editor/various/list-view.spec.js +++ b/test/e2e/specs/editor/various/list-view.spec.js @@ -58,7 +58,7 @@ test.describe( 'List View', () => { exact: true, } ); const headingBlockItem = listView.getByRole( 'gridcell', { - name: 'Untitled', + name: 'Heading', exact: true, } ); @@ -697,7 +697,7 @@ test.describe( 'List View', () => { await page.keyboard.press( 'ArrowRight' ); // Move focus and select the Heading block. await listView - .getByRole( 'gridcell', { name: 'Untitled', exact: true } ) + .getByRole( 'gridcell', { name: 'Heading', exact: true } ) .dblclick(); // Select both inner blocks in the column. await page.keyboard.press( 'Shift+ArrowDown' ); @@ -795,7 +795,7 @@ test.describe( 'List View', () => { } ); // Click on the Heading block to select it. await listView - .getByRole( 'gridcell', { name: 'Untitled', exact: true } ) + .getByRole( 'gridcell', { name: 'Heading', exact: true } ) .click(); await listView .getByRole( 'gridcell', { name: 'File' } ) From 13b0fe794c2df41d7fd6145e843c34224e2240bd Mon Sep 17 00:00:00 2001 From: Andrea Fercia Date: Mon, 18 Mar 2024 13:30:39 +0100 Subject: [PATCH 9/9] Make heading with only spaces be considered empty. --- packages/block-library/src/heading/index.js | 2 +- packages/editor/src/components/document-outline/index.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/block-library/src/heading/index.js b/packages/block-library/src/heading/index.js index c137f38210bf8f..f43b75e5320b25 100644 --- 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 index 20410959a17210..0a9a6725c9e257 100644 --- 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,