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

Jump to bottom

Try better block labels #18132

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Try better block labels #18132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14ca32d
Extract `getBlockDisplayName from block navigation list component and…
talldan Oct 28, 2019
58f90d0
Update expected label in e2e test
talldan Jan 10, 2020
97b2c8f
Update test func to be dynamic
talldan Jan 10, 2020
40ae5cd
Fix a comment typo that the speech synthesizer stumbled over.
MarcoZehe Jan 10, 2020
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
42 changes: 33 additions & 9 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import {
isReusableBlock,
isUnmodifiedDefaultBlock,
getUnregisteredTypeHandlerName,
__experimentalGetAccessibleBlockLabel as getAccessibleBlockLabel,
} from '@wordpress/blocks';
import { withFilters, Popover } from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';
import {
withDispatch,
withSelect,
Expand All @@ -50,6 +50,32 @@ import useMovingAnimation from './moving-animation';
import { ChildToolbar, ChildToolbarSlot } from './block-child-toolbar';
import { Context } from './root-container';

/**
* A debounced version of getAccessibleBlockLabel, avoids unnecessary updates to the aria-label attribute
* when typing in some blocks, like the paragraph.
*
* @param {Object} blockType The block type object representing the block's definition.
* @param {Object} attributes The block's attribute values.
* @param {number} index The index of the block in the block list.
* @param {string} moverDirection A string representing whether the movers are displayed vertically or horizontally.
* @param {number} delay The debounce delay.
*/
const useDebouncedAccessibleBlockLabel = ( blockType, attributes, index, moverDirection, delay ) => {
const [ blockLabel, setBlockLabel ] = useState( '' );

useEffect( () => {
const timeoutId = setTimeout( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a timeout?

Copy link
Member

Choose a reason for hiding this comment

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

Probably because of https://github.com/WordPress/gutenberg/pull/18132/files#r359241034

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this is being removed in #19597

setBlockLabel( getAccessibleBlockLabel( blockType, attributes, index + 1, moverDirection ) );
}, delay );

return () => {
clearTimeout( timeoutId );
};
}, [ blockType, attributes, index, moverDirection, delay ] );

return blockLabel;
};

function BlockListBlock( {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the most central components of our codebase and changes here can have big performance impacts, so, a sanity check (running performance tests) would be good (with and without the PR.

mode,
isFocusMode,
Expand All @@ -71,6 +97,7 @@ function BlockListBlock( {
isSelectionEnabled,
className,
name,
index,
isValid,
isLast,
attributes,
Expand Down Expand Up @@ -115,6 +142,9 @@ function BlockListBlock( {

const [ isToolbarForced, setIsToolbarForced ] = useState( false );

const blockType = getBlockType( name );
const blockAriaLabel = useDebouncedAccessibleBlockLabel( blockType, attributes, index, moverDirection, 400 );

// Handing the focus of the block on creation and update

/**
Expand Down Expand Up @@ -255,13 +285,6 @@ function BlockListBlock( {
{ bindGlobal: true, eventName: 'keydown', isDisabled: ! canFocusHiddenToolbar }
);

// Rendering the output
const blockType = getBlockType( name );
// translators: %s: Type of block (i.e. Text, Image etc)
const blockLabel = sprintf( __( 'Block: %s' ), blockType.title );
// The block as rendered in the editor is composed of general block UI
// (mover, toolbar, wrapper) and the display of the block content.

const isUnregisteredBlock = name === getUnregisteredTypeHandlerName();

// If the block is selected and we're typing the block should not appear.
Expand Down Expand Up @@ -382,7 +405,7 @@ function BlockListBlock( {
// Only allow shortcuts when a blocks is selected and not locked.
onKeyDown={ isSelected && ! isLocked ? onKeyDown : undefined }
tabIndex="0"
aria-label={ blockLabel }
aria-label={ blockAriaLabel }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might represent a bit of a performance issue for something like the paragraph block where the aria-label contains all the paragraph text. Typing in the block also updates the aria-label!

An option might be to debounce the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might represent a bit of a performance issue for something like the paragraph block where the aria-label contains all the paragraph text. Typing in the block also updates the aria-label!

How do the mobile friends do it? An option might be to update the paragraph text, for example, once editing mode is exited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling it's not as much of an issue on the mobile side as there's no document object model. I've updated the code now so that the function is debounced, hopefully that's an ok solution.

role="group"
{ ...wrapperProps }
style={
Expand Down Expand Up @@ -560,6 +583,7 @@ const applyWithSelect = withSelect(
hasFixedToolbar: hasFixedToolbar && isLargeViewport,
isLast: index === blockOrder.length - 1,
isNavigationMode: isNavigationMode(),
index,
Copy link
Contributor

Choose a reason for hiding this comment

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

We removed this explicitely some time ago for performance reasons. Adding this prop rerenders the block everytime a precedent block is removed in the list, this can be harmful for long posts. Also, We should favor the inner useSelect instead of adding new props to this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this is being removed in #19597

isRTL,

// Users of the editor.BlockListBlock filter used to be able to access the block prop
Expand Down
40 changes: 10 additions & 30 deletions packages/block-editor/src/components/block-list/block.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@ import { Component } from '@wordpress/element';
import { ToolbarButton, Toolbar } from '@wordpress/components';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose, withPreferredColorScheme } from '@wordpress/compose';
import { getBlockType, getUnregisteredTypeHandlerName } from '@wordpress/blocks';
import { __, sprintf } from '@wordpress/i18n';
import {
getBlockType,
getUnregisteredTypeHandlerName,
__experimentalGetAccessibleBlockLabel as getAccessibleBlockLabel,
} from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
Expand Down Expand Up @@ -77,31 +81,6 @@ class BlockListBlock extends Component {
);
}

getAccessibilityLabel() {
const { attributes, name, order, title, getAccessibilityLabelExtra } = this.props;

let blockName = '';

if ( name === 'core/missing' ) { // is the block unrecognized?
blockName = title;
} else {
blockName = sprintf(
/* translators: accessibility text. %s: block name. */
__( '%s Block' ),
title, //already localized
);
}

blockName += '. ' + sprintf( __( 'Row %d.' ), order + 1 );

if ( getAccessibilityLabelExtra ) {
const blockAccessibilityLabel = getAccessibilityLabelExtra( attributes );
blockName += blockAccessibilityLabel ? ' ' + blockAccessibilityLabel : '';
}

return blockName;
}

applySelectedBlockStyle() {
const {
hasChildren,
Expand Down Expand Up @@ -201,17 +180,20 @@ class BlockListBlock extends Component {

render() {
const {
attributes,
blockType,
clientId,
icon,
isSelected,
isValid,
order,
title,
showFloatingToolbar,
parentId,
isTouchable,
} = this.props;

const accessibilityLabel = this.getAccessibilityLabel();
const accessibilityLabel = getAccessibleBlockLabel( blockType, attributes, order + 1 );

return (
<>
Expand Down Expand Up @@ -277,7 +259,6 @@ export default compose( [
const blockType = getBlockType( name || 'core/missing' );
const title = blockType.title;
const icon = blockType.icon;
const getAccessibilityLabelExtra = blockType.__experimentalGetAccessibilityLabel;

const parents = getBlockParents( clientId, true );
const parentId = parents[ 0 ] || '';
Expand Down Expand Up @@ -319,7 +300,6 @@ export default compose( [
isLastBlock,
isSelected,
isValid,
getAccessibilityLabelExtra,
showFloatingToolbar,
parentId,
isParentSelected,
Expand Down
46 changes: 23 additions & 23 deletions packages/block-editor/src/components/block-mover/test/mover-description.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,113 +8,113 @@ describe( 'block mover', () => {
positiveDirection = 1;

describe( 'getBlockMoverDescription', () => {
const type = 'TestType';
const label = 'Header: Some Header Text';

it( 'generates a title for the first item moving up', () => {
expect( getBlockMoverDescription(
1,
type,
label,
0,
true,
false,
negativeDirection
) ).toBe(
`Block ${ type } is at the beginning of the content and can’t be moved up`
`Block ${ label } is at the beginning of the content and can’t be moved up`
);
} );

it( 'generates a title for the last item moving down', () => {
expect( getBlockMoverDescription(
1,
type,
label,
3,
false,
true,
positiveDirection
) ).toBe( `Block ${ type } is at the end of the content and can’t be moved down` );
positiveDirection,
) ).toBe( `Block ${ label } is at the end of the content and can’t be moved down` );
} );

it( 'generates a title for the second item moving up', () => {
expect( getBlockMoverDescription(
1,
type,
label,
1,
false,
false,
negativeDirection
) ).toBe( `Move ${ type } block from position 2 up to position 1` );
negativeDirection,
) ).toBe( `Move ${ label } block from position 2 up to position 1` );
} );

it( 'generates a title for the second item moving down', () => {
expect( getBlockMoverDescription(
1,
type,
label,
1,
false,
false,
positiveDirection
) ).toBe( `Move ${ type } block from position 2 down to position 3` );
positiveDirection,
) ).toBe( `Move ${ label } block from position 2 down to position 3` );
} );

it( 'generates a title for the only item in the list', () => {
expect( getBlockMoverDescription(
1,
type,
label,
0,
true,
true,
positiveDirection
) ).toBe( `Block ${ type } is the only block, and cannot be moved` );
positiveDirection,
) ).toBe( `Block ${ label } is the only block, and cannot be moved` );
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if it makes sense to also add tests for horizontal movers so they are properly labeled for columns moving left and right. Also, in light of the other discussion, it would make sense to update the movers text to speak of rows in these situations I think. But that should be dealt with in a follow-up after this lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's a good point. I've added some tests in #19549. I noticed while working on it that it also looks like the direction part of the description isn't internationalised. I've made an issue for that which should be an easy one for new contributors (#19550)

} );

it( 'indicates that the block can be moved left when the orientation is horizontal and the direction is negative', () => {
expect( getBlockMoverDescription(
1,
type,
label,
1,
false,
false,
negativeDirection,
'horizontal'
) ).toBe( `Move ${ type } block from position 2 left to position 1` );
) ).toBe( `Move ${ label } block from position 2 left to position 1` );
} );

it( 'indicates that the block can be moved right when the orientation is horizontal and the direction is positive', () => {
expect( getBlockMoverDescription(
1,
type,
label,
1,
false,
false,
positiveDirection,
'horizontal'
) ).toBe( `Move ${ type } block from position 2 right to position 3` );
) ).toBe( `Move ${ label } block from position 2 right to position 3` );
} );

it( 'indicates that the block cannot be moved left when the orientation is horizontal and the block is the first block', () => {
expect( getBlockMoverDescription(
1,
type,
label,
0,
true,
false,
negativeDirection,
'horizontal'
) ).toBe(
`Block ${ type } is at the beginning of the content and can’t be moved left`
`Block ${ label } is at the beginning of the content and can’t be moved left`
);
} );

it( 'indicates that the block cannot be moved right when the orientation is horizontal and the block is the last block', () => {
expect( getBlockMoverDescription(
1,
type,
label,
3,
false,
true,
positiveDirection,
'horizontal'
) ).toBe( `Block ${ type } is at the end of the content and can’t be moved right` );
) ).toBe( `Block ${ label } is at the end of the content and can’t be moved right` );
} );
} );

Expand Down
30 changes: 5 additions & 25 deletions packages/block-editor/src/components/block-navigation/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,38 +8,18 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { Button } from '@wordpress/components';
import { getBlockType } from '@wordpress/blocks';
import {
__experimentalGetBlockLabel as getBlockLabel,
getBlockType,
} from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';
import { create, getTextContent } from '@wordpress/rich-text';

/**
* Internal dependencies
*/
import BlockIcon from '../block-icon';
import ButtonBlockAppender from '../button-block-appender';

/**
* Get the block display name, if it has one, or the block title if it doesn't.
*
* @param {Object} blockType The block type.
* @param {Object} attributes The values of the block's attributes
*
* @return {string} The display name value.
*/
function getBlockDisplayName( blockType, attributes ) {
const displayNameAttribute = blockType.__experimentalDisplayName;

if ( ! displayNameAttribute || ! attributes[ displayNameAttribute ] ) {
return blockType.title;
}

// Strip any formatting.
const richTextValue = create( { html: attributes[ displayNameAttribute ] } );
const formatlessDisplayName = getTextContent( richTextValue );

return formatlessDisplayName;
}

export default function BlockNavigationList( {
blocks,
selectedBlockClientId,
Expand Down Expand Up @@ -73,7 +53,7 @@ export default function BlockNavigationList( {
onClick={ () => selectBlock( block.clientId ) }
>
<BlockIcon icon={ blockType.icon } showColors />
{ getBlockDisplayName( blockType, block.attributes ) }
{ getBlockLabel( blockType, block.attributes ) }
{ isSelected && <span className="screen-reader-text">{ __( '(selected block)' ) }</span> }
</Button>
</div>
Expand Down
Loading