Create our own VisuallyHidden component to use for screen-reader text

[tofumatt]

Feature Description

Currently we use <span className="screen-reader-text"> across our codebase to visually hide screen-reader text in the app. This is a fine approach, but as pointed out in the IB for #2093, it'd be nice to have a dedicated and more-descriptive VisuallyHidden component we can use to put screen-reader text inside without having to memorise CSS class names, etc.

It was suggested we use Gutenberg's VisuallyHidden component, but as URL_FORTHCOMING shows, its defaults don't suit us. It'd be better to create our own, simple, VisuallyHidden component that looks like this:

import classnames from 'classnames';

const VisuallyHidden = ( { className, children, …otherProps } ) => {
  return <span { …otherProps } className={ classnames( 'screen-reader-text', className ) }>{ children }</span>;
}

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• A VisuallyHidden component should be made available to developers for screen-reader text.
• All <span className="screen-reader-text"> instances should be replaced with <VisuallyHidden>.

Implementation Brief

• Create a VisuallyHidden component (as outlined above) in assets/js/components/VisuallyHidden.js
• Replace all instances of <span className="screen-reader-text"> with the new <VisuallyHidden> component.
  Note that instances of <span class="screen-reader-text"> will be able to be removed; there are instances of using the <span> tags directly inside translation strings (eg

  site-kit-wp/assets/js/components/setup/compatibility-checks.js

  Lines 123 to 149 in d194354

  	if ( ! installed && installURL ) {
  	return {
  	labelHTML: __( 'Install<span class="screen-reader-text"> the helper plugin</span>', 'google-site-kit' ),
  	href: installURL,
  	external: false,
  	};
  	}
  	if ( installed && ! active && activateURL ) {
  	return {
  	labelHTML: __( 'Activate<span class="screen-reader-text"> the helper plugin</span>', 'google-site-kit' ),
  	href: activateURL,
  	external: false,
  	};
  	}
  	if ( installed && active && configureURL ) {
  	return {
  	labelHTML: __( 'Configure<span class="screen-reader-text"> the helper plugin</span>', 'google-site-kit' ),
  	href: configureURL,
  	external: false,
  	};
  	}
  	return {
  	labelHTML: __( 'Learn how<span class="screen-reader-text"> to install and use the helper plugin</span>', 'google-site-kit' ),
  	href: 'https://sitekit.withgoogle.com/documentation/using-site-kit-on-a-staging-environment/',
  	external: true,
  	};
  	}

  ) and they must remain. Those will be changed in Convert all HTML tags in translation strings to use createInterpolateElement #2216
• Add a new story to Storybook in stories/visually-hidden.stories.js under storiesOf( 'Global', module ).add( 'Visually hidden', () => {} ); that includes the VisuallyHidden component.

Test Coverage

• Add a new visual regression test (VRT) that ensures the VisuallyHidden component is always hidden, and possibly a test where it is hidden next to other non-hidden content.

Visual Regression Changes

• No visual changes should occur as a result of these changes.

QA Brief

• Ensure all existing screen reader text is still hidden.

Changelog entry

• Simplify adding strings only visible to screen reader users by implementing a VisuallyHidden component.

[aaemnnosttv]

@tofumatt if we aren't going to use the component from Gutenberg, why not use @reach/visually-hidden ? Not sure it makes sense to implement our own component to do something that we can just pull in, especially from a library which is specifically designed for accessibility.

Aside from that, it doesn't rely on a specific class name which I think makes more sense in our case since the component may be used on both front (at least admin bar) and backend.

Is there a better reason to create our own here over an existing option?

Edit it's also worth linking the Gutenberg PR where their component was introduced here as there is quite a bit of conversation there which may be useful to consider.

[tofumatt]

No major opposition from me to using Reach, but our (current) needs are quite spartan and our component, as proposed, would be lighter-weight than theirs. They use the style attribute directly for every instance of the component, which adds to HTML bulk and means reference usage for things like '<span class="screen-reader-text">Hello</span>' isn't as straightforward.

As mentioned: we need to use raw spans inside i18n strings (which we do have to do in several places), so having our VisuallyHidden component share class names and serve as a reference, all while being lighter-weight without adding to dependencies, seems like a better solution here.

I'm usually for adding dependencies to reduce our maintenance burden, especially for a11y concerns where we might miss things, but in this case I think a home-grown solution is better.

[aaemnnosttv]

I agree that the inline style is less efficient, but it's also "batteries included" 😄

Regarding the use in translatable strings, I don't think we should "mirror" the HTML of any components in these strings, but we didn't really have much options in the past.

Now we have createInterpolateElement which is ideal for this kind of thing I think.

We're already using it in a few places now:

site-kit-wp/assets/js/components/settings/module-setup-incomplete.js

Lines 44 to 59 in 6a69f0d

	{ createInterpolateElement(
	sprintf(
	/* translators: %s: link with next step */
	__( 'Setup incomplete: %s', 'google-site-kit' ),
	`<a>${ __( 'continue module setup', 'google-site-kit' ) }</a>`
	),
	{
	a: <Link
	className="googlesitekit-settings-module__edit-button"
	onClick={ () => {
	global.location = getReAuthURL( slug, true );
	} }
	inherit
	/>,
	}
	) }

Which ever component we use I think part of this issue should be going through and updating the existing screen reader text bits to use the real component, using createInterpolateElement where necessary.

I could go either way on which component we use; both have their pros and cons. I think the more valuable piece of this issue is to use a dedicated component instead, and use it everywhere it's needed 🙂

I still kind of think the Reach component is nice since it isn't that much added markup and should be up-to-date with accessibility best practices, but I'm open to keeping it closer to what we have now if preferred.

Thoughts @felixarntz ?

[tofumatt]

Yeah, our existing span class="screen-reader-text" components use WordPress styling, so are "batteries-included" too, but I getcha. Visually hidden spans are less finicky and need less maintenance than other components, but all your points are valid.

Using createInterpolateElement would be great, but I figured would be a separate issue from here as we need to do it for links in the app as well. Fair point about replacing all the spans in this issue though, I like it 👍🏻

[felixarntz]

@aaemnnosttv @tofumatt I think coding our own simple VisuallyHidden component makes sense. I wonder whether we should call it ScreenReaderText to clarify its intent better - WP people would immediately understand :)

Regarding the use of createInterpolateElement, I agree it might make sense to update existing translation strings and references to span class="screen-reader-text", however this could indeed be a separate issue as a follow-up.

[tofumatt]

I find "Visually hidden" to be a better description of what we're doing—the aim is to have that text there semantically, it's for more than just screen readers. Plus: WP itself uses VisuallyHidden 😄

I've created a new issue for the createInterpolateElement change: #2216.

[aaemnnosttv]

I agree with @tofumatt – FWIW, the component introduced in Gutenberg was originally ScreenReaderText and they changed it to VisuallyHidden for consistency with other UI libraries 😄 . It's also worth noting that the core component doesn't rely on the screen-reader-text class so that it works in a standalone/non-WP context (like Storybook).

Since we're not publishing the component for use outside of WP, I think it's okay to rely on that class for our needs, although if we ever decide to change it, that would really be an implementation detail and the usage wouldn't need to change.

@tofumatt as silly as it sounds, we should probably introduce a story for this component. We probably don't need additional VRT for it, since many other scenarios will use components that have it, but it might not be a bad idea to add a VRT test for this component specifically. What do you think?

[tofumatt]

Adding a Storybook component makes sense, and even a VRT. Especially since we're relying on a third-party (WordPress) style to provide this class, we should make sure it's always hidden 😄

I've added it to the IB 👍🏻

[aaemnnosttv]

IB ✅

[caseymorrisus]

QA ✅