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

[Dashboard] [Controls] Migrate to use EuiSelectable for options list suggestions #147624

Closed
Heenawter opened this issue Dec 15, 2022 · 3 comments · Fixed by #148420
Closed

[Dashboard] [Controls] Migrate to use EuiSelectable for options list suggestions #147624

Heenawter opened this issue Dec 15, 2022 · 3 comments · Fixed by #148420
Assignees
Labels
Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Accessibility Project:Controls Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@Heenawter
Copy link
Contributor

Heenawter commented Dec 15, 2022

Describe the feature:
Currently, we are building out the options list suggestions by mapping each suggestion to a single EuiFilterSelectItem. While this works, it means that our a11y support is limited - for example, you cannot currently use the arrow keys to navigate between items, you have to use tab instead.

Once elastic/eui#6485 is resolved and EUI has been upgraded to the version that includes these changes (I believe EUI v72.0.0), we should be able to migrate the options list suggestions to use EuiSelectable instead. This would not only clean up our code, it would also make our options list control much more a11y compliant 👍

@Heenawter Heenawter added blocked Feature:Dashboard Dashboard related features Project:i18n Feature:Input Control Input controls visualization loe:medium Medium Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Project:Controls labels Dec 15, 2022
@kibanamachine kibanamachine added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Dec 15, 2022
@botelastic botelastic bot added the needs-team Issues missing a team label label Dec 15, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@botelastic botelastic bot removed the needs-team Issues missing a team label label Dec 15, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-accessibility (Project:Accessibility)

@Heenawter
Copy link
Contributor Author

Now that #148354 is merged, this is no longer blocked 👍

@Heenawter Heenawter self-assigned this Jan 10, 2023
Heenawter added a commit that referenced this issue Jan 16, 2023
…ns (#148420)

Closes #147624
Closes #135470

## Summary
Previously, the suggestions/available options for the options list
control were manually built via individual `EuiFilterSelectItem`
components. While this worked well enough, it had a major `a11y` pitfall
- suggestions could only be navigated using `tab` rather than the up and
down arrow keys as expected.

This PR changes this behaviour to use `EuiSelectable` for managing and
displaying the suggestions instead, which has the following benefits:
1. Most importantly, `a11y` is significantly improved by allowing
navigation using the keyboard
2. A scrollbar is added to the options list popover - this makes it a
lot easier to manage since, between adding the footer to the popover and
adding the `exists` option, the popover was getting... rather unruly

![BeforeAndAfterVirtualization](https://user-images.githubusercontent.com/8698078/210903704-199f2953-e1e8-49c2-864d-4334f1da62a0.png)
3. The resulting `options_list_popover_suggestions.tsx` code is, in my
opinion, a bit easier to follow

This PR also fixes three small bugs:
1. The loading state was set to `false` for rejected requests, which
caused an early return of `No results` when typing quickly into the
search bar - see
#148420 (comment) for
more details.
2. When `singleSelect` was `true`, selecting `Exists` followed by a
different selection would not unselect `Exists`.
3. When `singleSelect` was `true`, selections could not be undone unless
`showOnlySelected` was `true`.

### Video of Keyboard Controls


https://user-images.githubusercontent.com/8698078/211362869-0b5f90ec-4784-4744-8de9-5c2881bfa7a9.mov


### Flaky Test Runners

-
`test/functional/apps/dashboard_elements/controls/options_list/options_list_dashboard_integration.ts`<br>
<a
href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1733"><img
src="https://user-images.githubusercontent.com/8698078/212110506-f05ba031-7328-4995-b929-d56c72d694ff.png"/></a>
This was the offending test suite that was causing all sorts of
flakiness, so I wanted to test it both on its own and as part of the
other controls tests - so, in combination with the below flaky test run,
this test suite was run 200 times successfully. Should **hopefully**
mean this thing is no longer flaky 🤞

- `test/functional/apps/dashboard_elements/controls/options_list/*`<br>
<a
href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1741"><img
src="https://user-images.githubusercontent.com/8698078/212143963-28856da0-d1ae-4b3f-801a-c96f8780e48b.png"/></a>

-
`test/functional/apps/dashboard_elements/controls/control_group_chaining.ts`<br>
<a
href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1744"><img
src="https://user-images.githubusercontent.com/8698078/212158041-a381ddb4-8b79-42b6-8b64-edd96cda262e.png"/></a>


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))

![BeforeAfterAxeFailures](https://user-images.githubusercontent.com/8698078/211360345-e9dea3c8-1068-41f7-bea3-921c9119afc2.png)
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Accessibility Project:Controls Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants