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.

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

Aria Live Configuration #4414

Merged
merged 20 commits into from
Mar 4, 2021
Merged

Aria Live Configuration #4414

merged 20 commits into from
Mar 4, 2021

Conversation

ebonow
Copy link
Collaborator

@ebonow ebonow commented Feb 1, 2021

WIP: recreation of Kashkovsky support intl a11y

Original PR: #3550
Revised PR: #4161
First pass: #4390
Documentation PR: #4203

Purpose

The purpose of this PR is to allow the aria live messages to be customized to support internationalization or serve to provide more context to assistive technologies #3352

Proposal

prop: AriaLiveMessages is an object supporting several formatting functions for various aria-live messages and events

  • onChange
  • onFocus
  • onFilter
  • guidance

Additionally: a new prop aria-live is introduced to allow users to change the voice of the aria-live component. This should satisfy users unhappy with passive voice (due to listbox guidance interfering with focus events) and users who want to implement a solution without any aria-live.

This PR also introduces a new LiveRegion component thought it is not yet shared publicly. As relevant state changes are made in Select.js, they are passed as props to LiveRegion and then use useEffect hooks to determine what changes should be made to the rendered aria live messaging, This also has the benefit of removing 100+ lines of code from Select.js

While several other aria-live issues exist, this one is comprehensive enough to test first and then isolate other issues in a follow up. I will keep the to-dos below to keep visibility.

Additional Findings

Allow aria-live and role to be configurable

  • Aria roles appear to have a blocking affect on the aria-live implementation once it begins to speak on the directions for the listbox. Keyboard interaction is blocked and in some cases queued up forcing several "Focused option" messages to be read off consecutively.
  • Setting role to alert seems to improve functionality in OSX VoiceOver, but unsure of impact on other SR's (especially since aria-live is still set to polite)
  • Setting aria-live to assertive creates the most predictable results, but apparently blocks some other expected functionality so it may be worth considering one or the other to be disabled.
  • Setting aria-atomic to true was suggested in another thread, but this causes issues with the selection text then being read on every update as all children nodes are read back instead of the just the changing context.

Concerns

  • The guidance instructions read back on every input and focus option are a bit tedious. Perhaps this is something we only want to read when the context changes.
  • It seems both redundant and inaccurate to state the index of the focused option as it applies to all the options as not all options are showing especially when the results as returned from the screenreaderStatus prop return the number of focusableOptions
  • It seems like a serious missing that the selected options are not read back when a user focuses on the select.

Groups

  • Group support is completely broken...
  • Group Headers are not available to be read with the option. There is no read-only function to identify the string mapping for the group headers. Would recommend using formatGroupHeader as a formatting function and creating a new bulitin called getGroupHeaderLabel.
  • Need to introduce a way to identify group heading from the option
  • Index of results always reads back 0 of # of groups, because groups are the child component of the menuList
  • Should headers be focusable? Current select behavior is such that they are focusable but disabled.

Aria-live TODOs

and...

@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2021

🦋 Changeset detected

Latest commit: f4aaf24

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@react-select/docs Minor
react-select Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ebonow ebonow mentioned this pull request Feb 1, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f4aaf24:

Sandbox Source
react-codesandboxer-example Configuration

@ebonow ebonow added category/accessibility Issues or PRs related to accessibility category/documentation Issues or PRs about documentation or the website itself labels Feb 1, 2021
@ebonow ebonow added this to the 4.1 milestone Feb 1, 2021
Copy link
Collaborator

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

I like the direction this is headed! I haven't done a thorough review of Select.js yet, but I thought I should provide some initial comments. Sorry it took so long for me to take a look!

packages/react-select/src/accessibility/index.js Outdated Show resolved Hide resolved
packages/react-select/src/components/LiveRegion.js Outdated Show resolved Hide resolved
packages/react-select/src/components/LiveRegion.js Outdated Show resolved Hide resolved
packages/react-select/src/Select.js Outdated Show resolved Hide resolved
packages/react-select/src/Select.js Outdated Show resolved Hide resolved
const isSelected = !!(
focusedOption &&
selectValue &&
selectValue.includes(focusedOption)
Copy link

@bozdoz bozdoz Mar 1, 2021

Choose a reason for hiding this comment

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

this fails to determine isSelected if isMulti and multiple options are chosen. I tried importing ../internal/react-fast-compare (which doesn't appear to be used anywhere) and made it work; for your consideration:

Suggested change
selectValue.includes(focusedOption)
!!selectValue.find((selected) => exportedEqual(selected, focusedOption))

that function imported at the top:

import exportedEqual from '../internal/react-fast-compare';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, let me take a look at this and add some tests to make sure this doesn't get missed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have latest and can you provide an example of it not working correctly? I do not seem to have the same problem of the select identifying menu options that are selected.
Screen Shot 2021-03-01 at 3 51 14 PM

Copy link

Choose a reason for hiding this comment

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

I do have latest. I don't know how to provide a good example, but I'll try. I'm using hooks:

const FRUITS_AND_VEGGIES = [
    'Apple',
    'Artichoke',
    'Asparagus',
    'Banana',
    'Beets',
    'Bell pepper',
    'Broccoli',
    'Brussels sprout',
]
export const MultipleValues = () => {
    const options = FRUITS_AND_VEGGIES.map((x, i) => ({
        value: x,
        label: x,
        isDisabled: i % 5 === 1,
    }));

    const [value, setValue] = useState<any>(options[2]);

    return (
        <div>
            <label id="ex3-label">Fruits and Vegetables</label>
            <Select
                isMulti
                value={value}
                onChange={(v) => setValue(v)}
                options={options}
                closeMenuOnSelect={false}
                aria-labelledby="ex3-label"
                isSearchable
                isClearable={false}
            />
       </div>
    );
};

Here's what I'm getting:

Screen Shot 2021-03-01 at 6 31 45 PM

But it's a different message initially:

Screen Shot 2021-03-01 at 6 32 30 PM

Copy link

@bozdoz bozdoz Mar 2, 2021

Choose a reason for hiding this comment

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

Oh, you know what's happening is that the function is being rerendered, and the array map is generating new objects (in my example). So your test is checking for identical objects, which fails in my case. If I defined it outside of my function component, then it should work with the simple equality check. Makes sense. For your consideration.

I could even put the array in a useMemo hook. But maybe it would be important to know that for this case. Not sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is some internal discussion regarding the conversation of comparing objects by value or reference. I don't disagree with a value based approach over one that is referential, but opted to remain consistent with the existing approach for the sake of consistency.

@ebonow
Copy link
Collaborator Author

ebonow commented Mar 1, 2021

@bozdoz , so I reworked a few things about this component over the last week.

  • I condensed all of the formatting functions to take a single prop.
    • The rationale was that it's not clear to me whether it makes sense to have different things in different places and just makes it harder to deconstruct.
    • At first I wanted synchronicity with the onChange function, but found that to be confusing since they are essentially sued for two different things.
      • onChange as a select prop function is expected to pass value back to state for controlled input.
      • onChange as a formatting function is expected to pass back information regarding the selected/removed option to vocalize it to the user and the biggest difference between the two is that the formatting function fires when a user selects a disabled option.
    • It's also worth noting that the option prop is not populated in the actionMeta of onChange when select-option is called on a single select, but is when it isMulti, so for consistency, I return a selected prop and the derived label of that, because I find the onChange actionMeta overly complicated when option could simply be present on every action.

</span>
<span id="aria-context">&nbsp;{this.constructAriaLiveMessage()}</span>
</A11yText>
<LiveRegion
Copy link

Choose a reason for hiding this comment

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

Low effort improvement would be to include this in getComponents:

In Select.js

const { LiveRegion } = this.getComponents()

And then for components/index.js:

LiveRegion: ComponentType<LiveRegionProps>,

Copy link
Collaborator Author

@ebonow ebonow Mar 3, 2021

Choose a reason for hiding this comment

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

I think exposing it in the public facing components is a near future goal but not for this release based on a few reasons:

  • It's not completely known how well this works with all screen readers yet.
  • The logic to recreate the LiveRegion given the amount of dependencies isn't trivial.
  • React-select does exist as something designed to be customizable, but we want to be cautious given that this can very negatively affect usability for those reliant on a11y assistive technology.
  • There are still a few aria-live issues to be discussed and solved before exposing the api as incomplete and potentially in need of changing.

I think ultimately I envision all of the derived messages would be passed as props to the LiveRegion making it easier to customize in as many different a11yText sections as desired.

@JedWatson JedWatson enabled auto-merge March 4, 2021 22:48
@JedWatson JedWatson merged commit f9b2015 into JedWatson:master Mar 4, 2021
@github-actions github-actions bot mentioned this pull request Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/accessibility Issues or PRs related to accessibility category/documentation Issues or PRs about documentation or the website itself
Projects
None yet
4 participants