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

[EuiComboBox] Revising to WAI-ARIA 1.2 spec. #5636

Merged
merged 10 commits into from
Feb 23, 2022
Merged

[EuiComboBox] Revising to WAI-ARIA 1.2 spec. #5636

merged 10 commits into from
Feb 23, 2022

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Feb 14, 2022 •
edited
Loading

Summary

The axe browser and CLI scan was throwing a few errors on our EuiCombobox component. I refactored it to match the WAI-ARIA 1.2 pattern for combobox with a listbox autocomplete.

This PR closes #5301 and closes #5024.

Have a few items to complete before bringing the PR out of draft:

  • Add an aria-autocomplete="list" attribute to the input
  • Add i18n to the default accessible listbox label

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@1Copenut
Revising EuiCombobox to WAI-ARIA 1.2 spec.
b4dfddf 
* Updated EuiCombobox to WAI-ARIA 1.2 pattern
* Fixing one unit test selector.
@1Copenut 1Copenut self-assigned this Feb 14, 2022
@cee-chen
Copy link
Contributor

Just to check, is this an intermediate step until we refactor EuiComboBox to use EuiSelectable? Or if that's coming up very soon, would it be more worth it to wait for that more holistic refactor? (Not sure if it is, this is fine if so)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5636/

@cee-chen
Copy link
Contributor

@1Copenut any thoughts on grabbing #5024 while you're here? It should be (hopefully) as easy as adding tabindex -1 and/or aria-hidden to the arrow icon, although I def could be wrong about that. If it's harder then we can punt it until the EuiSelectable refactor

@1Copenut
Copy link
Contributor Author

@constancecchen Yeah, that seems like an easy one to add to ^^ this PR. I'll fold it in. Should be working on the last items after lunch today. Appreciate the heads up.

@1Copenut 1Copenut mentioned this pull request Feb 17, 2022
Closed
@1Copenut
Added aria-autocomplete, aria-label to combo box."
10ac7b5 
* Removing open/close button from tab order, updating snapshots.
* Adding i18n to the listbox aria-label.
@1Copenut
Adding changelog entry for PR 5636.
016613d
@1Copenut
Upstream merge from main.
ea8bddd
@1Copenut 1Copenut marked this pull request as ready for review February 18, 2022 04:41
1Copenut
1Copenut commented Feb 18, 2022
View reviewed changes
Copy link
Contributor Author

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

Left general review notes about changes.

I don't think these rise to the level of breaking changes, but if reviewers feel different, I'll add the breaking change tag and make sure to be available to help with next Kibana upgrade.

src/components/combo_box/combo_box_input/combo_box_input.tsx Show resolved Hide resolved
src/components/combo_box/combo_box_input/combo_box_input.tsx Show resolved Hide resolved
src/components/combo_box/combo_box_input/combo_box_input.tsx Show resolved Hide resolved
src/components/combo_box/combo_box.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5636/

@1Copenut 1Copenut requested a review from cee-chen February 22, 2022 19:25
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5636/

@1Copenut 1Copenut requested review from chandlerprall and removed request for thompsongl February 22, 2022 21:35
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5636/

cee-chen
cee-chen approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks for super thoroughly responding to all my feedback! ❤️

chandlerprall
chandlerprall reviewed Feb 23, 2022
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Needs to merge with main again & update the changelog entry to the right location (otherwise git'll merge it into the v49 release)

Otherwise this LGTM! That was a smaller changeset than I was expecting for this change 🎉 . Thanks for linking directly to the WAI 1.2 combobox example, definitely helped streamline the review to directly compare that with our implementation.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5636/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5636/

@1Copenut 1Copenut merged commit bfddf67 into elastic:main Feb 23, 2022
@1Copenut 1Copenut deleted the feature/tpierce-eui-5301-combobox branch February 23, 2022 22:57
@thompsongl thompsongl mentioned this pull request Mar 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees

@1Copenut 1Copenut

Labels
accessibility - automated accessibility - cognition accessibility - screen reader accessibility - WCAG A Level A WCAG Accessibility Criteria accessibility
Projects
None yet
Milestone
No milestone
4 participants
@1Copenut @cee-chen @kibanamachine @chandlerprall