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

[Unified Search] Improve accessibility of flex width comboboxes #171439

Closed
wants to merge 5 commits into from

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Nov 16, 2023

Summary

.blur()ing focus from the combobox was done for the custom flex/width behavior, but is an accessibility issue as it strands focus for keyboard and screen reader users (see elastic/eui#7279).

I've worked around this by focusing the dropdown arrow toggle button instead of blurring focus, and modifying the CSS selectors from :focus-within to :has to only expand the combobox width on input focus. This has the added bonus of removing an unnecessary extra onFocus workaround (see elastic/eui#7170).

sr

While here, I also noticed a bug with a disabled cursor appearing that I fixed.

Checklist

- use `:has` rather than `:focus-within` for determining when the combobox should be embiggened, so that the dropdown arrow focus doesn't trigger the increase
- can be reverted once elastic#170716 merges
@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes accessibility: focus Accessibility focus management ci:cloud-deploy Create or update a Cloud deployment v8.12.0 labels Nov 16, 2023
@@ -26,9 +26,13 @@ export const fieldAndParamCss = (euiTheme: EuiThemeComputed) => css`
.euiFormRow {
max-width: 800px;
}
&:focus-within {
// Expand the combobox if the dropdown is open or the input has focus
&:has(input[aria-expanded='true'], input:focus) {
Copy link
Member Author

@cee-chen cee-chen Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note: I'm aware that latest production Firefox currently does not support :has (caniuse link), however it will by release 120 (slated for mid December), and it also works when the experimental flag is enabled.

IMO, this is reason enough to move forward with :has() especially as I see the flex width UX as a progressive enhancement (it's nice to have to show more text, but not a must have).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with moving forward on using the has() selector. Showing more text is exactly the spirit of progressive enhancement, and leaves no one with a less-usable experience. Great call Cee!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Nov 16, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
unifiedSearch 213.7KB 213.8KB +156.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cee-chen cee-chen marked this pull request as ready for review November 16, 2023 23:24
@cee-chen cee-chen requested a review from a team as a code owner November 16, 2023 23:24
@andreadelrio
Copy link
Contributor

andreadelrio commented Nov 17, 2023

Thanks for taking this on @cee-chen. I thought we were going to get rid of this workaround (expand input width on focus) with the release of panelMinWidth in EuiComboBox. @timductive can you confirm what the direction should be here?

@cee-chen
Copy link
Member Author

Oh, I had no idea! That would be even better!! :)

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! Thank you 🥇

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The focus landing on the Open combobox button is a marked improvement for UX. I was able to traverse backwards using they keyboard + screenreader, hear the selected item, and open the combobox in one move. This pattern has a small level of user discovery required, but I feel will be internalized quickly.

@@ -26,9 +26,13 @@ export const fieldAndParamCss = (euiTheme: EuiThemeComputed) => css`
.euiFormRow {
max-width: 800px;
}
&:focus-within {
// Expand the combobox if the dropdown is open or the input has focus
&:has(input[aria-expanded='true'], input:focus) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with moving forward on using the has() selector. Showing more text is exactly the spirit of progressive enhancement, and leaves no one with a less-usable experience. Great call Cee!

@timductive
Copy link
Member

We talked about whether to remove this or not and the answer was that the unified search can keep the flex-width solution as it has another small benefit of showing the currently selected value text

@timductive
Copy link
Member

This seems reasonable to me @cee-chen!

@cee-chen
Copy link
Member Author

another small benefit of showing the currently selected value text

To be honest, knowing now that removing this flex behavior is an option - I'm not sure I consider this benefit enough to keep it in 😅 Especially with recent EuiComboBox plain text changes (that will land in Kibana main with #170716), the selected text will always be visible to users the same way a text input is (via arrow keys):

plain text selection

I'd love to instead move forward with Andrea's proposal of removing the behavior totally combined with panelMinWidth. my 2c is that this is a better and less unexpected/jarring UX than the combobox widths changing dynamically.

@mbondyra
Copy link
Contributor

mbondyra commented Nov 22, 2023

@cee-chen @timductive I implemented the panelMinWidth solution for the same problem in other places in Kibana comboboxes and decided to see how it would look if we choose it instead of merging this PR. Please take a look at the video below and this PR (it's the last commit which I can easily revert if we decide to stick with the solution from this PR.

Nov-22-2023.14-20-48.mp4

@cee-chen
Copy link
Member Author

I love it! You can also use inputProps={{ anchorPosition: 'downRight' }} for the rightmost combobox to have it grow to the left!

@JasonStoltz
Copy link
Member

@timductive @andreadelrio @mbondyra @nickofthyme

Hey all. Cee is OOO for the next couple of weeks and I just wanted to close the loop on this one.

It sounds to me like this combo box will be changing to use a combination of panelMinWidth and singleSelection={{ asPlainText: true }}, rather than the flex width behavior, which effectively solves the long value issues that the flex width behavior was meant to address.

Changing to this new behavior will solve the core accessibility issue that this PR was meant to address, making this PR unnecessary.

For that reason, I'm going to close this PR. If I've misstated anything in the above, please let me know and we will re-open this PR.

@JasonStoltz JasonStoltz closed this Dec 6, 2023
@cee-chen cee-chen deleted the combobox-a11y-ux branch June 3, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility: focus Accessibility focus management ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants