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

[EuiSuperSelect] Refactor to use EuiSelectable #2708

Closed
peteharverson opened this issue Dec 20, 2019 · 5 comments
Closed

[EuiSuperSelect] Refactor to use EuiSelectable #2708

peteharverson opened this issue Dec 20, 2019 · 5 comments

Comments

@peteharverson
Copy link

An accessibility audit on the ML UI has raised several issues related to the EuiSuperSelect control:

  • The rendered <button> element is given role="option" (see [ML] (Accessibility) Anomaly Detection/Anomaly Explorer - ARIA issue and potential JS bug kibana#52378). The "option" role is intended to identify a selectable item in a select list. Authors must ensure elements with role option are contained in, or owned by, an element with the role listbox. It has been suggested that for the button element, you are restricted to the following roles: checkbox, link, menuitem, menuitemcheckbox, menuitemradio, radio, or switch, although I wonder if the listbox role is more suitable here?
<button role="option" type="button" class="euiSuperSelectControl" 
aria-haspopup="true" 
aria-labelledby="undefined y1q6hj0r" 
aria-selected="true">

...More Code...

</button>
  • The button is rendered with an undefined value for aria-labelledby. This looks like it is because src/components/form/super_select/super_select_control.js is using the id property which is undefined.

  • Focus order is lost when exiting the options. After tabbing into the options in the control, and then pressing ESC to close the control or ENTER to select a value, the next TAB should navigate to the next control on the page, but focus returns to the first element on the page (can be reproduced on the EUI Examples page, or see [ML] (Accessibility) Machine Learning - Focus Order and focus visible kibana#50181).

@myasonik
Copy link
Contributor

Like other dropdown-like things, EuiSuperSelect should probably be rewritten using EuiSelectable which should fix all of the a11y issues outlines here. (This would also serve to consolidate implementations of similar components and unify the UX.)

On top of EuiSelectable, EuiFilterGroup's multi-select largely provides:

  1. A dropdown view of the results instead of inline (which is also a requirement for EuiCombobox)

I think the EUI team prefers to keep the EuiSuperSelect 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.

@cchaos
Copy link
Contributor

cchaos commented Jul 14, 2020

It would definitely make sense and be a great re-use of EuiSelectable. (Confused by your second sentence though, think a copy/paste error).

A dropdown view of the results instead of inline (which is also a requirement for EuiCombobox)

This is simply where the component, EuiSuperSelect, decides to place the list. It should not be a part of EuiSelectable.

One concern, or necessity, is that EuiSuperSelect's options can have variable heights right now because it doesn't use Virtualized. This is great for the main use-cases of providing extra description text to the options like in Lens:

Screen Shot 2020-07-14 at 11 43 31 AM

@myasonik
Copy link
Contributor

Confused by your second sentence though, think a copy/paste error

Are you referring to "This would also serve to consolidate implementations of similar components and unify the UX."? I just mean that if all of our list-like components could use the same base component then things like when/how arrow keys work, how focus functions, other keyboard shortcuts would all be the same across the board making it easier for users and a single implementation would also be easier to maintain.

This is simply where the component, EuiSuperSelect, decides to place the list. It should not be a part of EuiSelectable.

That's true but there also needs to be some sort of way for the "button" and the list to communicate with each other. So the two parts can't be completely ignorant of each other either.

EuiSuperSelect's options can have variable heights right now because it doesn't use react-irtualized

EuiSelectable can be extended to do variable heights fairly easily now that we've moved to react-window instead of react-virtualized but it would require defining the heights of each row which I'm not sure if that's acceptable or not. If not, I think it'd still be better to extend EuiSelectable to support non-virtualized, variable height rows rather than maintain two list implementations.

@JasonStoltz JasonStoltz changed the title [EuiSuperSelect] Accessibility issues [Meta][EuiSuperSelect] Accessibility issues Jan 23, 2023
@cee-chen cee-chen changed the title [Meta][EuiSuperSelect] Accessibility issues [EuiSuperSelect] Refactor to use EuiSelectable Apr 17, 2023
@github-actions
Copy link

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

@cee-chen
Copy link
Member

cee-chen commented Apr 5, 2024

Closing this out as not planned. The original a11y issues in the PR description should have been addressed by various EuiSuperSelect PRs:

The complexity of EuiComboBox warrants dogfooding EuiSelectable; I'm not convinced the same is true of EuiSuperSelect (in terms of dev LOE cost/benefit at least).

@cee-chen cee-chen closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2024
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

4 participants