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

[EuiComboBox] Rebuild with EuiSelectable #2841

Open
myasonik opened this issue Feb 10, 2020 · 6 comments
Open

[EuiComboBox] Rebuild with EuiSelectable #2841

myasonik opened this issue Feb 10, 2020 · 6 comments

Comments

@myasonik
Copy link
Contributor

myasonik commented Feb 10, 2020

The existing EuiComboBox has a few a11y shortcomings and differs in implementation from EuiSelectable which has been crowned as the future implementation of dropdown-like things. To consolidate implementations of similar components, fix the a11y problems, and unify the UX, EuiComboBox should be rewritten using EuiSelectable.

On top of EuiSelectable, EuiComboBox largely provides:

  1. A clear button (which needs improved contrast from what's there today)
    Picture example (should be 4.5:1)
  2. Buttons with the selected items inside the search field
  3. A dropdown view of the results instead of inline
    Which currently has a minor a11y issue associated with it (Remove EuiComboBox down arrow button from tab order #5024) Popup indicator should be excluded from focus order (Under "Keyboard Interaction" for Combobox)

I think the EUI team prefers to keep the EuiComoboBox component and to wrap EuiSelectable within it but another implementation option is to add a "renderAs" prop (or something along those lines) to EuiSelectable to adjust styling.

This replaces Kibana issues #42988 and #39357

@thompsongl
Copy link
Contributor

thompsongl commented Jan 18, 2022

As part of this work, also mark EuiFilterSelectItem for deprecation (See #5387)

@JasonStoltz JasonStoltz changed the title [EuiComboBox] Rebuild with EuiSelectable [Meta][EuiComboBox] Rebuild with EuiSelectable Jan 23, 2023
jeramysoucy added a commit to elastic/kibana that referenced this issue Mar 30, 2023
…153808)

## Summary

Related to #27749

While the [EuiComboBox
rebuild](elastic/eui#2841) is in progress,
this PR addresses missing aria properties and fixes option announcements
for the `run as user` combo. It additionally adds an aria-label for the
Spaces Navigation EuiSelectable that I found was missing.

### Testing
- Add all sample data
- Edit a non-reserved role
- Turn on a screen reader (NVDA is recommended, but VoiceOver will do)
- Tab through the controls on the Edit role screen. When you arrive in a
combo box you may or may not hear an announcement declaring the name of
the combo box (this is expected, especially with VoiceOver).
- With focus in a combo box press the down arrow key. Verify the options
are announced as you traverse them with the down and up arrow keys.
- If there was success in option announcement, press escape key and
verify that the name of the combo box is announced. You may have to
press escape twice when using NVDA.

Note: It seems to be the consensus that this is best we can do with the
existing implementation of EuiComboBox, which is partly why it is being
rebuilt with the EuiSelectable component.
@cee-chen cee-chen removed the meta label Apr 17, 2023
@cee-chen cee-chen changed the title [Meta][EuiComboBox] Rebuild with EuiSelectable [EuiComboBox] Rebuild with EuiSelectable Apr 17, 2023
@cee-chen
Copy link
Contributor

cee-chen commented May 8, 2023

To add more requested context, EuiSelectable has the best built-in accessibility by far (semantic HTML, keyboard navigation, screen reader announcements) and we should dogfood that a11y instead of rewriting it for the 4+ selection components we use. In this case, EuiComboBox has several reported a11y and perf issues (in the github mentions history - see above) that would likely be resolved by this refactor.

Copy link

github-actions bot commented Nov 5, 2023

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

@JasonStoltz
Copy link
Member

JasonStoltz commented May 14, 2024

Just a quick update. We're putting a 3 month window on completing this issue. I believe it should also solve the following issues:

#7289
#5951
elastic/kibana#27749
elastic/kibana#136527
#7712

EDIT: I think we'll be targeting 8.16 for this one.

@legrego
Copy link
Member

legrego commented Aug 19, 2024

EDIT: I think we'll be targeting 8.16 for this one.

Hey @JasonStoltz, is a target of 8.16 still the goal for this issue?

@cee-chen cee-chen removed their assignment Dec 13, 2024
@cee-chen
Copy link
Contributor

cee-chen commented Dec 13, 2024

Apologies all that I never got around to this. It's been a very hot minute since I looked at either component, but before I go, I'll try to summarize what I recall were the blockers or setup steps needed to move towards this.

  1. EuiSelectable needs to be updated to support isCaseSensitive to match EuiComboBox (right now, searching is never case sensitive)
  2. EuiSelectable's search input needs to be customized a lot to fit EuiComboBox's very custom input (both the multi select badges and single selection mode).
    • It might not even make sense to use EuiSelectable's render prop input at all, but instead figure out some way of passing a custom input to EuiSelectable and having it use that ref/node instead.
  3. EuiSelectable currently does not have any OOTB focus UX for list usage in an EuiInputPopover, which EuiComboBox has in spades (e.g. refocusing search on popover close, etc.). There will need to be some amount of work to get UX parity there, possibly in EuiInputPopover itself.
    • I'm not sure if I'm remembering this correctly, but some amount of compromise in EuiComboBox's current focus and/or keyboard behavior may have to be made as it currently has some gaps in a11y behavior.
    • I have what looks like a very investigatory spike here of that work - no guarantees of how useful it is however, as it was very WIP with no git history or commit message 😅
    • See also [docs] Add example of EuiSelectable in an EuiInputPopover #7683 for more context there
  4. EuiComboBox's onCreateOption behavior doesn't exist at all in EuiSelectable - I'm not sure how much modification to EuiSelectable itself will be necessary to accommodate that. Hopefully very little (other than passing an onBlur or on enter keypress callback), and most of that logic can remain in EuiComboBox.
  5. The EuiFilterSelectItem component should be fully removed once EuiComboBox's options list has been updated to use EuiSelectable's list.

There's probably more than just the above list that will only become apparent once the work for this begins, but starting at the top of the list and working down is probably the way to go. The biggest dev/architectural effort will almost certainly be bullet point 2., as EuiComboBox's input/form control is hands down the most custom/involved thing about it (compared to EuiSelectable)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants