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.

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

Try better block labels #18132

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Try better block labels #18132

merged 4 commits into from
Jan 10, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Oct 28, 2019

Description

Related #17519, #18014

Introduces a new __experimentalLabel( attributes, context ) function to the block api. The aim of this is to provide more context about a particular block which can be currently surfaced in two ways:

  1. Instead of displaying the Block Title in the Block Navigator, it displays some content from the block. This is currently being used by the Navigation Block to let users differentiate their nav links:
    Screen Shot 2019-12-11 at 3 38 30 pm

  2. Providing more contextual information to screen readers, this might be paragraph text or an image caption so that users can be more sure of the block content.
    Screen Shot 2019-12-11 at 4 09 18 pm

The new __experimentalLabel also replaces the mobile function __experimentalGetAccessibilityLabel and those labels now work across both mobile and web.

The blocks package now also has two new convenience functions for retrieving these labels:

  • __experimentalGetBlockLabel - returns just the output of the __experimentalLabel.
  • __experimentalGetAccessibleBlockLabel - returns some extra information useful to screen readers like the block title and the position in the block list.

Those functions will also strip any html (i.e. from formatting) from the labels.

How has this been tested?

Additional unit testing.

Manual testing:

  1. Add a Navigation Block (make sure it's enabled as an experimental feature).
  2. Ensure the navigation block has menu items.
  3. View the Block navigator.
  4. Observe the block labels for menu items
  5. Inspect the block mover for a menu item using dev tools
  6. Observe the screen reader text includes the new block label
  7. Inspect the menu item's block wrapper
  8. Observe the aria label includes the new block label

Other blocks worth testing for the aria-label - Paragraph, Heading, More, Image.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan self-assigned this Oct 28, 2019
@talldan talldan added [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Package] Blocks /packages/blocks [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Oct 28, 2019
@@ -33,6 +33,7 @@
"@wordpress/html-entities": "file:../html-entities",
"@wordpress/i18n": "file:../i18n",
"@wordpress/is-shallow-equal": "file:../is-shallow-equal",
"@wordpress/rich-text": "file:../rich-text",
Copy link
Contributor Author

@talldan talldan Oct 28, 2019

Choose a reason for hiding this comment

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

I imagine adding RichText as a dependency here is something to avoid. At the moment this is used to strip formatting/html from the attribute, but an alternative could be to add something to @wordpress/dom that does this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is definitely something that should happen. I consider it a blocker personally.

Copy link
Member

Choose a reason for hiding this comment

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

@ellatrix - can you help to extract the related logic from @wordpress/rich-text?

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've now added stripHTML to the dom package in 7bf4683, and I'm using that.

For the implementation of that function I used some stack overflow driven development:
https://stackoverflow.com/questions/822452/strip-html-from-text-javascript/47140708#47140708

As far as I can tell this works across platform. MDN has a ? against mobile safari in its browser support chart (https://developer.mozilla.org/en-US/docs/Web/API/DOMParser), but it worked in my simulator.

@mtias
Copy link
Member

mtias commented Oct 28, 2019

I don't think we need the "Menu Item" prefix there when we are already under a "Navigation Menu" block.

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 blockLabel = sprintf( __( 'Block: %s' ), __experimentalGetBlockLabel( blockType, attributes, ' - ' ) );
Copy link
Member

Choose a reason for hiding this comment

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

@hypest @koke @Tug - it didn't take a lot of time to see this experimental method useful in the web context as well :)

Copy link
Contributor

@pinarol pinarol Nov 8, 2019

Choose a reason for hiding this comment

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

Hmm it is a different method though. The one used on mobile is __experimentalGetAccessibilityLabel, input parameters and the content returned is different as well. __experimentalGetAccessibilityLabel is for constructing a custom description for the block to be read by VoiceOver. It can be a long description too. For example it reads the caption and the alt text for the image block. AFAIU __experimentalGetBlockLabel is used in places where a short description of block is needed.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great to have one method which would cover all those use cases through some sort of context. In your case it could be:

getBlockLabel( clientId, { context: 'screen-reader` } );

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a very good idea.

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, they're a bit different at the moment. I'll have a look into it and see what the options are for consolidating the two.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Image block and Paragraph block descriptions could be pretty long for accessibility labels, but I think the others could work pretty well for both accessible and visual block labels.

This is usually not a problem if there is no important information after that long text. So if block type, level, coordinates etc., all come first, and then the actual text is last, users can always interrupt speech once they know what that block is about. One could artificially limit that character count to 100 or 150, but that runs the potential of not being enough for some.

The friends at Gutenberg Mobile already do this block labeling in their navigation layer, at least for VoiceOver on iOS. Perhaps look at what they're doing and mirror that?

Copy link
Contributor Author

@talldan talldan Dec 4, 2019

Choose a reason for hiding this comment

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

I've consolidated the functionality now so that both mobile and web code uses __experimentalGetAccessibilityLabel and it should return the same format as the mobile function produced previously. I haven't tested it beyond unit tests for the function, though.

@pinarol Let me know what you think.

I think there's merit in what @gziolo proposes for the context of the labels. At the moment there's only the navigation link block that has a visual label, but others like the image, paragraph and heading block might require a slightly different label (or in some cases, no label at all).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarcoZehe Thanks for the feedback! For some reason I didn't see your message until after the one I posted above.

I've kept the format of the text broadly the same across web and mobile, it follows the pattern established on mobile. Having said that the labels for each block themselves still need to be implemented separately on web as they're in different files. I plan to look at some of this next.

Copy link
Contributor

Choose a reason for hiding this comment

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

@talldan this is looking good overall. But there's just one thing that sounds odd when VoiceOver is on and it should be fixed. If there's an unrecognized block the screen reader says 'Unrecognized block block', so there's an extra block at the end. I think it is related with the removal of this logic:

if ( name === 'core/missing' ) { // is the block unrecognized?	
			blockName = title;	
}

Missing blocks are already coming with 'Unrecognized Block' title.

The rest looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, thanks for spotting that @pinarol!

const richTextValue = create( { html: attributes[ displayNameAttribute ] } );
const formatlessDisplayName = getTextContent( richTextValue );

return `${ blockTitle }${ separator }${ formatlessDisplayName }`;
Copy link
Member

Choose a reason for hiding this comment

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

What if I wanted to avoid using the block title and separator format? Currently, we are working on some blocks that look like this:

Before:
Screen Shot 2019-10-28 at 6 34 54 PM

After:
Screen Shot 2019-10-28 at 6 34 10 PM

I think we'd like to just use "Header" instead of displaying "Template: Header". To us, the "Template" block (Which is dubiously named. One shouldn't trust what they think "template" means! 😆) is just a transparent wrapper around some other blocks and isn't really meant to be understood by or visible to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. A previous proposal has been to use a function or component to create the label, which is useful if the label is created from multiple attributes.

__experimentalDisplayName( { attributes, blockType } ) {
  return `${ blockType.title }: ${ attributes.label }`
}

My concern would be a lack of consistency across blocks. Also where this is used as an aria-label, the block type might make sense, whereas in the navigator it might not.

I suppose this could be customisable by getBlockLabel:

getBlockLabel( blockType, blockAttributes, { includeBlockTitle: false } );

So that aria-labels can include the block titles, but the navigator doesn't.

Copy link
Member

@gziolo gziolo Oct 29, 2019

Choose a reason for hiding this comment

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

How about the 3rd param is more general and contains properties like:

{
    context: 'block-navigation',
    format: 'concise',
}

Then each block's author could decide how to code the label depending on the properties passed. We could always provide a default handler which works fine in 90% of cases.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still doubtful of the use case in general. We are not going to be using "Menu Item: Label" for navigation, for example. If we'd also not use it for template parts, we might be preemptively overcomplicating things here.

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'm thinking the best approach might be to have two functions getAccessibleBlockLabel and getVisualBlockLabel. The former can include the block title for some extra context. Thoughts?

I've rebased and made the change described above, though it seems at the moment there's a bug with the nav block in master, so hard to test it.

@gziolo gziolo mentioned this pull request Oct 30, 2019
@talldan talldan mentioned this pull request Nov 8, 2019
5 tasks
@mtias
Copy link
Member

mtias commented Nov 8, 2019

For Navigation block I think we should keep it as it is, which is much easier to browse in the navigator.

Copy link
Contributor

@MarcoZehe MarcoZehe left a comment

Choose a reason for hiding this comment

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

Left a spelling error suggestion.

packages/block-editor/src/components/block-list/block.js Outdated Show resolved Hide resolved
packages/block-editor/src/components/block-mover/index.js Outdated Show resolved Hide resolved
packages/blocks/src/api/index.js Outdated Show resolved Hide resolved
packages/blocks/src/api/test/utils.js Outdated Show resolved Hide resolved
packages/blocks/src/api/test/utils.js Outdated Show resolved Hide resolved
packages/blocks/src/api/test/utils.js Outdated Show resolved Hide resolved
packages/blocks/src/api/test/utils.js Outdated Show resolved Hide resolved
packages/blocks/src/api/test/utils.js Outdated Show resolved Hide resolved
packages/blocks/src/api/test/utils.js Outdated Show resolved Hide resolved
packages/blocks/src/api/utils.js Outdated Show resolved Hide resolved
@talldan
Copy link
Contributor Author

talldan commented Dec 4, 2019

Thanks for spotting that @MarcoZehe!

@talldan talldan force-pushed the try/consistent-block-label branch from bfdf709 to 07e2ee0 Compare December 4, 2019 07:53
@talldan talldan added the [Status] In Progress Tracking issues with work in progress label Dec 5, 2019
@talldan talldan force-pushed the try/consistent-block-label branch from 07e2ee0 to 516c05c Compare December 11, 2019 07:24
@talldan talldan force-pushed the try/consistent-block-label branch from ed80aa9 to 40ae5cd Compare January 10, 2020 08:46
@draganescu draganescu merged commit a8f43fa into master Jan 10, 2020
@draganescu draganescu deleted the try/consistent-block-label branch January 10, 2020 10:22
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.

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

@@ -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


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.

*
* @return {string} The block label.
*/
export function getAccessibleBlockLabel( blockType, attributes, position, direction = 'vertical' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We've moving slowly away from functions that take blockType, and attributes in favor of the whole block object. I also feel like position and direction are not necessary and can be retrieved from state if this is becomes more a selector or a component instead of a global function. cc @aduth

Copy link
Member

Choose a reason for hiding this comment

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

We've moving slowly away from functions that take blockType, and attributes in favor of the whole block object.

Right. It already raises the question: What if I wanted the label to change depending on the inner blocks? How would that be implemented today with this interface?

Also, there's some inconsistency between this utility and the others in the file; where most all of the other utilities will accept either a block type (object) or a block name (string).

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 mentioned it above in another comment, but I'm now looking at moving this out of the blocks package into block-editor as selectors.

the api would be getAccessibleBlockLabel( clientId, direction ) and the selector implementation could be responsible for grabbing any other data.

Comment on lines +149 to +150
// Strip any HTML (i.e. RichText formatting) before returning.
return stripHTML( label );
Copy link
Member

Choose a reason for hiding this comment

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

Can't we make this the responsibility of the block implementation in generating the label? In other words, force the block to return a plain string. There, we already have some availability of the rich-text tooling. I see there was some prior discussion about rich-text being used here, but if we would put it on the block to return, then it doesn't need to be a dependency here.

I'm not sure there exists a utility already for generating a plain-text string from a RichText value, but it would seem to align with some of the existing utilities (toTree, toDOM, toHTMLString). Something like a toPlainString (cc @ellatrix).

I am a bit worried about introducing something like a stripHTML, both for these reasons above in that it ideally should not be necessary, but also because it could become a security weakness† or a source of poor performance.

† Not to say that it's vulnerable today in how it's used, notably because we're not dangerouslySetInnerHTML, but it's one of those things where we should try to avoid this altogether if we can, for fear of future mis-use, browser inconsistencies, etc.

Copy link
Contributor Author

@talldan talldan Jan 15, 2020

Choose a reason for hiding this comment

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

So I had some feedback that the __experimentalLabel api on the block needed some thought. It looks like the option is to split it into two separate things:

This for the accessibility label:

__experimentalGetAccessibilityLabel( attributes ) { return attributes.title }

and this for the visual label that might appear in the block navigator:

__experimentalLabel: 'title' // title is the name of the attribute

So it looks like the option of moving this responsibility to the block might not be possible for the latter. One option is something that @youknowriad mentioned in another comment, is that these could become selectors in the block-editor package. I started working on that, and I think that might be a good option.

Then I could change stripHTML to toPlainString (and move it to rich-text) as suggested.

*
* @return {string} The block label.
*/
export function getAccessibleBlockLabel( blockType, attributes, position, direction = 'vertical' ) {
Copy link
Member

Choose a reason for hiding this comment

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

We've moving slowly away from functions that take blockType, and attributes in favor of the whole block object.

Right. It already raises the question: What if I wanted the label to change depending on the inner blocks? How would that be implemented today with this interface?

Also, there's some inconsistency between this utility and the others in the file; where most all of the other utilities will accept either a block type (object) or a block name (string).

*/
export function getBlockLabel( blockType, attributes, context = 'visual' ) {
const {
__experimentalLabel: getLabel,
Copy link
Member

Choose a reason for hiding this comment

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

Was it intentional that this was called __experimentalLabel and not __experimentalGetLabel, as the deconstructed assignment would imply that its stable form might take?

I would expect either one of the following:

Suggested change
__experimentalLabel: getLabel,
__experimentalGetLabel: getLabel,
Suggested change
__experimentalLabel: getLabel,
__experimentalLabel: label,

While I'd grant that most functions of a block type don't have these sorts of prefixes ("get"), the reason for that as I'd see it is that they're all (to my knowledge) already in verb forms anyways: "save", "edit", etc.

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, I wasn't sure about this, thanks for clarifying.

*
* @param {Object} blockType The block type.
* @param {Object} attributes The values of the block's attributes.
* @param {Object} context The intended use for the label.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't an object. If we documented this as an union type of string literals, I think it would have helped clarify (both for us as the implementers and the future consumers) which sorts of values we're expecting to be supported.

Suggested change
* @param {Object} context The intended use for the label.
* @param {"visual"|"accessibility"} context The intended use for the label

It is a bit confusing for me though in how we expect block implementers to be handling this. Even in the implementations we've revised so far, we're only ever handling the "accessibility" case. So what's the point of "visual"? And how would we expect to treat the undefined return value from those implementations? Should have gone ahead and returned a value for the "visual" context for existing block revisions? (Getting a bit of YAGNI vibes here)

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'm now working on having two separate params in the block definition, so this won't be an issue any longer.

Copy link
Member

Choose a reason for hiding this comment

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

I'm now working on having two separate params in the block definition, so this won't be an issue any longer.

What about the second part of my comment, in which I question whether we even need two?

It is a bit confusing for me though in how we expect block implementers to be handling this. Even in the implementations we've revised so far, we're only ever handling the "accessibility" case. So what's the point of "visual"? And how would we expect to treat the undefined return value from those implementations? Should have gone ahead and returned a value for the "visual" context for existing block revisions? (Getting a bit of YAGNI vibes here)

@ellatrix
Copy link
Member

I'm not super fond of the new label on the block wrapper elements, since we'll eventually remove this wrapper element entirely. Additionally, this seems more useful in navigation mode, where it's not added at all.

@MarcoZehe
Copy link
Contributor

Yes, indeed these make the most sense on the buttons that get focus in navigation mode. Sorry I didn't catch that during my review. I believe moving these in a follow-up PR should be feasible, right?

@ellatrix
Copy link
Member

@MarcoZehe Yes, no worries, it's good to improve gradually. :)

@talldan
Copy link
Contributor Author

talldan commented Jan 15, 2020

@aduth @youknowriad @ellatrix @MarcoZehe I've addressed most of the remaining feedback in #19664. Thanks for the reviews and patience!

@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
@jorgefilipecosta
Copy link
Member

Hi @talldan, is this API ready to be stabilized in WordPress 5.4?

@jorgefilipecosta jorgefilipecosta added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 4, 2020
@talldan
Copy link
Contributor Author

talldan commented Feb 6, 2020

Hi @jorgefilipecosta, at the moment there's still some follow-up work to do on on these APIs, so I think they'll have to remain experimental for 5.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Blocks /packages/blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.