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.

Sign up for GitHub

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

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

Merged
merged 9 commits into from
Mar 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f145e76
Fix labeling of the navigation links in the site editor list view.
afercia Feb 26, 2024
8d94b33
Adjust tests.
afercia Feb 27, 2024
7dcc508
Simplify labeling by removing aria-label and using text content.
afercia Mar 4, 2024
6060b93
Improve logic and add test.
afercia Mar 6, 2024
147c89a
Adjust test.
afercia Mar 7, 2024
99edf89
Adjust block renaming test.
afercia Mar 7, 2024
6f8821f
Adjust list view test.
afercia Mar 8, 2024
c8384f7
Revert changes to useBlockDisplayTitle.
afercia Mar 18, 2024
13b0fe7
Make heading with only spaces be considered empty.
afercia Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/block-editor/src/components/list-view/block-select-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ function ListViewBlockSelectButton(
onDragEnd,
draggable,
isExpanded,
ariaLabel,
ariaDescribedBy,
updateFocusAndSelection,
},
Expand Down Expand Up @@ -249,7 +248,6 @@ function ListViewBlockSelectButton(
onDragEnd={ onDragEnd }
draggable={ draggable }
href={ `#block-${ clientId }` }
aria-label={ ariaLabel }
aria-describedby={ ariaDescribedBy }
aria-expanded={ isExpanded }
>
Expand Down
33 changes: 12 additions & 21 deletions packages/block-editor/src/components/list-view/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand All @@ -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';
Expand Down Expand Up @@ -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 ) => {
Expand All @@ -93,6 +95,7 @@ function ListViewBlock( {
},
[ clientId ]
);

const allowRightClickOverrides = useSelect(
( select ) =>
select( blockEditorStore ).getSettings().allowRightClickOverrides,
Expand All @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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>
) }
Expand Down Expand Up @@ -405,7 +396,7 @@ function ListViewBlock( {
clientIds={ dropdownClientIds }
block={ block }
icon={ moreVertical }
label={ settingsAriaLabel }
Copy link
Member

@richtabor richtabor Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had previously inquired about simplifying the "Options for %s" label, but was informed the block name was necessary by @alexstine. Is that no longer the case?

Either way though, this should be renamed "Options", like it's called in the block toolbar (as they're the same).

@afercia @Mamaduka Was there a reason to change "Options" to "Actions"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richtabor I guess it was a battle I was going to lose eventually. It was really a stopgap to make NVDA behave as sometimes navigating the rows on the right side would not tell you which block you were opening an options menu for.

label={ __( 'Actions' ) }
popoverProps={ {
anchor: settingsPopoverAnchor, // Used to position the settings at the cursor on right-click.
} }
Expand Down
5 changes: 4 additions & 1 deletion packages/block-editor/src/components/list-view/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 2 additions & 1 deletion packages/editor/src/components/document-outline/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
102 changes: 51 additions & 51 deletions test/e2e/specs/editor/blocks/navigation-list-view.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
} );

Expand Down Expand Up @@ -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();
} );

Expand All @@ -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();
} );

Expand All @@ -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.
Expand Down Expand Up @@ -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();
} );

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

Expand All @@ -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();
} );

Expand Down
Loading
Loading