Fix/screen reader text

[adamziel]

Description

This PR solves #18167

See #18022 which introduced the VisuallyHidden component. The usage of screen-reader-text assumes a WordPress context with the proper CSS already defined. This may not always be the case, especially when pieces of the Gutenberg project are used elsewhere.

How has this been tested?

Tested locally. This PR touches a number of blocks and components. For each of them, you need to check if the new <VisuallyHidden> part behaves in the same way as the old .screen-reader-text. For example, this PR touches a gallery block caption:

https://github.com/WordPress/gutenberg/pull/20607/files#diff-e25f8ba6320e6c1650e6ec1c0ae5ba34L43-L102

To test it, let's follow some steps:

1. Create a new gallery block
2. Confirm the caption is hidden when it's empty and the block is not selected
3. Confirm the caption is visible then it's empty and the block is selected
4. Confirm the caption is visible when it's not empty and the block is not selected
5. Confirm the caption is visible then it's not empty and the block is selected

Without this PR applied it should use CSS class screen-reader-text (gif)

With this PR applied it should use the new VisuallyHidden wrapper (gif)

Types of changes

Non-breaking change which fixes #18167

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]

It looks like this can be defined this as:

<VisuallyHidden as="label" htmlFor={ `block-editor-inserter__search-${ instanceId }` }>
    { __( 'Search for a block' ) }
</VisuallyHidden>

which avoids the extra div around the label. I saw a few other places in the PR where that would be beneficial.

I noticed the VisuallyHidden docs don't mention this, could be a good follow up PR to update those docs 😄

[tellthemachines]

Thanks for taking this on! I left a bunch of comments below to indicate places where it's particularly important to preserve existing semantics, but the same recommendation applies to practically all of the instances where<VisuallyHidden> is being introduced: unless the existing element to be hidden is a <div>, we should always use the as prop to make <VisuallyHidden> itself render the semantically appropriate element, instead of wrapping the existing markup with it.

The readme for VisuallyHidden doesn't really explain its correct usage, but the Storybook demo may be helpful (you might have to use devtools to inspect the generated markup though, as some of it is hidden 😄 )

[tellthemachines]

To avoid wrapping the label in an unnecessary <div>, we should instead replace the label with <VisuallyHidden as="label" htmlFor={...} />

[adamziel]

Is extra <div> a bad thing though? I'd say a clear distinction between <VisuallyHidden> and a <label> is more readable.

[tellthemachines]

We might have to work on the docs, but it should be clear enough that <VisuallyHidden> will render as whatever element it's passed with the as prop 😅
As a general principle though, adding extra markup for code readability is not advisable because it has a direct impact on performance for the end user. It's best to keep markup to a minimum and use spacing and (non-rendering) comments where needed for clarification.

[tellthemachines]

We need <VisuallyHidden as="legend"> here. A <legend> must be the first direct descendant of a <fieldset>, so we can't have any intermediate wrappers.

[tellthemachines]

This doesn't feel right; if we need these styles here we should be applying them with whatever classname we're currently using to hide stuff from vision. Or we should be using <VisuallyHidden> if possible. Probably something to tackle in a follow-up PR though 😅. I don't think ResponsiveBlockControl is being used anywhere atm.

[tellthemachines]

Unrelated to this changeset, but these snapshots are not very readable. I wonder why all the spacing has been removed 🤔

[tellthemachines]

This presents a problem, because <figcaption> has to be a direct descendant of <figure>. I suspect the best way forward here will be to just pass the components-visually-hidden class to RichText directly. We have to support direct use of the class anyway because of the php files 🤷‍♀

[talldan]

It's a bit mad, but this should work:

<VisuallyHidden as={ RichText } tagName="figcaption" // other RichText props ... />

It might be cleanest to extract the logic into a separate component so that the props can be spread onto either RichText or VisuallyHidden.

But looking at the VisuallyHidden implementation, it doesn't look like it accepts additional class names. I may knock together a quick PR to fix that.

[talldan]

PR to better handle class names in VisuallyHidden - #20649

[gziolo]

Yes, let's fix it :)

In general, you should be able to pass anything you want as as and all props should be passed down.

[adamziel]

@talldan I think that makes sense! I added a helper component inline since it's pretty specific to this file, and I prepared a more general helper in #20928

[tellthemachines]

This change and the one above will break form semantics; best leave things as they were.

[tellthemachines]

While <div> and <span> have no semantic meaning, they do have user agent styling differences, so best not change this.

[adamziel]

Does the user agent styling matters if <VisuallyHidden> is always visually hidden?

[tellthemachines]

In practice it won't make a difference because the styles for hiding position the element absolutely, so it's removed from the flow, and it doesn't matter if its display type is block or inline.
But there's no good reason for making this change either, so best keep the changeset to what is strictly necessary 🙂

[tellthemachines]

Same as above.

[talldan]

There might be some issues using component class name in this way, since a component's class name isn't meant to be used outside of the component.

This html is also going to be output to a WordPress site's frontend, so would the components styles be loaded? Not sure, I'd need to test it. 😄

It might be preferable for it to continue using the screen-reader-text global style or for the block to have its own visually hidden style.

It may also be an idea to split some of the changes that need more discussion into a separate PR so that they don't hold up the other changes.

[gziolo]

It might be preferable for it to continue using the screen-reader-text global style or for the block to have its own visually hidden style.

Yes, correct. PHP part should be left as is.

[gziolo]

gutenberg/packages/components/src/visually-hidden/index.js

Lines 10 to 15 in 81d8afc

	function VisuallyHidden( { as = 'div', ...props } ) {
	return renderAsRenderProps( {
	as,
	className: 'components-visually-hidden',
	...props,
	} );

VisuallyHidden it's a simple wrapper that defaults to div. All existing divs should be replaced with VisuallyHidden when they use the WP version of the class name.

In case of any other tag name or component is used, you should pass it as as prop.

In the case of PHP implementation, it should use the class name that WordPress core supports to ensure it works properly on the frontend.

[adamziel]

I think #20928 would be a better solution here

[gziolo]

Keep in mind that you still need to apply a different class name when it's visually hidden so there is going to be some extra logic involved.

[adamziel]

@talldan @mkaz @gziolo @tellthemachines I addressed your feedback and this one is ready for re-review :-)

[tellthemachines]

Couple of minor comments below; looks good overall 🙂

[tellthemachines]

Wouldn't the className be passed in anyway as part of richTextProps, or am I missing something?

[adamziel]

I wanted to avoid passing className to <VisuallyHidden />, but when I looked into it more .block-gallery-caption shouldn't interfere with any styles we have there so I'll simplify this.

[tellthemachines]

If we're keeping the original classname as per this thead then this needn't change either.

[gziolo]

@tellthemachines, can you explain why we need still to render RichText when it's hidden visually? I see you introduced this behavior in #17101. In practice, it will never get focused because as soon as you move the focus to the Gallery block, then this condition will evaluate to false because isSelected will become true and the placeholder will show up when the caption is empty.

[tellthemachines]

Yup, I did that so it'll show up on the list of form controls for screen reader users. So if you're using a screen reader, you can navigate straight to the caption element through the forms list, even if the gallery block isn't focused. Of course, as soon as you navigate to the element it becomes focused and visible.

I notice that this is inconsistent with how we're treating the citation in the Quote block (which disappears from the DOM when empty not focused) and how we treat empty Paragraph blocks (which have visible placeholder text at all times). I'm not 100% sure which of these solutions is best accessibility and usability-wise. Perhaps this could be discussed further in the weekly accessibility chat? It would be best to have a consistent behaviour for all these cases.

[gziolo]

It's definitely something worth discussing with the bigger group 👍

It would be nice to be more consistent if other blocks always show placeholders then maybe we should follow that as well. However, I'm sure it's something that always has trade-offs. In the case of Paragraph without the placeholder, you could miss the fact that the block is even there when using mouse navigation :) On the other hand, if you would display caption placeholders for all images in the Gallery block then it would be very loaded. I think it also translates to the screen read case, too many controls exposed might make it harder to navigate to the currently selected block.

[tellthemachines]

Update: a similar issue is being discussed in #15308 and seems to be going in the direction of making placeholders visible by default.

[gziolo]

Nice work @adamziel. I don't want to disturb the process for this PR, I think it's ready to go.

We should clarify the issue with the wrapper for VisuallyHidden separately. I'd like to learn first if it is really necessary or we can simplify the code instead.