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

[select2] fix(a11y): add aria-multiselectable, aria-autocomplete #5357

Merged
merged 4 commits into from
Jun 7, 2022

Conversation

bvandercar-vt
Copy link
Contributor

Fixes #0000

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

@bvandercar-vt bvandercar-vt changed the title Bv/multiselect role Bv/multiselect aria attributes a11y improvement Jun 7, 2022
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

the aria-multiselectable docs say that we should use this with aria-selected.

I looked into this... at first glance, it seems like the simplest thing we can do to get that working is apply "aria-selected": selected === true here:

{
role: "menuitem",
tabIndex: 0,
...htmlProps,
...(disabled ? DISABLED_PROPS : {}),
className: anchorClasses,
},

BUT -- that's not right because of some weirdness in the current MenuItem API around "selected" vs. "active" appearance. MenuItem's styling API sort of assumes single (not multi) selection. I will follow up on this problem later.

for the time being, we must push the aria-selected responsibility onto consumers. you should set aria-selected={this.isFilmSelected(film)} here:

<MenuItem
selected={modifiers.active}
icon={this.isFilmSelected(film) ? "tick" : "blank"}
key={film.rank}
label={film.year.toString()}
onClick={handleClick}
text={`${film.rank}. ${film.title}`}
shouldDismissPopover={false}
/>

@@ -241,6 +241,7 @@ export class MultiSelect2<T> extends AbstractPureComponent2<MultiSelect2Props<T>
};
return (
<div
aria-autocomplete="list"
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a good addition... I think we should also set this in other places we use role="combox"... Select2 and Suggest2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have that on Suggest2. On Select2, it didn't seem appropriate on the combobox since you can't type into that. I put it on the filter input though.

@adidahiya adidahiya changed the title Bv/multiselect aria attributes a11y improvement [select2] fix(a11y): add aria-multiselectable, aria-autocomplete attrs Jun 7, 2022
@bvandercar-vt
Copy link
Contributor Author

aria-selected={this.isFilmSelected(film)}

done.

@adidahiya adidahiya changed the title [select2] fix(a11y): add aria-multiselectable, aria-autocomplete attrs [select2] fix(a11y): add aria-multiselectable, aria-autocomplete Jun 7, 2022
@adidahiya adidahiya merged commit 7d372c3 into palantir:develop Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants