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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
33dbb4d
:sparkles: Add new ScreenReaderStatus render prop helper
cee-chen Jul 9, 2019
9c2faf0
:white_check_mark: Add tests for new ScreenReaderStatus helper
cee-chen Jul 9, 2019
a3118be
:recycle: Update MultiCheckboxFacet to use new ScreenReaderStatus ren…
cee-chen Jul 9, 2019
8ad0791
:white_check_mark: Write new tests and update snapshots for MultiChec…
cee-chen Jul 9, 2019
31dd3ad
:recycle: Update PagingInfo to use new ScreenReaderStatus helper
cee-chen Jul 9, 2019
0f8be39
:bug: Prevent crashing/errors on server-side rendered apps
cee-chen Jul 9, 2019
9086bcf
:white_check_mark: Fix react-search-ui snapshot tests
cee-chen Jul 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions packages/react-search-ui-views/src/MultiCheckboxFacet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import React from "react";
import deepEqual from "deep-equal";

import { FacetValue, FilterValue } from "./types";
import { appendClassName, getFilterValueDisplay } from "./view-helpers";
import {
appendClassName,
getFilterValueDisplay,
ScreenReaderStatus
} from "./view-helpers";

function MultiCheckboxFacet({
className,
Expand All @@ -12,6 +16,7 @@ function MultiCheckboxFacet({
onRemove,
onSelect,
options,
optionsCount,
showMore,
values,
showSearch,
Expand Down Expand Up @@ -82,14 +87,28 @@ function MultiCheckboxFacet({
</div>

{showMore && (
<button
type="button"
className="sui-multi-checkbox-facet__view-more"
onClick={onMoreClick}
aria-label="Show more options"
>
+ More
</button>
<ScreenReaderStatus
render={announceToScreenReader => (
Copy link
Member

Choose a reason for hiding this comment

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

A couple of thoughts:

  1. 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.
  2. Rather than a render prop, did you consider creating a custom hook?
  3. 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?
  4. Instead of a view helper, could this be its own view component?

Copy link
Member Author

@cee-chen cee-chen Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

  2. 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.

  3. 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.

  4. 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)

Copy link
Member Author

@cee-chen cee-chen Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Screen Shot 2019-07-11 at 8 24 18 AM

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

<button
type="button"
className="sui-multi-checkbox-facet__view-more"
aria-label="Show more options"
onClick={() => {
onMoreClick();

const newLimit = options.length + 10;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

const showingAll = newLimit >= optionsCount;
const message = `${
showingAll ? `All ${optionsCount}` : newLimit
} options shown.`;

announceToScreenReader(message);
}}
>
+ More
</button>
)}
/>
)}
</fieldset>
);
Expand All @@ -102,6 +121,7 @@ MultiCheckboxFacet.propTypes = {
onSelect: PropTypes.func.isRequired,
onSearch: PropTypes.func.isRequired,
options: PropTypes.arrayOf(FacetValue).isRequired,
optionsCount: PropTypes.number,
showMore: PropTypes.bool.isRequired,
values: PropTypes.arrayOf(FilterValue).isRequired,
className: PropTypes.string,
Expand Down
26 changes: 18 additions & 8 deletions packages/react-search-ui-views/src/PagingInfo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,27 @@
import PropTypes from "prop-types";
import React from "react";

import { appendClassName } from "./view-helpers";
import { appendClassName, ScreenReaderStatus } from "./view-helpers";
Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 🤔.


function PagingInfo({ className, end, searchTerm, start, totalResults }) {
end = Math.min(end, totalResults);
return (
<div className={appendClassName("sui-paging-info", className)}>
Showing{" "}
<strong>
{start} - {Math.min(end, totalResults)}
</strong>{" "}
out of <strong>{totalResults}</strong> for: <em>{searchTerm}</em>
</div>
<>
<div className={appendClassName("sui-paging-info", className)}>
Showing{" "}
<strong>
{start} - {end}
</strong>{" "}
out of <strong>{totalResults}</strong> for: <em>{searchTerm}</em>
</div>
<ScreenReaderStatus
render={announceToScreenReader => {
let message = `Showing ${start} to ${end} results out of ${totalResults}`;
if (searchTerm) message += `, searching for "${searchTerm}".`;
return announceToScreenReader(message);
}}
/>
</>
);
}

Expand Down
60 changes: 50 additions & 10 deletions packages/react-search-ui-views/src/__tests__/MultiCheckboxFacet.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const params = {
count: 5
}
],
optionsCount: 20,
showMore: true,
values: ["fieldValue2"]
};
Expand Down Expand Up @@ -70,16 +71,55 @@ it("renders range filters", () => {
expect(wrapper).toMatchSnapshot();
});

it("will render 'more' button if more param is true", () => {
const wrapper = shallow(
<MultiCheckboxFacet
{...{
...params,
showMore: true
}}
/>
);
expect(wrapper.find(".sui-multi-checkbox-facet__view-more")).toHaveLength(1);
describe("'more' button behavior", () => {
const announceToScreenReader = jest.fn();
const divePastScreenReaderStatus = wrapper =>
shallow(
wrapper.find("ScreenReaderStatus").prop("render")(announceToScreenReader)
);

it("renders button if showMore param is true", () => {
const wrapper = shallow(<MultiCheckboxFacet {...params} showMore={true} />);
const button = divePastScreenReaderStatus(wrapper);

expect(button.find(".sui-multi-checkbox-facet__view-more")).toHaveLength(1);
expect(button).toMatchSnapshot();
});

it("does not render button or screen reader status if there are no more options to show", () => {
const wrapper = shallow(
<MultiCheckboxFacet {...params} showMore={false} />
);

expect(wrapper.find("ScreenReaderStatus")).toHaveLength(0);
});

it("fires onMoreClick", () => {
const wrapper = shallow(<MultiCheckboxFacet {...params} showMore={true} />);
const button = divePastScreenReaderStatus(wrapper);

button.simulate("click");

expect(params.onMoreClick).toHaveBeenCalledTimes(1);
});

it("fires announceToScreenReader with the correct option counts", () => {
const wrapper = shallow(<MultiCheckboxFacet {...params} showMore={true} />);
const button = divePastScreenReaderStatus(wrapper);

button.simulate("click");
expect(announceToScreenReader).toHaveBeenCalledWith("12 options shown.");
});

it("fires announceToScreenReader with the correct maximum option count", () => {
const wrapper = shallow(
<MultiCheckboxFacet {...params} showMore={true} optionsCount={5} />
);
const button = divePastScreenReaderStatus(wrapper);

button.simulate("click");
expect(announceToScreenReader).toHaveBeenCalledWith("All 5 options shown.");
});
});

it("will render a no results message is no options are available", () => {
Expand Down
19 changes: 18 additions & 1 deletion packages/react-search-ui-views/src/__tests__/PagingInfo.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,28 @@ it("renders with className prop applied", () => {
const wrapper = shallow(
<PagingInfo className={customClassName} {...props} />
);
const { className } = wrapper.props();
const { className } = wrapper.childAt(0).props();
expect(className).toEqual("sui-paging-info test-class");
});

it("does not render a higher end than the total # of results", () => {
const wrapper = shallow(<PagingInfo {...props} totalResults={15} />);
expect(wrapper).toMatchSnapshot();
});

it("renders ScreenReaderStatus with the correct message", () => {
const announceToScreenReader = jest.fn();
const wrapper = shallow(<PagingInfo {...props} start={41} end={80} />);

wrapper.find("ScreenReaderStatus").prop("render")(announceToScreenReader);
expect(announceToScreenReader).toHaveBeenCalledWith(
'Showing 41 to 80 results out of 1000, searching for "grok".'
);

// Should not call out search term if one isn't present
wrapper.setProps({ searchTerm: "" });
wrapper.find("ScreenReaderStatus").prop("render")(announceToScreenReader);
expect(announceToScreenReader).toHaveBeenCalledWith(
"Showing 41 to 80 results out of 1000"
);
});
33 changes: 17 additions & 16 deletions packages/react-search-ui-views/src/__tests__/__snapshots__/MultiCheckboxFacet.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,9 @@ exports[`renders 1`] = `
</span>
</label>
</div>
<button
aria-label="Show more options"
className="sui-multi-checkbox-facet__view-more"
onClick={[MockFunction]}
type="button"
>
+ More
</button>
<ScreenReaderStatus
render={[Function]}
/>
</fieldset>
`;

Expand Down Expand Up @@ -145,13 +140,19 @@ exports[`renders range filters 1`] = `
</span>
</label>
</div>
<button
aria-label="Show more options"
className="sui-multi-checkbox-facet__view-more"
onClick={[MockFunction]}
type="button"
>
+ More
</button>
<ScreenReaderStatus
render={[Function]}
/>
</fieldset>
`;

exports[`'more' button behavior renders button if showMore param is true 1`] = `
<button
aria-label="Show more options"
className="sui-multi-checkbox-facet__view-more"
onClick={[Function]}
type="button"
>
+ More
</button>
`;
90 changes: 50 additions & 40 deletions packages/react-search-ui-views/src/__tests__/__snapshots__/PagingInfo.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,57 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`does not render a higher end than the total # of results 1`] = `
<div
className="sui-paging-info"
>
Showing

<strong>
0
-
15
</strong>

out of
<strong>
15
</strong>
for:
<em>
grok
</em>
</div>
<Fragment>
<div
className="sui-paging-info"
>
Showing

<strong>
0
-
15
</strong>

out of
<strong>
15
</strong>
for:
<em>
grok
</em>
</div>
<ScreenReaderStatus
render={[Function]}
/>
</Fragment>
`;

exports[`renders correctly 1`] = `
<div
className="sui-paging-info"
>
Showing

<strong>
0
-
20
</strong>

out of
<strong>
1000
</strong>
for:
<em>
grok
</em>
</div>
<Fragment>
<div
className="sui-paging-info"
>
Showing

<strong>
0
-
20
</strong>

out of
<strong>
1000
</strong>
for:
<em>
grok
</em>
</div>
<ScreenReaderStatus
render={[Function]}
/>
</Fragment>
`;
17 changes: 17 additions & 0 deletions ...es/react-search-ui-views/src/__tests__/view-helpers/screenReaderAnnouncements.ssr.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @jest-environment node
*/
import React from "react";
import { shallow } from "enzyme";
import { ScreenReaderStatus } from "../../view-helpers";

it("does not crash or create errors in server-side rendered apps", () => {
shallow(
<ScreenReaderStatus
render={announceToScreenReader => {
announceToScreenReader("Test");
return null;
}}
/>
);
});
Loading