[EuiSuperSelect] Add new `placeholder` API; clean up screen reader accessibility #6630

cee-chen · 2023-03-02T20:21:05Z

Summary

This PR adds a working placeholder prop that accepts either a string or ReactNode (to potentially match inputDisplay, which can also be a ReactNode).

This PR also incidentally cleans up and removes some unnecessary screen reader accessibility that was not quite working as intended. Rationales are in commit messages/code comments.

QA

Go to http://localhost:8030/#/forms/super-select#more-complex
Confirm placeholder renders as expected, and does not render when a value is selected

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs (using @default if default values are missing) and playground toggles
Added documentation
Added or updated jest and cypress tests
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~

- it's not being announced on Tab in any case, because it's outside the button - it's only accessible via SR navigation + it also announces weirdly for when values are missing or react nodes - `aria-haspopup="listbox"` already covers describing the button's function as a select/listbox

- as far as I can tell it's not doing anything meaningful on the popover, and the content was previously not predictable or useful - native `select`s do not have an association with the select label and the dropdown, so I don't think this component needs to either

cee-chen · 2023-03-02T20:25:04Z

src/components/form/super_select/super_select_control.tsx

-        {/*
-          This is read when the user tabs in. The comma is important,
-          otherwise the screen reader often combines the text.
-        */}
-        <EuiScreenReaderOnly>
-          <span id={screenReaderId}>
-            <EuiI18n
-              token="euiSuperSelectControl.selectAnOption"
-              default="Select an option: {selectedValue}, is selected"
-              values={{ selectedValue }}
-            />
-          </span>
-        </EuiScreenReaderOnly>


@1Copenut I removed this SR text in 599a47c for a few reasons:

It's not actually being announced on Tab in any case, because this SR text is outside the button. It's only accessible via SR navigation

It also announces weirdly/badly for when values are missing or react nodes (see snapshots)

aria-haspopup="listbox" already covers describing the button's function as a select/listbox, so all in all I think this information is unnecessary & repetitive, but I'd appreciate your thoughts on this!

I agree, this EuiScreenReaderOnly wasn't being used effectively. Better for us to have visual + semantic button text and let screen readers announce it as a listbox popup button. Let users decide if they're going to open it, then hear the instructions just in time, when it matters most.

cee-chen · 2023-03-02T20:27:25Z

src/components/form/super_select/super_select_control.tsx

-  /**
-   * Creates a semantic label ID for the `div[role="listbox"]` that's opened on click or keypress.
-   * __Generated and passed down by `EuiSuperSelect`.__
-   */
-  screenReaderId?: string;


@1Copenut I removed this ID in 2336109, which was being used by for the dropdown's aria-labelledby attribute, for several reasons:

The content was not predictable or useful as a label - see point 2 above (where the SR text in snapshot reads out badly for empty values or react nodes)

As far as I can tell it's not actually announcing anything meaningful on the popover itself - I couldn't get the popover focus to read out the labelledby at any point

Native selects do not have an association with the select label and the dropdown, so I don't think this component needs to either.

Thoughts?

I agree on all points but the custom div[role="listbox"] will need an accessible label to satisfy axe. I left a comment on the super_select.tsx#347 about swapping it for a reasonable aria-label.

kibanamachine · 2023-03-02T20:45:23Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6630/

1Copenut

I have one requested change and one suggestion/question:

The aria-labelledby you removed was the right call, but is now throwing an axe violation. I offered an alternative solution in comments.
What do you think about making the placeholder text color more like a selected option. I have a feeling we'll get a11y color contrast reports with this lighter gray.

1Copenut · 2023-03-02T22:23:06Z

src/components/form/super_select/__snapshots__/super_select.test.tsx.snap

-            class="emotion-euiScreenReaderOnly"
-            id="euiSuperSelect__generated-id__screenreaderLabelId"
-          >
-            Select an option: , is selected


Agreed, this not a good UX for screen readers.

1Copenut · 2023-03-02T22:29:01Z

src/components/form/super_select/super_select_control.tsx

-        {/*
-          This is read when the user tabs in. The comma is important,
-          otherwise the screen reader often combines the text.
-        */}
-        <EuiScreenReaderOnly>
-          <span id={screenReaderId}>
-            <EuiI18n
-              token="euiSuperSelectControl.selectAnOption"
-              default="Select an option: {selectedValue}, is selected"
-              values={{ selectedValue }}
-            />
-          </span>
-        </EuiScreenReaderOnly>


I agree, this EuiScreenReaderOnly wasn't being used effectively. Better for us to have visual + semantic button text and let screen readers announce it as a listbox popup button. Let users decide if they're going to open it, then hear the instructions just in time, when it matters most.

1Copenut · 2023-03-02T22:37:54Z

src/components/form/super_select/super_select.tsx

@@ -344,7 +349,6 @@ export class EuiSuperSelect<T extends string> extends Component<
          </p>
        </EuiScreenReaderOnly>
        <div
-          aria-labelledby={this.labelledById}


Removing this aria-labelledby is the right call. It also causes axe to throw a violation because the custom div[role="listbox"] needs an accessible name. I experimented locally with adding aria-label="Super select listbox" that satisfied the axe violation and didn't sound horrible.

VoiceOver skips over the container and sets focus on the selected option, so it skips that accessible label and just reads the option + descriptive instructions. If I traverse back to the container, it will read "Super select listbox", which isn't bad.

64d4e8d

I went with Select listbox, omitting super - hope that's okay! My reasoning was that I don't think end users really need to know (or care about) the difference between a native select and a super select.

1Copenut · 2023-03-02T22:39:57Z

src/components/form/super_select/super_select_control.tsx

-  /**
-   * Creates a semantic label ID for the `div[role="listbox"]` that's opened on click or keypress.
-   * __Generated and passed down by `EuiSuperSelect`.__
-   */
-  screenReaderId?: string;


I agree on all points but the custom div[role="listbox"] will need an accessible label to satisfy axe. I left a comment on the super_select.tsx#347 about swapping it for a reasonable aria-label.

cee-chen · 2023-03-02T23:29:48Z

What do you think about making the placeholder text color more like a selected option. I have a feeling we'll get a11y color contrast reports with this lighter gray.

I would prefer not to deviate from our current components with placeholder text, e.g. EuiComboBox:

eui/src/components/combo_box/_combo_box.scss

Lines 117 to 120 in 0d31625

    
           .euiComboBoxPlaceholder, 
        
           .euiComboBoxPill--plainText { 
        
             @include euiFormControlDisabledTextStyle; 
        
           }

I think we need to take the time later to evaluate our placeholder text color contrast across the board / across all form/input elements, not just this one. IIRC WCAG allows lower contrast ratios for disabled elements... but apparently not for placeholder text (https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html) 😕

kibanamachine · 2023-03-06T17:45:28Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6630/

1Copenut

👍 LGTM! Retested briefly with Safari + VO and axe in Chrome. Automated check violation is gone and the listbox announces itself as expected.

cee-chen · 2023-03-07T01:51:20Z

Thanks Trevor! I also created a follow-up issue the placeholder color contrast convo here: #6634

## Summary [email protected] ⏩ [email protected] ## [`76.3.0`](https://github.com/elastic/eui/tree/v76.3.0) - Updated `EuiSkipLink`'s `fallbackDestination` prop to support an array of query selector strings ([#6646](elastic/eui#6646)) **Bug fixes** - Fixed `EuiFlyout` to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) - Fixed `EuiImage`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) - Fixed `EuiCodeBlock`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([#6645](elastic/eui#6645)) ## [`76.2.0`](https://github.com/elastic/eui/tree/v76.2.0) - Added new `renderCustomGridBody` escape hatch rendering prop to `EuiDataGrid` ([#6624](elastic/eui#6624)) **Bug fixes** - Fixed visual listbox focus ring bug on non-searchable `EuiSelectable`s ([#6637](elastic/eui#6637)) - Added a legacy `alert` alias for the `warning` `EuiIcon` type ([#6640](elastic/eui#6640)) - Fixed a type definition incorrectly coming from a dev dependency, which was causing issues for some consuming projects ([#6643](elastic/eui#6643)) ## [`76.1.0`](https://github.com/elastic/eui/tree/v76.1.0) - Added more detailed screen reader instructions to `EuiSelectable`, `EuiSuggest`, `EuiSelectableTemplateSitewide`, `EuiRange`, and `EuiDualRange`. ([#6589](elastic/eui#6589)) - Added new `placeholder` prop to `EuiSuperSelect` ([#6630](elastic/eui#6630)) - Added new `setCellPopoverProps` parameter callback to `EuiDataGrid`'s `renderCellPopover` prop ([#6632](elastic/eui#6632)) **Bug fixes** - Fixed an ARIA attribute in `EuiSelectableList` ([#6589](elastic/eui#6589)) - Fixed `EuiSelectable` to no longer show active selection state or respond to the Up/Down arrow keys when focus is inside the selectable container, but not on the searchbox or listbox. ([#6631](elastic/eui#6631)) --------- Co-authored-by: Tiago Costa <[email protected]>

## Summary [email protected] ⏩ [email protected] ## [`76.3.0`](https://github.com/elastic/eui/tree/v76.3.0) - Updated `EuiSkipLink`'s `fallbackDestination` prop to support an array of query selector strings ([elastic#6646](elastic/eui#6646)) **Bug fixes** - Fixed `EuiFlyout` to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) - Fixed `EuiImage`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) - Fixed `EuiCodeBlock`'s full screen mode to not scroll jump & to preserve body scrollbar width on open ([elastic#6645](elastic/eui#6645)) ## [`76.2.0`](https://github.com/elastic/eui/tree/v76.2.0) - Added new `renderCustomGridBody` escape hatch rendering prop to `EuiDataGrid` ([elastic#6624](elastic/eui#6624)) **Bug fixes** - Fixed visual listbox focus ring bug on non-searchable `EuiSelectable`s ([elastic#6637](elastic/eui#6637)) - Added a legacy `alert` alias for the `warning` `EuiIcon` type ([elastic#6640](elastic/eui#6640)) - Fixed a type definition incorrectly coming from a dev dependency, which was causing issues for some consuming projects ([elastic#6643](elastic/eui#6643)) ## [`76.1.0`](https://github.com/elastic/eui/tree/v76.1.0) - Added more detailed screen reader instructions to `EuiSelectable`, `EuiSuggest`, `EuiSelectableTemplateSitewide`, `EuiRange`, and `EuiDualRange`. ([elastic#6589](elastic/eui#6589)) - Added new `placeholder` prop to `EuiSuperSelect` ([elastic#6630](elastic/eui#6630)) - Added new `setCellPopoverProps` parameter callback to `EuiDataGrid`'s `renderCellPopover` prop ([elastic#6632](elastic/eui#6632)) **Bug fixes** - Fixed an ARIA attribute in `EuiSelectableList` ([elastic#6589](elastic/eui#6589)) - Fixed `EuiSelectable` to no longer show active selection state or respond to the Up/Down arrow keys when focus is inside the selectable container, but not on the searchbox or listbox. ([elastic#6631](elastic/eui#6631)) --------- Co-authored-by: Tiago Costa <[email protected]>

cee-chen added 3 commits March 2, 2023 12:10

Add placeholder prop, tests, and docs

bd0800c

cee-chen requested a review from 1Copenut March 2, 2023 20:21

changelog

f1d22a5

cee-chen commented Mar 2, 2023

View reviewed changes

1Copenut suggested changes Mar 2, 2023

View reviewed changes

[PR feedback] aria-label

64d4e8d

cee-chen requested a review from 1Copenut March 6, 2023 17:22

1Copenut approved these changes Mar 6, 2023

View reviewed changes

cee-chen mentioned this pull request Mar 7, 2023

[Form components] Ensure all placeholder text meets WCAG accessibility color contrast requirements #6634

Open

cee-chen merged commit 393fa6e into elastic:main Mar 7, 2023

cee-chen deleted the superselect/placeholder branch March 7, 2023 01:51

This was referenced Mar 20, 2023

Upgrade EUI to latest v76.2.0 elastic/kibana#153302

Closed

Upgrade EUI to v76.3.0 elastic/kibana#153341

Merged

cee-chen mentioned this pull request Apr 5, 2024

[EuiSuperSelect] Refactor to use EuiSelectable #2708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiSuperSelect] Add new `placeholder` API; clean up screen reader accessibility #6630

[EuiSuperSelect] Add new `placeholder` API; clean up screen reader accessibility #6630

cee-chen commented Mar 2, 2023 •

edited

Loading

cee-chen Mar 2, 2023

1Copenut Mar 2, 2023

cee-chen Mar 2, 2023 •

edited

Loading

1Copenut Mar 2, 2023

kibanamachine commented Mar 2, 2023

1Copenut left a comment

1Copenut Mar 2, 2023

1Copenut Mar 2, 2023

1Copenut Mar 2, 2023

cee-chen Mar 6, 2023

1Copenut Mar 2, 2023

cee-chen commented Mar 2, 2023

kibanamachine commented Mar 6, 2023

1Copenut left a comment

cee-chen commented Mar 7, 2023

[EuiSuperSelect] Add new placeholder API; clean up screen reader accessibility #6630

[EuiSuperSelect] Add new placeholder API; clean up screen reader accessibility #6630

Conversation

cee-chen commented Mar 2, 2023 • edited Loading

Summary

QA

General checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 2, 2023

1Copenut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Mar 2, 2023

kibanamachine commented Mar 6, 2023

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented Mar 7, 2023

[EuiSuperSelect] Add new `placeholder` API; clean up screen reader accessibility #6630

[EuiSuperSelect] Add new `placeholder` API; clean up screen reader accessibility #6630

cee-chen commented Mar 2, 2023 •

edited

Loading

cee-chen Mar 2, 2023 •

edited

Loading