Gutenberg: Switch screen-reader-text to use VisuallyHidden

[mkaz]

Describe the Issue

This issue is to gather all the spots for changing spots using the screen-reader-text classname to using the VisuallyHidden component. Per comment in #18127

A good reference implementation is PR #18165 that converts within the components package the spots that use screen-reader-text to VisuallyHidden component.

Why the switch?

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.

File list with screen-reader-text

• [ ] packages/block-library/src/categories/edit.js
• [ ] packages/block-library/src/archives/index.php
• [ ] packages/block-library/src/gallery/edit.js
• [ ] packages/components/src/button/style.scss
• [ ] packages/editor/src/components/post-visibility/index.js
• [ ] packages/editor/src/components/post-title/index.js
• [ ] packages/editor/src/components/post-text-editor/index.js
• [ ] packages/editor/src/components/post-preview-button/index.js
• [ ] packages/block-editor/src/components/button-block-appender/index.js
• [ ] packages/e2e-tests/specs/editor/various/taxonomies.test.js
• [ ] packages/block-editor/src/components/inserter/menu.js
• [ ] packages/block-editor/src/components/block-navigation/list.js
• [ ] packages/edit-post/src/components/admin-notices/test/index.js
• [ ] packages/editor/src/components/post-preview-button/test/__snapshots__/index.js.snap

@youknowriad - just wanted to confirm that you do what this updated across all of the above?

[youknowriad]

@mkaz yes :)

[mkaz]

I've added this as a Good First Issue and switched the list to checkboxes. If you would like to contribute feel free to take any items above as separate PRs, everything doesn't have to be done in a single PR if that makes it easier to develop and test.

[MarkMarzeotti]

I am working on this issue at Contributor Day. I am focused on the three files in block-library.

packages/block-library/src/categories/edit.js
packages/block-library/src/archives/index.php
packages/block-library/src/gallery/edit.js

[adamziel]

@mkaz I was looking into this one, and encountered more than one situation like this:

return (
    <div className={classnames( {
        'screen-reader-text': ! isSelected && RichText.isEmpty( caption ),
    } )}>
        { caption }
    </div>
);

In which case we need some additional boilerplate code:

const shouldHideCaption = ! isSelected && RichText.isEmpty( caption );
const CaptionComponent = shouldHideCaption ? VisuallyHidden : RichText;
const captionProps = shouldHideCaption ? { as: RichText } : {};
return (
    <CaptionComponent { ...captionProps }>{ caption }</CaptionComponent>
);

I feel like a better solution would be supporting an additional prop that we could use like this:

<VisuallyHidden
     as={ RichText }
     hidden={ ! isSelected && RichText.isEmpty( caption ) }
>
    { caption }
</VisuallyHidden>

What do you think?

[mkaz]

I think the logic would be better outside the wrapper.

{ ! isSelected && RichText.isEmpty( caption ) 
   ? <VisuallyHidden>{ caption } </VisuallyHidden>
  : <div>{ caption }</div>
}

Also, I don't quite follow that logic, if the caption is empty, then what is being shown in the tag?

[adamziel]

@mkaz I think I oversimplified my example. Let's take a look at the actual excerpt from gallery.js here:

const captionClassNames = classnames( 'blocks-gallery-caption', {
    'screen-reader-text': ! isSelected && RichText.isEmpty( caption ),
});
return (
    // ...other stuff
    <RichText
        tagName="figcaption"
        className={ captionClassNames }
        placeholder={ __( 'Write gallery caption…' ) }
        value={ caption }
        unstableOnFocus={ onFocusGalleryCaption }
        onChange={ ( value ) => setAttributes( { caption: value } ) }
        inlineToolbar
        />
)

So we're not talking about just an empty <div />, but a component which does something even when the caption is empty. In particular, it provides a visually hidden label for screen readers. In this case, using a ternary would still require assigning props to a variable before returning:

const richTextProps = {
    tagName: "figcaption",
    className: 'blocks-gallery-caption',
    placeholder:  __( 'Write gallery caption…' ),
    value: caption,
    unstableOnFocus: onFocusGalleryCaption,
    onChange: ( value ) => setAttributes( { caption: value } ),
    inlineToolbar: true
};

return (
    // ... other stuff ...
    ! isSelected && RichText.isEmpty( caption )
        ? <VisuallyHidden as={ RichText } { ...richTextProps } />
        : <RichText { ...richTextProps } />
)

In which case we'd need to address one more issue: blocks-gallery-caption className would override the one provided by VisuallyHidden. A small update would be required to make sure VisuallyHidden glues blocks-gallery-caption on top of any className it receives.

One more idea that wouldn't involve any changes to VisuallyHidden would be:

const richTextElem = (
    <RichText
        tagName="figcaption"
        className={ captionClassNames }
        placeholder={ __( 'Write gallery caption…' ) }
        value={ caption }
        unstableOnFocus={ onFocusGalleryCaption }
        onChange={ ( value ) => setAttributes( { caption: value } ) }
        inlineToolbar
        />
);

return (
    // ...other stuff
    ! isSelected && RichText.isEmpty( caption ) 
        ? <VisuallyHidden> { richTextElem } </VisuallyHidden>
        : { richTextElem }
)

[adamziel]

@mkaz let's continue the conversation in the draft PR here: https://github.com/WordPress/gutenberg/pull/20607

[mkaz]

I think the final option will work, I'll take a look at the PR.

My concern was around adding a prop called "hidden" to a component called VisuallyHidden, it would be confusing. I think a goal would be trying to keep the component relatively simple.

make sure VisuallyHidden glues blocks-gallery-caption on top of any className it receives.

I remember considering this, attaching passed classnames, but I think it might conflict with what VisuallyHidden is trying to do. For example, if a passed class sets a display: inline-block or other similar value it could overwrite the components styles. I think my thought was that any additional class shouldn't be needed since the item is going to be hidden, we don't need styling from anywhere else.