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

(Accessibility) [EuiSelectable] Unable to make selections when second interactable element in focus #6627

Closed
Heenawter opened this issue Mar 2, 2023 · 2 comments · Fixed by #6631

Comments

@Heenawter
Copy link
Contributor

For Dashboard input controls, we give users the ability to both search and filter the items in the EuiSelectable component as part of the creation/editing process:

Screenshot 2023-03-02 at 9 12 57 AM

To accomplish this, we are currently using the searchable prop and adding our filter component between the native search and list, like so:

<EuiSelectable
  searchable
  ...
>
{(list, search) => (
  <>
    {search}
    <EuiSpacer size={'s'} />
    {fieldTypeFilter} // this is our custom filter component
    <EuiSpacer size={'s'} />
    {list}
  </>
)}
</EuiSelectable>

Unfortunately, this messes with the a11y support that was added to the EuiSelectable component, since it seems to assume that all children should retain focus as the user navigates the list. This works great when the search is in focus, but it gives a confusing experience when the fieldTypeFilter is in focus, like so:

Mar-02-2023 09-28-26

In the above GIF, you can see that, while the fieldTypeFilter is in focus, the user is unable to make selections in the list despite it giving the appearance that they should be able to. It would be nice if only the search child retained focus in this way, whereas any other children did not - or, perhaps, if there was a way to control whether or not a child retained focus.

@cee-chen
Copy link
Contributor

cee-chen commented Mar 2, 2023

Here's the code that impacts this particular issue:

// For non-searchable instances, SPACE interaction should align with
// the user expectation of selection toggling (e.g., input[type=checkbox]).
// ENTER is also a valid selection mechanism in this case.
case keys.ENTER:
case keys.SPACE:
if (this.props.searchable) {
// For searchable instances, SPACE is reserved as a character for filtering
// via the input box, and as such only ENTER will toggle selection.
if (event.target === this.inputRef && event.key === keys.SPACE) {
return;
}
// Check if the user is interacting with something other than the
// searchbox or selection list. If not, the user is attempting to
// interact with an internal button such as the clear button,
// and the event should not be altered.
if (
!(
event.target === this.inputRef ||
event.target ===
this.optionsListRef.current?.listBoxRef?.parentElement
)
) {
return;
}
}

Lines 350-358 are what's causing this "problem". That early return was originally added to prevent selection from happening when the search clear button was 'pressed' (via enter or space key). In theory, however, it applies to literally any wildcard custom content inside the <EuiSelectable> as well.

So for example, if you added a custom button below the list that did some custom action (opened a modal, sent an email, loaded more items, etc.), wouldn't you want that item to not accidentally trigger selection as well? In theory, BTW, pressing Enter on your custom filter dropdown/button should be toggling its associated popover only, not triggering a selection.

In theory, what would be a "complete" fix would be to also prevent the up/down arrow keys from changing selection if the searchbox or listbox isn't the current focus. However, I totally get the filter behavior you've added and that you want it to behave the same way / as seamlessly as the searchbox itself. I'm just not really certain there's a way to implement that while also implementing a way to ignore keydowns on children that shouldn't trigger selections.

My personal instinct/recommendation for a fix would be:

  1. On filter selection or popover close, you should return focus manually to the EuiSelectable search input (restoring default keydown functionality) - you can access this input via <EuiSelectable searchProps={{ inputRef: yourRef }} />
  2. On EUI's side, we'll tweak our keydown event handler to return early for all events if the focus is not on either the searchbox or the listbox (meaning that the current misleading up/down behavior will not occur when focus is on the filter button).

Thoughts? This is definitely a complex problem to solve without a single clear/easy fix, FWIW.

@Heenawter
Copy link
Contributor Author

However, I totally get the filter behavior you've added and that you want it to behave the same way / as seamlessly as the searchbox itself.

My main issue was simply the fact that, when our filter dropdown has the focus, there was still a visual indicator in the listbox that makes it seem like you can still make a selection. That, combined with the fact that the listbox still responds to arrow key presses when the filter dropdown is in focus, made for an overall pretty confusing experience.

I'm just not really certain there's a way to implement that while also implementing a way to ignore keydowns on children that shouldn't trigger selections.

100% agree! I don't know how that would work. If selections are only possible when either the search or listbox is in focus, and we remove the visual indicators that suggest otherwise when the filter dropdown is in focus, I think this is a good enough fix for this case.

Thanks for the thorough response, @cee-chen - your suggested fix makes total sense to me! 💃

Heenawter added a commit to elastic/kibana that referenced this issue Aug 4, 2023
…rm (#162067)

Closes #162697

## Summary

This PR adds a few tiny UI improvements to the control creation
elements, including...
  
**Data view picker:**
- Made the `Data view` form title respond to focus as expected
  | <div align="center">Before</div> | <div align="center">After</div> |
  |--------|--------|
| ![Jul-28-2023
15-55-13](https://github.com/elastic/kibana/assets/8698078/c287978d-a54a-4809-a806-5a2caa41cf5d)
| ![Jul-28-2023
15-56-24](https://github.com/elastic/kibana/assets/8698078/8f403c2d-80a5-4fc1-989a-1ecceb056fc9)
|

- Switched to use `EuiInputPopover` rather than `EuiPopover`
- Removed the redundant popover title

  | <div align="center">Before</div> | <div align="center">After</div> |
  |--------|--------|
|
![image](https://github.com/elastic/kibana/assets/8698078/013fc848-3a9a-4280-9b37-6c1f025f3597)
| ![Screenshot 2023-07-28 at 4 16 18
PM](https://github.com/elastic/kibana/assets/8698078/22a2de30-cae1-49d4-9c33-d1537488d08d)
|

**Field picker:**
- Made the `Field` form row title respond to focus as expected for all
of the inner form elements
  | <div align="center">Before</div> | <div align="center">After</div> |
  |--------|--------|
| ![Jul-28-2023
16-06-01](https://github.com/elastic/kibana/assets/8698078/7dd845bc-0476-4b2a-b9b5-efce3c2e2844)
| ![Jul-28-2023
16-07-00](https://github.com/elastic/kibana/assets/8698078/222a9199-e5c2-4180-9501-e31588020855)
|

- Switched the `FieldTypeFilter` to use `EuiInputPopover` rather than
`EuiPopover`
- Removed the redundant title from the `FieldTypeFilter` popover
  | <div align="center">Before</div> | <div align="center">After</div> |
  |--------|--------|
|
![image](https://github.com/elastic/kibana/assets/8698078/007c61db-989b-4615-a36f-5f6307f04aaf)
|
![image](https://github.com/elastic/kibana/assets/8698078/ed7aea0c-d852-4f1c-ae03-14933fa2888a)
|

- Made changes described in
elastic/eui#6627 (comment) so
that, when the field type filter is closed (either via `Esc` or through
the natural tab order), the focus returns to the search field
  | <div align="center">Before</div> | <div align="center">After</div> |
  |--------|--------|
| ![Jul-28-2023
16-12-58](https://github.com/elastic/kibana/assets/8698078/aea49501-1f61-4ae8-bc90-1bacbbc232e7)
| ![Jul-28-2023
16-13-54](https://github.com/elastic/kibana/assets/8698078/8068b090-9cca-427f-bc36-2b9e6b2324f1)
|

- If provided, the initial selected field is now brought to the top of
the list

  | <div align="center">Before</div> | <div align="center">After</div> |
  |--------|--------|
|
![image](https://github.com/elastic/kibana/assets/8698078/2bdad643-d184-4c80-b940-5a73820dc8a5)
|
![image](https://github.com/elastic/kibana/assets/8698078/cda382e2-0e15-48c0-bdbf-c530a77570b8)
|

**Controls display settings:**

- Surrounded the `Minimum width` row with a `div` so that it can receive
the `id` passed down from the `EuiFormRow` and respond to focus as
expected

  | <div align="center">Before</div> | <div align="center">After</div> |
  |--------|--------|
| ![Jul-28-2023
16-31-56](https://github.com/elastic/kibana/assets/8698078/125d2a75-bcec-452c-8682-85de3a44185b)
| ![Jul-28-2023
16-31-20](https://github.com/elastic/kibana/assets/8698078/935a17f1-4adc-4b86-811b-334a42e4627e)
|


### Checklist

- [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))
- [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: Jatin Kathuria <jatin.kathuria@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants