Screen reader notifications #355

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

Description

#320 redux - moving things out of views to SearchDriver 🚂

TODO - I'm opening this now to get some sanity checks/discussion points before moving further in my assumptions.

List of changes

TODO

Associated Github Issues

- a11yNotifications flag/bool - a11yNotificationMessages obj of functions (this feels like it has to be a state to be passed to the withSearch HOC, correct me if I'm wrong) - a11yNotify action

cee-chen · 2019-07-17T23:17:24Z

@JasonStoltz and @yakhinvadim - I'd super appreciate your sanity checks on what direction to go with announcing search results.

487e24d - Use PagingInfoContainer from react-search-ui - this is pretty straightforward but it does mean that non-React users no longer gain screen reader accessibility.
4e87abf - Use SearchDriver from search-ui itself - this guarantees that all users will get this screen reader messaging. I think this is preferable, however, as you can see I now have to repeat the start/end calculation logic from PagingInfoContainer.

I'm pretty sure 2. is the way to go - if so, would you all be OK with moving the start/end logic to SearchDriver.js and passing it down from there for DRYness instead?

cee-chen · 2019-07-17T23:17:40Z

packages/search-ui/src/SearchDriver.js

@@ -151,7 +165,8 @@ export default class SearchDriver {
    // to the correct default values for the initial UI render
    this.state = {
      ...this.state,
-      ...searchParameters
+      ...searchParameters,
+      ...{ a11yNotificationMessages }


actually - I'm not 100% sure, but should this really be in state? Or does it feel more like an action?

In theory you are calling functions, e.g. a11yNotificationMessages.searchResults({ ... }) to produce strings (e.g. Showing X out of Y ...), which makes me think this belongs more in actions than in state.

Please, take the following with a grain of salt, I'm not sure if I'm right or not.

They don't look like state - they don't change after initialization.
They also don't look like actions - you don't call them directly to change state.
Aren't they more like a part of a config?

Yeah, I think I tend to agree with you. The thing is, I don't know how else to pass it down to the withSearch HOC though - since state and action are what get passed down, and we need views/users to be able to access the message functions somehow.

Should we maybe pass down a whole new key like getMessages or driver.messages?... 😬

we need views/users to be able to access the message functions somehow

Why? I thought users would add these functions to config once and then not touch it anywhere in the components (in most cases). If (in rare cases) users need to change the a11y message for some reason manually, they would call one universal action a11yNotify.

Yes, that was my impression too. I thought the only thing we were going to do with them is have the SearchDriver write them out to a DOM element.

This makes the a11yNotify action feel much more legit now too. Thanks a billion! ✨

Also, just one more follow-up to this refactor that I only just considered - if users want to roll their own completely custom a11y notification for, e.g. their completely custom component - they should accomplish this by extending the a11yNotificationMessages obj, correct? So, for example:

SearchDriver({ a11yNotificationMessages: { customMessage: () => 'Hello world', }, });

This means that:

We're now limiting the a11yNotify action to not simply allow strings - originally I was thinking users could do something like a11yNotify('Whatever I want!'), but that will no longer be the case with this architecture. Are we good with that? (I'm good with it personally, just checking that we're all on the same page.)

Per 78ffd85, we're now limiting a11yNotificationMessages to only contain functions. For example, if someone tried to pass customMessage: 'Hello world', a type error would get thrown. Are we okay with this restriction? Or do we want to add a typeof function check to allow both primitive strings and functions? 🤔 (Feels a bit overkill to me and I'm fine with the limitation, but also wanted to check that we all agree.)

@JasonStoltz @yakhinvadim - would love your thoughts! 😀

@constancecchen Probably clearer for users to just always use functions. Easier to document, less code, smaller API surface.

(2) I agree with Jason

(1) I'm not 100% sure about this, more like 70%, but if you both think that that's ok, I'm in.

yakhinvadim · 2019-07-18T18:49:32Z

I agree that 2 is the way to go. I'm also for storing the start/end logic in SearchDriver, I can't think of a case where a user would want to calculate it another way. This would also be very convenient for non-React users.

JasonStoltz · 2019-07-18T19:47:20Z

@constancecchen I think you are spot on with option 2. That would be great.

- per Jason's excellent comment: #355 (comment)

cee-chen · 2019-07-18T21:38:14Z

packages/search-ui/src/SearchDriver.js

+        const end =
+          totalResults <= start + resultsPerPage
+            ? totalResults
+            : start + resultsPerPage - 1;


Following up here on our decision to move start/end logic out of PagingInfoContainer and into SearchDriver - are you guys good with this? Or is there a more elegant way/place to put this that I'm missing? 🤔

Here's the full commit for context: 4c58a73

@constancecchen This looks correct to me. Since these have moved to the global state, I think these should be renamed to give some more context. Maybe pagingStart and pagingEnd?

cee-chen · 2019-07-19T17:47:58Z

Thanks so much for all the comments and help everyone! Sounds like we're good to go with this direction. I'm going to close this draft PR and open a new one that's been cleaned up and rebased (so we can keep this architecture discussion for reference but have a fresh start for code review). 🙏

cee-chen added 4 commits July 17, 2019 15:58

Add initial a11y notifications helper/manager

6999702

Set up SearchDriver with a11y notification state

47acba0

- a11yNotifications flag/bool - a11yNotificationMessages obj of functions (this feels like it has to be a state to be passed to the withSearch HOC, correct me if I'm wrong) - a11yNotify action

??? Add a11y notification to PagingInfoContainer ???

487e24d

??? Do Search Results announcement in SearchDriver.js instead ???

4e87abf

cee-chen added the WIP label Jul 17, 2019

cee-chen changed the title ~~A11y notifications~~ Screen reader notifications Jul 17, 2019

cee-chen mentioned this pull request Jul 17, 2019

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

Closed

1 task

cee-chen commented Jul 17, 2019

View reviewed changes

cee-chen added 4 commits July 18, 2019 13:26

Add a11y +More options behavior for Facet.js

114921e

Refactor a11yNotificationMessages to remain within SearchDriver

78ffd85

- per Jason's excellent comment: #355 (comment)

Revert PagingInfoContainer changes

46fdf99

??? Move start/end calculation out of PagingInfo and into SearchDriver

4c58a73

cee-chen commented Jul 18, 2019

View reviewed changes

cee-chen closed this Jul 19, 2019

cee-chen deleted the a11y-notifications branch July 19, 2019 17:48

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

Screen reader notifications #355

Screen reader notifications #355

cee-chen commented Jul 17, 2019

cee-chen commented Jul 17, 2019 •

edited

Loading

cee-chen Jul 17, 2019 •

edited

Loading

yakhinvadim Jul 18, 2019

cee-chen Jul 18, 2019 •

edited

Loading

yakhinvadim Jul 18, 2019 •

edited

Loading

JasonStoltz Jul 18, 2019

cee-chen Jul 18, 2019 •

edited

Loading

cee-chen Jul 18, 2019 •

edited

Loading

JasonStoltz Jul 19, 2019

yakhinvadim Jul 19, 2019

yakhinvadim Jul 19, 2019

yakhinvadim commented Jul 18, 2019

JasonStoltz commented Jul 18, 2019

cee-chen Jul 18, 2019

JasonStoltz Jul 19, 2019

cee-chen commented Jul 19, 2019

Screen reader notifications #355

Screen reader notifications #355

Conversation

cee-chen commented Jul 17, 2019

Description

List of changes

Associated Github Issues

cee-chen commented Jul 17, 2019 • edited Loading

cee-chen Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

yakhinvadim Jul 18, 2019

Choose a reason for hiding this comment

cee-chen Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

yakhinvadim Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

JasonStoltz Jul 18, 2019

Choose a reason for hiding this comment

cee-chen Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

cee-chen Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

JasonStoltz Jul 19, 2019

Choose a reason for hiding this comment

yakhinvadim Jul 19, 2019

Choose a reason for hiding this comment

yakhinvadim Jul 19, 2019

Choose a reason for hiding this comment

yakhinvadim commented Jul 18, 2019

JasonStoltz commented Jul 18, 2019

cee-chen Jul 18, 2019

Choose a reason for hiding this comment

JasonStoltz Jul 19, 2019

Choose a reason for hiding this comment

cee-chen commented Jul 19, 2019

cee-chen commented Jul 17, 2019 •

edited

Loading

cee-chen Jul 17, 2019 •

edited

Loading

cee-chen Jul 18, 2019 •

edited

Loading

yakhinvadim Jul 18, 2019 •

edited

Loading

cee-chen Jul 18, 2019 •

edited

Loading

cee-chen Jul 18, 2019 •

edited

Loading