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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1b60ca8
[misc] Fix draggable disabled cursor CSS
cee-chen Nov 16, 2023
5c306c8
[a11y] Focus the toggle dropdown button instead of entirely stranding…
cee-chen Nov 16, 2023
596b377
Restore behavior of reducing the combobox width on selection
cee-chen Nov 16, 2023
4d5d1a1
Remove now-unnecessary extra `onFocus` handler
cee-chen Nov 16, 2023
0798463
[TODO] input click workaround
cee-chen Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/plugins/unified_search/public/filter_bar/filter_editor/phrase_value_input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ class PhraseValueInputUI extends PhraseSuggestorUI<PhraseValueInputProps> {
onChange={([newValue = '']) => {
onChange(newValue);
setTimeout(() => {
// Note: requires a tick skip to correctly blur element focus
this.inputRef?.blur();
// Note: requires a tick skip to correctly move focus from the input to the combobox toggle
this.comboBoxWrapperRef.current
?.querySelector<HTMLButtonElement>('[data-test-subj="comboBoxToggleListButton"]')
?.focus();
});
}}
onSearchChange={this.onSearchChange}
Expand Down
12 changes: 4 additions & 8 deletions src/plugins/unified_search/public/filters_builder/filter_item/field_input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,13 @@ export function FieldInput({ field, dataView, onHandleField }: FieldInputProps)
onFieldChange(newValues);

setTimeout(() => {
// Note: requires a tick skip to correctly blur element focus
inputRef?.current?.blur();
// Note: requires a tick skip to correctly move focus from the input to the combobox toggle
comboBoxWrapperRef.current
?.querySelector<HTMLButtonElement>('[data-test-subj="comboBoxToggleListButton"]')
?.focus();
});
};

const handleFocus: React.FocusEventHandler<HTMLDivElement> = () => {
// Force focus on input due to https://github.com/elastic/eui/issues/7170
inputRef?.current?.focus();
};

return (
<div ref={comboBoxWrapperRef}>
<EuiComboBox
Expand All @@ -101,7 +98,6 @@ export function FieldInput({ field, dataView, onHandleField }: FieldInputProps)
isClearable={false}
compressed
fullWidth
onFocus={handleFocus}
data-test-subj="filterFieldSuggestionList"
singleSelection={SINGLE_SELECTION_AS_TEXT_PROPS}
truncationProps={MIDDLE_TRUNCATION_PROPS}
Expand Down
8 changes: 6 additions & 2 deletions src/plugins/unified_search/public/filters_builder/filter_item/filter_item.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 Author

@cee-chen cee-chen Nov 16, 2023 •
edited
Loading

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!

flex-grow: 4;
}
input {
width: 100% !important; // TODO: Remove this once latest EUI upgrade merges
}
`;

export const operationCss = (euiTheme: EuiThemeComputed) => css`
Expand All @@ -48,7 +52,7 @@ export const actionButtonCss = (euiTheme: EuiThemeComputed) => css`
`;

export const disabledDraggableCss = css`
&.euiDraggable .euiDraggable__item.euiDraggable__item--isDisabled {
&.euiDraggable .euiDraggable__item-isDisabled {
cursor: unset;
}
`;