[a11y] Adding a screen reader live region and announcements for actions #320

cee-chen · 2019-07-09T19:37:37Z

Description

This is an an accessibility PR that provides more context and information to screen reader users. The goal is to announce the results/consequences of certain actions being taken to users, so that visually impaired users aren't left wondering what happened after pressing a button.

For example:

After clicking the "+ More" button in the filters sidebar, a screen reader user should know how many total filters are now being shown. (This was originally spiked out here).
After pressing the search button, a screen reader user should know how many results are being shown and how many total results were returned.
After navigating to another page of results, a screen reader user should have the results/paging info read out to them (same as above)
After changing how many results are being shown per-page, a screen reader user should have the results/paging info read out to them (same as above)

Screencaps

Search results announcements:

+ More Filters annoucements:

List of changes

I recommend following along commit-by-commit if possible!

Adds a new ScreenReaderStatus render prop helper
- This creates a one-time live region in the app, and reads out any updates in that region to screen readers. This region can be used by multiple different components.
- Note: Because this directly modifies the client DOM, a check needs to be added to this helper to ensure it doesn't crash for SSR React apps.
Updates MultiCheckboxFacet's "+ More" button to announce shown filters to ScreenReaderStatus
Updates PagingInfo to announce changes in results shown to ScreenReaderStatus

QA

Tested on MacOS Voiceover

(Note - uncovered lines are same as before, and all coverage %s went up slightly)

Future

I'll continue to investigate other opportunities to use this new live region (the next likely being the SingleLinksFacet component), but for now I'd like to keep this PR fairly small and to the two lowest hanging fruit for screen reader announcements.

cee-chen · 2019-07-09T23:30:28Z

packages/react-search-ui-views/src/PagingInfo.js

@@ -1,17 +1,27 @@
 import PropTypes from "prop-types";
 import React from "react";

-import { appendClassName } from "./view-helpers";
+import { appendClassName, ScreenReaderStatus } from "./view-helpers";


One potential downside to adding this to the <PagingInfo /> component is that if users roll their own view, they're inadvertently making their app less accessible - so it's possible that adding this to the react-search-ui container may be a bit more robust. Definitely open to thoughts on this!

In this project it's often a bit fuzzy to me what should go into a container and what should go into a view.

The guidance we give users for customizing views is to use the default view as a base and customize from there. That could be a bit of a foot gun if they forget to call "announceToScreenReader", as you point out.

The goal of having swappable views, is to give users a very easy way to use custom markup for a component. The less they have to reproduce in the view, the better. To that end, it would seem more ideal to do this in a container. We could either do it completely transparently, or we could roll all of the logic into a single prop that we pass down.

function PagingInfo({ className, end, searchTerm, start, totalResults, screenReaderAnnouncement }) { end = Math.min(end, totalResults); return ( <> <div className={appendClassName("sui-paging-info", className)}> Showing{" "} <strong> {start} - {end} </strong>{" "} out of <strong>{totalResults}</strong> for: <em>{searchTerm}</em> </div> {screenReaderAnnouncement(someCustomMessageFunction)} </> ); }

It could be something like, if you don't call it at all, the default announcement will be used. if you call it with a custom function, screenReaderAnnouncement(), then that overrides the default message with a custom message.

I also envisioned a future state where users could use react-search-ui without including react-search-ui-views at all, so they could have a completely custom view layer. Having this be part of the container would make that a challenge 🤔.

cee-chen · 2019-07-09T23:33:40Z

packages/react-search-ui-views/src/MultiCheckboxFacet.js

+              onClick={() => {
+                onMoreClick();
+
+                const newLimit = options.length + 10;


This is a bit of a magic constant (from the parent Facet.js container) - feel free to shout if you think it should be converted to a prop or something a bit less static

JasonStoltz

This works great, I just have a whole bunch of larger architectural questions about how we best make this fit into Search UI. I left some open ended thoughts. Hoping we can put some of these thoughts through the old rock tumbler and come up with a clean answer.

JasonStoltz · 2019-07-10T21:09:40Z

packages/react-search-ui-views/src/view-helpers/screenReaderAnnouncements.js

+  return render(announceToScreenReader);
+};
+
+export default ScreenReaderStatus;


I think the name of this file should match the name of the default export, so ScreenReaderStatus in this case. Just makes it easier to find things.

JasonStoltz · 2019-07-11T12:47:11Z

packages/react-search-ui-views/src/MultiCheckboxFacet.js

-          + More
-        </button>
+        <ScreenReaderStatus
+          render={announceToScreenReader => (


A couple of thoughts:

I like the render prop pattern for this, but did you consider using the "children" property rather than an explicit "render" property? I think it just makes the syntax a little more concise.

Rather than a render prop, did you consider creating a custom hook?

If a user is creating a custom view, how do we expect them to use this? Do we expect them to import it from "@elastic/react-search-ui-view/view-helpers"? If so, we should document that in the "customization" section or something. Let's do that documentation in a separate PR though (Document "view-helpers" in view customization guide. #323). There's also probably a larger discussion there as well, of how we expose helpers for views... require users to import from "view-helpers", or pass relevant helpers through as props from the container?

Instead of a view helper, could this be its own view component?

I've used both in the past, just tend to use render out of habit. Good with switching to children or whatever we prefer!

After reading your comments down below I think I'm actually leaning towards moving away from a render prop (if we do indeed want to make this usable to non-React apps). Re: custom hooks specifically - I ran into crashes/errors when I tried to use hooks with the MultiCheckboxFacet component (I think related to the previous hook issues we've seen?) - let me try to create a reproducible branch.

Yeah, I definitely didn't think too much about making this work for developers using custom views/not using react views, etc. I think we're starting to think about moving away from this in other comments.

See previous comments - I think we're discussing moving away from this being React dependent, correct me if I'm wrong? (although I'm definitely not 100% on this, so happy to get clarification)

Re 2. - here's an example branch/commit with a very basic hook usage that creates the following console error:

I think this is related to #276 but I've been wrong before :)

JasonStoltz · 2019-07-11T13:00:06Z

packages/react-search-ui-views/src/PagingInfo.js

@@ -1,17 +1,27 @@
 import PropTypes from "prop-types";
 import React from "react";

-import { appendClassName } from "./view-helpers";
+import { appendClassName, ScreenReaderStatus } from "./view-helpers";


In this project it's often a bit fuzzy to me what should go into a container and what should go into a view.

The guidance we give users for customizing views is to use the default view as a base and customize from there. That could be a bit of a foot gun if they forget to call "announceToScreenReader", as you point out.

The goal of having swappable views, is to give users a very easy way to use custom markup for a component. The less they have to reproduce in the view, the better. To that end, it would seem more ideal to do this in a container. We could either do it completely transparently, or we could roll all of the logic into a single prop that we pass down.

function PagingInfo({ className, end, searchTerm, start, totalResults, screenReaderAnnouncement }) { end = Math.min(end, totalResults); return ( <> <div className={appendClassName("sui-paging-info", className)}> Showing{" "} <strong> {start} - {end} </strong>{" "} out of <strong>{totalResults}</strong> for: <em>{searchTerm}</em> </div> {screenReaderAnnouncement(someCustomMessageFunction)} </> ); }

It could be something like, if you don't call it at all, the default announcement will be used. if you call it with a custom function, screenReaderAnnouncement(), then that overrides the default message with a custom message.

JasonStoltz · 2019-07-11T13:05:26Z

packages/react-search-ui-views/src/PagingInfo.js

@@ -1,17 +1,27 @@
 import PropTypes from "prop-types";
 import React from "react";

-import { appendClassName } from "./view-helpers";
+import { appendClassName, ScreenReaderStatus } from "./view-helpers";


I also envisioned a future state where users could use react-search-ui without including react-search-ui-views at all, so they could have a completely custom view layer. Having this be part of the container would make that a challenge 🤔.

JasonStoltz · 2019-07-11T13:11:33Z

packages/react-search-ui-views/src/view-helpers/screenReaderAnnouncements.js

+};
+
+const announceToScreenReader = announcement => {
+  if (hasDOM) {


Do we need to do explicit DOM manipulation? Could we use a more declarative, stateful approach? Like could announcements be managed in central state somewhere?

For instance, messages could go into their own context store and we could just wire a ScreenReaderStatus component up to that context and re-render when it changes.

We could even take it a step further and make screen reader status part of the core Search UI state, and have an action to update it.

If it were part of the core Search UI state, it would make it available for any library also, whether it's Vue, React, or vanilla js, etc.

At some point we'll probably want to do explicit DOM manipulation (we could use a React portal, but that's also essentially just DOM appending/manipulation and will still need a document/window check for SSR apps). Also FWIW, I looked at how downshift was doing announcements as well after noticing their live region, and they're essentially doing what we're doing, which gives me a bit of confidence.

Ah, wait, no, I think I maybe see what you mean by central state. Originally when I was making this, I was really only thinking in terms of putting this in react-search-ui-views, and my hesitation towards putting it in, e.g. <Layout> was something along the lines of "What if the developer isn't using our Layout component".

But if we're moving to react-search-ui instead (correct me if I'm wrong), then we can take advantage of the top level <SearchProvider> component itself to create the region + manage a central state.

Is that what you were thinking?

Or, wait, sorry, I'm still catching up again :) You're thinking even higher level than that - putting it in SearchDriver.js (or it's own file - like URLManager?) - correct me if I'm wrong?

I'd love to get a chance to chat with you more about this to clarify architecture a bit more!

@constancecchen Yes, I think this warrants a chat. Will message you later today!

Summary of our chat: #325

JasonStoltz · 2019-07-11T13:18:46Z

Also 💯 for this PR.

cee-chen · 2019-07-17T23:05:50Z

Closing this in favor of a new PR: #355

cee-chen added 4 commits July 9, 2019 12:06

✨ Add new ScreenReaderStatus render prop helper

33dbb4d

✅ Add tests for new ScreenReaderStatus helper

9c2faf0

♻️ Update MultiCheckboxFacet to use new ScreenReaderStatus render prop

a3118be

✅ Write new tests and update snapshots for MultiCheckboxFacet

8ad0791

cee-chen requested a review from JasonStoltz July 9, 2019 19:37

cee-chen added 2 commits July 9, 2019 12:39

♻️ Update PagingInfo to use new ScreenReaderStatus helper

31dd3ad

🐛 Prevent crashing/errors on server-side rendered apps

0f8be39

cee-chen requested a review from yakhinvadim July 9, 2019 19:41

✅ Fix react-search-ui snapshot tests

9086bcf

cee-chen commented Jul 9, 2019

View reviewed changes

JasonStoltz suggested changes Jul 11, 2019

View reviewed changes

JasonStoltz mentioned this pull request Jul 11, 2019

Add screen reader live region support #325

Closed

cee-chen added the WIP label Jul 13, 2019

cee-chen mentioned this pull request Jul 17, 2019

Screen reader notifications #355

Closed

cee-chen closed this Jul 17, 2019

cee-chen deleted the a11y-screenreader-live branch July 17, 2019 23:05

cee-chen mentioned this pull request Jul 19, 2019

Accessible screen reader live region and notifications #359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[a11y] Adding a screen reader live region and announcements for actions #320

[a11y] Adding a screen reader live region and announcements for actions #320

cee-chen commented Jul 9, 2019 •

edited

Loading

cee-chen Jul 9, 2019

JasonStoltz Jul 11, 2019

JasonStoltz Jul 11, 2019

cee-chen Jul 9, 2019

JasonStoltz left a comment

JasonStoltz Jul 10, 2019

JasonStoltz Jul 11, 2019

cee-chen Jul 11, 2019 •

edited

Loading

cee-chen Jul 11, 2019 •

edited

Loading

JasonStoltz Jul 11, 2019

JasonStoltz Jul 11, 2019

JasonStoltz Jul 11, 2019 •

edited

Loading

JasonStoltz Jul 11, 2019

cee-chen Jul 11, 2019

cee-chen Jul 11, 2019

cee-chen Jul 11, 2019 •

edited

Loading

JasonStoltz Jul 11, 2019

JasonStoltz Jul 11, 2019

JasonStoltz commented Jul 11, 2019

cee-chen commented Jul 17, 2019

[a11y] Adding a screen reader live region and announcements for actions #320

[a11y] Adding a screen reader live region and announcements for actions #320

Conversation

cee-chen commented Jul 9, 2019 • edited Loading

Description

Screencaps

List of changes

QA

Future

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

cee-chen Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz commented Jul 11, 2019

cee-chen commented Jul 17, 2019

cee-chen commented Jul 9, 2019 •

edited

Loading

cee-chen Jul 11, 2019 •

edited

Loading

cee-chen Jul 11, 2019 •

edited

Loading

JasonStoltz Jul 11, 2019 •

edited

Loading

cee-chen Jul 11, 2019 •

edited

Loading