[EuiSuggest] Use `EuiSelectable` by thompsongl · Pull Request #5157 · elastic/eui

thompsongl · 2021-09-08T20:08:29Z

Summary

Resolves #3733 by converting the list element in EuiSuggest to use EuiSelectable, which now implements the ARIA 1.2 combobox pattern.

Other changes:

Completes keyboard remediation ([EuiSuggest] Accessibility #2404)
- EuiSelectable does the heavy lifting
- Filtering and Enter selection behavior is left to consumers, so it intentionally appears incomplete
⚠️ Breaking changes
- Removes EuiSuggestInput
- Requires either aria-label or aria-labelledby
- Renames sendValue prop to onSearchChange

Closes #5336, closes #2404, closes #4419, closes #4345, closes #4342

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 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

kibanamachine · 2021-09-08T20:38:21Z

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

thompsongl · 2021-09-08T21:32:29Z

I had an idea for a custom hook that would provide stateful logic and ARIA attributes needed to make combox-like components that we could use here and elsewhere. Going to look into that before determining the best path forward for this PR.

kibanamachine · 2021-09-08T21:53:30Z

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

kibanamachine · 2021-09-29T19:48:58Z

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

kibanamachine · 2021-09-30T15:39:02Z

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

kibanamachine · 2021-10-13T18:36:36Z

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

kibanamachine · 2021-10-13T22:00:08Z

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

kibanamachine · 2021-10-14T17:18:14Z

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

thompsongl · 2021-10-14T17:36:48Z

@1Copenut Before I open this up for full review, would you mind checking this for a11y completeness? I'd like to have that part solid before making further review adjustments.

1Copenut

The code changes all look good to me. I noticed the focus was being returned to the search input and the listbox being re-opened when I tried to tab away. Requested change added as an image here. I'll retest for keyboard and screenreader interaction after change is in.

kibanamachine · 2022-02-07T22:09:40Z

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

thompsongl · 2022-02-07T23:48:25Z

I noticed the focus was being returned to the search input and the listbox being re-opened when I tried to tab away

Looks like a focus trap change in #5584 had an impact here. Still trying to diagnose.

Update:
Should be fixed in a58f492.
Haven't tracked it down, but I suspect a bug was fixed in a dep and made available to us in #5584. The now-current configuration seems more correct to me based on desired key behavior.

kibanamachine · 2022-02-08T00:40:59Z

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

cee-chen · 2022-02-08T18:06:23Z

🎉 This keyboard accessibility is way better than prod! Couple QA comments/questions in no particular order (apologies if I should have threaded these):

Is there any way to override (via CSS) the white gradient/mask on the .euiSelectableList__list dropdown for EuiSuggest only? It just looks weird on EuiSuggest (personal opinion).
It looks like this longer item is getting cut off and cannot be scrolled to in the virtualized dropdown demo. It does not get cut off when the demo is not virtualized. Any thoughts on how to fix it?
[optional, can be a separate PR] This is not a specific issue with EuiSuggest, but with the underlying EuiSelectable - when on the virtualized scrolling dropdown example, when I press a non-character key (e.g. shift, ctrl, option, command), I get unexpectedly punted/scrolled back up to the top item in the list. I feel like we should fix this by checking for keydowns that aren't actual input characters. Thoughts?
[optional, question] Maybe I'm just dumb/missing how EuiSuggest works, but is there any way to validate that pressing enter or space on an option actually selects something? The input value never appears to change. 🙈

thompsongl · 2022-02-08T18:27:24Z

Is there any way to override (via CSS) the white gradient/mask on the .euiSelectableList__list dropdown for EuiSuggest only? It just looks weird on EuiSuggest (personal opinion).

Maybe these could be removed in non-virtualized instances? Perhaps @cchaos has thoughts.

It looks like this longer item is getting cut off and cannot be scrolled to in the virtualized dropdown demo. It does not get cut off when the demo is not virtualized. Any thoughts on how to fix it?

This is a noted limitation of virtualized EuiSelectable lists (see Row height and virtualization). We could add a link to this from the EuiSuggest page.

[optional, can be a separate PR] This is not a specific issue with EuiSuggest, but with the underlying EuiSelectable - when on the virtualized scrolling dropdown example, when I press a non-character key (e.g. shift, ctrl, option, command), I get unexpectedly punted/scrolled back up to the top item in the list. I feel like we should fix this by checking for keydowns that aren't actual input characters. Thoughts?

Yep, separate PR makes sense. In that same PR I can resolve @1Copenut concern about the input clear button not responding to ENTER. Both of these are EuiSelectable issues and not specific to EuiSuggest.

[optional, question] Maybe I'm just dumb/missing how EuiSuggest works, but is there any way to validate that pressing enter or space on an option actually selects something? The input value never appears to change.

You could console.log (or whatever) the onChange callback. We leave selection behavior up to the consumer. I'll add to the docs that selection is not handled with internal state.

1Copenut

👍 I retested for keyboard and screen reader behavior with VO + Safari on MacOS Big Sur. The focus handling is spot on and the reduced set of options when I started typing worked well too. VO tends to be fairly verbose and repetitive, so I have no doubt this will be a big upgrade for NVDA and JAWS too. Thanks @thompsongl !

cee-chen

Mostly minor/optional comments, and a few maybe-blocking comments about unit tests

src/components/suggest/_suggest_item.scss

src/components/suggest/suggest.tsx

src/components/suggest/types.ts

src/components/suggest/suggest_item.tsx

src/components/suggest/suggest.tsx

cchaos · 2022-02-08T19:26:26Z

Is there any way to override (via CSS) the white gradient/mask on the .euiSelectableList__list dropdown for EuiSuggest only? It just looks weird on EuiSuggest (personal opinion).

Unfortunately there's no way to discriminately show the overflow mask for when the box is scrolling or not. Yes, it may be awkward for short lists, but it has high impact on long lists to indicate "there is more below here":

I'm open to ideas, but I don't want to turn it off for all of EuiSelectable/EuiSuggest because it will turn if off for overflowing lists.

cchaos

Did a real quick final code and design review. LGTM!

kibanamachine · 2022-02-08T22:41:21Z

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

src/components/suggest/suggest.tsx

cee-chen · 2022-02-08T23:20:33Z

src/components/suggest/suggest.spec.tsx

+
+describe('EuiSuggest', () => {
+  describe('onItemClick', () => {
+    it('is called when an option is clicked', () => {


🤩 Nice!! These Cypress tests are great!!! Thanks for adding them!

cee-chen

New tests look awesome! I have just one super minor/opinionated code style suggestion, feel free to ignore it and merge if you don't think it's a significant improvement

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

kibanamachine · 2022-02-09T00:40:40Z

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

thompsongl added 4 commits September 8, 2021 14:51

update euiselectable with paddingSize prop, tests

0c26555

euisuggest use euiselectable; more tests

08afa9b

CL

37d8c62

handle empty list

ec00be6

Merge branch 'master' into 3733-euisuggest

7ad561e

thompsongl added 3 commits September 10, 2021 11:29

Merge branch 'master' into 3733-euisuggest

b582e45

WIP: useCombobox

4441d93

WIP: temporary docs

9abae26

thompsongl added 2 commits September 30, 2021 10:12

Merge branch 'master' into 3733-euisuggest

9f36199

snapshots

58eb516

thompsongl mentioned this pull request Sep 30, 2021

[EuiFormRow] API and a11y cleanup #2493

Closed

1 task

thompsongl added 4 commits October 7, 2021 12:19

Merge branch 'master' into 3733-euisuggest

4ea443f

Merge branch 'master' into 3733-euisuggest

942829d

keyboard nav

1bf759f

i18n

ddbbe51

thompsongl added 2 commits October 13, 2021 16:15

prevent prop overlap

8c81549

open logic

61d1356

thompsongl added 2 commits October 14, 2021 11:50

status i18n

037d6e8

Merge branch 'master' into 3733-euisuggest

fb7e65f

clean up

9dd680f

thompsongl requested a review from 1Copenut October 14, 2021 17:34

1Copenut suggested changes Feb 7, 2022

View reviewed changes

disabled redundant focus trap

a58f492

thompsongl requested a review from 1Copenut February 8, 2022 00:06

1Copenut approved these changes Feb 8, 2022

View reviewed changes

cee-chen reviewed Feb 8, 2022

View reviewed changes

cchaos approved these changes Feb 8, 2022

View reviewed changes

thompsongl added 3 commits February 8, 2022 14:16

Merge branch 'main' into 3733-euisuggest

5fa82d1

docs and tests

4930e20

move state message

f1fb323

thompsongl requested a review from cee-chen February 8, 2022 22:04

Merge branch 'main' into 3733-euisuggest

ee0eb31

cee-chen reviewed Feb 8, 2022

View reviewed changes

src/components/suggest/suggest.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Feb 8, 2022

View reviewed changes

cee-chen approved these changes Feb 8, 2022

View reviewed changes

thompsongl and others added 2 commits February 8, 2022 17:32

Update src/components/suggest/suggest.tsx

bd7c33e

Co-authored-by: Constance <constancecchen@users.noreply.github.com>

lint

cf55e68

thompsongl enabled auto-merge (squash) February 9, 2022 00:03

thompsongl merged commit c8f2b55 into elastic:main Feb 9, 2022

thompsongl mentioned this pull request Feb 9, 2022

[EuiSelectable] Handle more keyboard scenarios #5613

Merged

6 tasks

breehall mentioned this pull request Feb 10, 2022

Upgrade EUI to v48.1.1 elastic/kibana#125023

Merged

thompsongl mentioned this pull request Jun 3, 2022

[EuiSuggest] Status changes width of text field #2528

Closed

cee-chen mentioned this pull request Jun 22, 2022

Create a status component and use it in EUI Docs to announce page loads to screen readers #5991

Closed

11 tasks

Conversation

thompsongl commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Sep 8, 2021

Uh oh!

thompsongl commented Sep 8, 2021

Uh oh!

kibanamachine commented Sep 8, 2021

Uh oh!

kibanamachine commented Sep 29, 2021

Uh oh!

kibanamachine commented Sep 30, 2021

Uh oh!

kibanamachine commented Oct 13, 2021

Uh oh!

kibanamachine commented Oct 13, 2021

Uh oh!

kibanamachine commented Oct 14, 2021

Uh oh!

thompsongl commented Oct 14, 2021

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 7, 2022

Uh oh!

thompsongl commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Feb 8, 2022

Uh oh!

cee-chen commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thompsongl commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cchaos commented Feb 8, 2022

Uh oh!

cchaos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 8, 2022

Uh oh!

Uh oh!

cee-chen Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

thompsongl commented Sep 8, 2021 •

edited

Loading

thompsongl commented Feb 7, 2022 •

edited

Loading

cee-chen commented Feb 8, 2022 •

edited

Loading

thompsongl commented Feb 8, 2022 •

edited

Loading

cchaos left a comment •

edited

Loading