hypothesis / via

Proxies third-party PDF files and HTML pages with the Hypothesis client embedded, so you can annotate them
https://via.hypothes.is/
BSD 2-Clause "Simplified" License
20 stars 7 forks source link

Notify screen readers when transcript is filtered #1291

Closed acelaya closed 9 months ago

acelaya commented 9 months ago

Closes #1274

This PR ensures a hidden toast message is appended when filtering the transcript via search term, making screen readers announce the amount of results.

The way this has been implemented is not 100% correct regarding to https://react.dev/learn/you-might-not-need-an-effect, since it adds a side effect to Transcript based on some internal state.

The more correct solution would probably imply appending the toast message as part of the onChange event handler for the FilterInput (I think it would fall under this case https://react.dev/learn/you-might-not-need-an-effect#notifying-parent-components-about-state-changes), however, at that point we don't know how many segments matched the search term, hence the proposed solution here.

I evaluated other approaches as well, like wrapping FilterInput into a component which calculates filterMatches and appends the toast message as part of FilterInput's setFilter callback, and make that a piece of state that is handled by VideoPlayerApp and passed down to Transcript. This would mean the raw filter is no longer exposed beyond that wrapper component, but it doesn't seem to be needed anyway.

This would require more changes, though.

EDIT: Another argument in favor of going in a different direction is that, if onFilterMatch is not properly memoized, you get infinite toast messages appended, which is very unintuitive.

Testing steps

  1. Run a screen reader
  2. Try filtering the transcript.
  3. The screen reader should announce a message in the lines of "foo" returned 5 results every time the search term changes to more than 2 characters.
acelaya commented 9 months ago

We have decided to go with this approach for now.

This slack thread gives more details on what to do in future if filtered segments need to be used in other components.

acelaya commented 9 months ago

When testing this manually I noticed that the current approach is prone to reading duplicate notifications because the search input uses incremental search. This means if I type "some query", then a new search is executed for each character typed, and multiple hidden toast messages are rendered. Screen readers will try to read each of these. Additionally the "success" prefix that is added is redundant in this context.

I think what we'll need to do is avoid using toast messages and instead add a dedicated visually hidden container with an aria-live status that contains the current number of matches. That way the screen reader can use its existing logic for coalescing updates to a live element.

Yeah, that makes sense. Also, it would avoid the problematic behavior explained in the PR description, where a side effect is triggered when a prop changes.

I'll close this PR and prepare a new one with the other approach.