Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle #4056

cchaos · 2020-09-19T22:32:52Z

Replacement for #3099 and closes #3023

... and closes #4036

⚠️ Breaking: Removed EuiToggle & EuiButtonToggle

The EuiToggle component wasn't appropriately handling the accessibility of how a toggle should behave. It was replaced in EUI components with correct implementations and there's little to none usages outside of EUI, so it is safe to remove.

This PR also removes the EuiButtonToggle because it was using the EuiToggle component. Instead, we now support a quick addition of the appropriate aria-pressed via the isSelected prop on all our button components.

Upgrade path

EuiToggle didn't come with any styles or other visual indications. Consider whether you're replicating EuiSwitch, EuiRadio, EuiCheckbox, or EuiButton (as a toggle) behavior and use the appropriate component.
Simple EuiButtonToggle instances should be replaced with EuiButton. Moving label to be the children.
If your EuiButtonToggle is of iconOnly, simply swap to using an EuiButtonIcon.
If your EuiButtonToggle is of isEmpty , simply swap to using an EuiButtonEmpty.

All button replacements should address the toggle functionality in one of the two ways listed above:

If your button changes its readable text, via children or aria-label, then there is no additional accessibility concern.
If your button only changes the visual appearance, you must add aria-pressed passing a boolean for the on and off states. All 3 button types (will) provides a helper prop for this called isSelected.

Re-built the entire EuiButtonGroup component

The two main concepts brought over from #3099 are:

1. Use a `<button aria-pressed="true or false">` for multiple selection groups

This is the preferred method for signifying whether a single button is on or off (pressed). Multi-selection groups are just a visually grouped button set all with their own state but under one legend.

2. Use a `<input type="radio">` for single selection groups

Since only one button can be selected at a time for single selection groups, we make these behave as radio groups by actually using hidden radio groups. However, instead of trying to sync selection of a hidden input with the display, the element rendered by EuiButtonDisplay is a <label> connected to the input as is done in plain HTML.

Both of these button group types are established in the new EuiButtonGroupButton component since they share more props/similarities than differences so it's easier to maintain.

3. Additional required props

In addition to changing the actual buttons that are rendered, a few props are now necessary for the accessibility features to work properly.

legend: Required to label the whole group with a single heading. Will be visibly hidden.
name: Optional for single selections. Applies to the name attribute of the inputs to keep them grouped. This cannot be the same as legend because of naming collisions. Will generate an string by default.
idSelected: Required for single selections because radio groups must have an option always selected.

Other (quick) changes

i. Added `minWidth` prop to EuiButton

There have been consumer situations where the min-width is not ideal and so this new prop allows consumers to quickly override this CSS property with an inline style.

ii. Added `baseClassName` to EuiButtonDisplay

Before, EuiButtonDisplay was only used by EuiButton which made it ok to hard-code the euiButton class, but now it's being used directly by EuiButtonGroupButton which wanted to provide it's own base class that all the variations are built on.

EuiButton still produces all the same class lists it did before, but now EuiButtonDisplay is just more reusable (but still for internal use only).

iii. Updated EuiButton disabled styles to be based on a class and not `:disabled`

Because EuiButton can now render as something other than a <button>, we want to ensure that the disabled styles are applied, so the mixins now are based on .euiButton-isDisabled instead of :disabled.

Checklist

kibanamachine · 2020-09-19T22:54:31Z

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

and other tests

kibanamachine · 2020-10-12T16:39:09Z

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

src/components/button/button_content.tsx

And using it in EuiButtonEmpty and EuiButtonIcon

kibanamachine · 2020-10-14T23:53:54Z

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

kibanamachine · 2020-10-15T14:56:49Z

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

kibanamachine · 2020-10-15T18:59:01Z

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

kibanamachine · 2020-10-15T20:18:31Z

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

myasonik

a11y seems great, docs seem clear, 🚀 ship it!

One possible enhancement (can be ignored, added now, or added in a future issue): of isSelected is passed in, should that change any default styles? Could adjust fill state for you so that consumers don't have to pass in two props of the same value.

src-docs/src/views/button/button_example.js

cchaos · 2020-10-15T20:23:29Z

Could adjust fill state for you so that consumers don't have to pass in two props of the same value.

I had thought of this too, but that's not typically how we want the fill visual to indicate. Usually, fill should be reserved to promote the primary action for a page, not the selected. Though I understand that's a bit conflicty with our group styles, but I also consider that large button groups are also probably the primary action. So 🤷 I think it's better for the consumer to decide.

Co-authored-by: Michail Yasonik <[email protected]>

kibanamachine · 2020-10-15T20:47:06Z

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

chandlerprall · 2020-10-16T14:13:14Z

jenkins test this

chandlerprall · 2020-10-16T14:24:46Z

If we're relying on the documentation to communicate this functionality, I'm not sure it's worth having the custom isSelected prop compared to having devs specify aria-pressed directly. isSelected is also less clear when reading through existing code - my first thought would be that it visually modifies the button, if we force devs to use aria-pressed instead that obfuscation goes away.

kibanamachine · 2020-10-16T14:35:13Z

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

cchaos · 2020-10-16T15:09:35Z

Well actually with the addition of the isSelected prop and prop documentation, we don't have to solely rely on documentation. The prop is now discoverable through IDE auto-completion and the prop comment describes is use-case (and caveat).

Also, this prop name aligns with the EuiButtonGroup's selection prop of isSelected as well, so there's some consistency there. I realized it also simplifies the EuiButtonGroupButton usage, updating now.

The other nice thing is that it gives us a scalable prop to add more functionality as a "toggle" button if we need it in the future.

kibanamachine · 2020-10-16T15:31:25Z

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

chandlerprall

Changes LGTM; pulled & tested locally, including keyboard nav & screen reader support

…#4056) * [EuiButton] add `minWidth` prop * Adding text shift to button and fix DataGrid usage and other tests * [Breaking] Removing EuiToggle * [Breaking] Removed EuiButtonToggle in favor of EuiButton.isSelected

cchaos added 21 commits October 12, 2020 11:46

Moved changes over from old PR

34e804f

Moved changes over from old PR

e4bf21e

Cleanup multi button

fb5fd51

Fix up single button

24a9623

Clean slate with styles

0c3f397

Clean slate with styles

ed50d7a

[EuiButton] add minWidth prop

7875eba

Semi-fix disabled states

4a0f285

Trying to fix styles as EuiButton

c7fb7e3

Styles are working for sizes small & medium in EUI theme

f433aaa

Fix flex and ghost

9ca2a35

Finish up styles for EUI theme

2305f81

Fixed Amsterdam styles

38dcaee

Fixing disabled states

812e5da

Update existing tests for button group

bfccd91

Merging to a single button compontent

972ec07

Merging to a single button compontent. Part II and fixing types

f5f5fc2

Fixing styles and a11y

87fb166

Adding text shift to button and fix DataGrid usage

98969d8

and other tests

[Breaking] Removing EuiToggle

1fdaf8c

[Break] Change functionality of EuiButtonToggle

5edd2fc

cchaos force-pushed the a11y/button_group branch from c8896f0 to 5edd2fc Compare October 12, 2020 16:18

Cleanup

29dff4c

cchaos commented Oct 12, 2020

View reviewed changes

src/components/button/button_content.tsx Show resolved Hide resolved

cchaos requested review from chandlerprall and myasonik October 12, 2020 19:32

cchaos added accessibility breaking change labels Oct 12, 2020

Added isSelected to PropsForButton

90cbd81

And using it in EuiButtonEmpty and EuiButtonIcon

Add isSelected individually not to PropsForButton

79c7400

miukimiu mentioned this pull request Oct 15, 2020

[EuiButtonGroup] Improve interaction states #4142

Merged

4 tasks

Some docs stuff

dca6377

Merge remote-tracking branch 'upstream/master' into a11y/button_group

dd0445a

cchaos changed the title ~~Fix EuiButtonGroup a11y and remove EuiToggle~~ Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle Oct 15, 2020

cl

6c1f946

myasonik approved these changes Oct 15, 2020

View reviewed changes

src-docs/src/views/button/button_example.js Outdated Show resolved Hide resolved

typo

d8b6f1e

Co-authored-by: Michail Yasonik <[email protected]>

Simplify isSelected on EuiButtonGroupButton

49a78c7

chandlerprall approved these changes Oct 16, 2020

View reviewed changes

cchaos merged commit 9e55637 into elastic:master Oct 16, 2020

cchaos deleted the a11y/button_group branch October 16, 2020 16:57

myasonik mentioned this pull request Oct 16, 2020

Deprecating EuiToggle and EuiToggleButton #3099

Closed

10 tasks

thompsongl mentioned this pull request Oct 22, 2020

Upgrade EUI to v30.1.1 elastic/kibana#81499

Merged

snide mentioned this pull request Feb 14, 2021

Attempted import error: 'EuiButtonToggle' is not exported from '@elastic/eui'. #4518

Closed

cjcenizal mentioned this pull request Apr 2, 2021

[EuiButtonGroup] Usability/accessibility enhancements #4684

Closed

cchaos mentioned this pull request Jun 11, 2021

[Amsterdam] Fixing some focus states #4876

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle #4056

Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle #4056

cchaos commented Sep 19, 2020 •

edited

Loading

kibanamachine commented Sep 19, 2020

kibanamachine commented Oct 12, 2020

kibanamachine commented Oct 14, 2020

kibanamachine commented Oct 15, 2020

kibanamachine commented Oct 15, 2020

kibanamachine commented Oct 15, 2020

myasonik left a comment

cchaos commented Oct 15, 2020

kibanamachine commented Oct 15, 2020

chandlerprall commented Oct 16, 2020

chandlerprall commented Oct 16, 2020

kibanamachine commented Oct 16, 2020

cchaos commented Oct 16, 2020

kibanamachine commented Oct 16, 2020

chandlerprall left a comment

Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle #4056

Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle #4056

Conversation

cchaos commented Sep 19, 2020 • edited Loading

Replacement for #3099 and closes #3023

⚠️ Breaking: Removed EuiToggle & EuiButtonToggle

Upgrade path

Re-built the entire EuiButtonGroup component

1. Use a <button aria-pressed="true or false"> for multiple selection groups

2. Use a <input type="radio"> for single selection groups

3. Additional required props

Other (quick) changes

i. Added minWidth prop to EuiButton

ii. Added baseClassName to EuiButtonDisplay

iii. Updated EuiButton disabled styles to be based on a class and not :disabled

Checklist

kibanamachine commented Sep 19, 2020

kibanamachine commented Oct 12, 2020

kibanamachine commented Oct 14, 2020

kibanamachine commented Oct 15, 2020

kibanamachine commented Oct 15, 2020

kibanamachine commented Oct 15, 2020

myasonik left a comment

Choose a reason for hiding this comment

cchaos commented Oct 15, 2020

kibanamachine commented Oct 15, 2020

chandlerprall commented Oct 16, 2020

chandlerprall commented Oct 16, 2020

kibanamachine commented Oct 16, 2020

cchaos commented Oct 16, 2020

kibanamachine commented Oct 16, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

cchaos commented Sep 19, 2020 •

edited

Loading

1. Use a `<button aria-pressed="true or false">` for multiple selection groups

2. Use a `<input type="radio">` for single selection groups

i. Added `minWidth` prop to EuiButton

ii. Added `baseClassName` to EuiButtonDisplay

iii. Updated EuiButton disabled styles to be based on a class and not `:disabled`