[EuiButtonGroup] Remove radio group markup/behavior #7325

cee-chen · 2023-10-27T19:12:34Z

Summary

See https://lea.verou.me/blog/2022/07/button-group/ - per recent a11y/industry discussion, using <input type="radio"> buttons for single selection/exclusive button groups is no longer recommended.

No functionality should have regressed as a result of this change, although potentially some consumers' test selectors or CSS that were targeting label instead of button will need to be updated.

Other PR notes

FYI, I grabbed this work because it's needed to unblock the link or button service/logic I'm shortly going to be implementing.

As always, I recommend following along by commit - I tried to separate things out atomically to make changes easier to understand.

QA

Mostly regression testing - ensure the following links behave as before / as expected in terms of selection UX, UI appearance, and focus rings for all color and size permutations:

General checklist

Browser QA
- Checked for accessibility including keyboard-only and screenreader modes
- Checked in Chrome, Safari, Edge, and Firefox
  - [ ] Checked in mobile
  ~~- [ ] Checked in both light and dark modes~~
Docs site QA - N/A
Code quality checklist
- Added or updated jest ~~and cypress tests~~
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist
~~- [ ] Updated the Figma library counterpart~~

- topmost prop API will probably want a deprecation period rather than removing it wholesale

- by having it set as a parent conditional `onClick` and spread via `...rest`

- remove `elementProps` entirely

- while still preserving previous focus outline appearance

- with `mathWithUnits`

cee-chen · 2023-10-27T19:35:04Z

src/components/button/button_group/button_group.tsx

@@ -157,7 +159,6 @@ export const EuiButtonGroup: FunctionComponent<Props> = ({
  );

  const typeIsSingle = type === 'single';
-  const nameIfSingle = useGeneratedHtmlId({ conditionalId: name });

  return (
    <fieldset


I left the wrapping <fieldset> (and visually hidden <legend>) intact because it appears to work well and be valid when tabbing directly to buttons. This demo I found online acts as a bare minimum repro and confirms the working behavior:

https://russmaxdesign.github.io/accessible-forms/fieldset-buttons.html

Also @1Copenut I know you mentioned using a <section> in #7101 (comment) but after our discussion in #7316 about regions and MDN's docs on landmark roles, I strongly feel we shouldn't ship with that as a default.

If consumers decide their button group is important enough to wrap in a <section>, great - they can do so themselves around the component, but otherwise IMO <fieldset> and <legend> suffices and best matches what most consumers in Kibana use the component for.

@cee-chen I agree with your assessment about fieldset > legend here. Given the restructuring that's the best grouping and is a lot closer to true intent than a section or region is going to be.

kibanamachine · 2023-10-27T20:15:43Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7325/
Storybook: https://eui.elastic.co/pr_7325/storybook

elasticmachine · 2023-10-27T20:15:47Z

💚 Build Succeeded

Buildkite Build
Commit: 66650a7

1Copenut

👍 LGTM! I did the manual regression testing on all four greenfield browsers in light and dark mode. I also ran through keyboard and screen reader behaviors for the big three SRs and their preferred browsers. This is a nice UX improvement.

1Copenut · 2023-10-27T21:48:14Z

src/components/button/button_group/button_group.tsx

@@ -157,7 +159,6 @@ export const EuiButtonGroup: FunctionComponent<Props> = ({
  );

  const typeIsSingle = type === 'single';
-  const nameIfSingle = useGeneratedHtmlId({ conditionalId: name });

  return (
    <fieldset


@cee-chen I agree with your assessment about fieldset > legend here. Given the restructuring that's the best grouping and is a lot closer to true intent than a section or region is going to be.

`v90.0.0`⏩`v91.0.0-backport.0` ⚠️ While this upgrade pings many teams and has a large code diff, **the majority of the changes are snapshots or tests-related** and do not touch source code, so should theoretically only need a code review and not dedicated QA. The changes in EUI that required a large swathe of these updates are: - **EuiPopover** removed an extra unnecessary `<div>` wrapper on its anchors, which affected many snapshots and a few CSS overrides, which should have been updated - **EuiButtonGroup** now renders `<button>` elements instead of `<input type="radio">` elements for single selection, which affected both snapshots and E2E tests - **EuiSuperDatePicker**'s absolute date input now requires an `Enter` keypress when parsing dates (affected E2E tests) - **EuiComboBox**, when rendered with `singleSelection={{ plainText: 'true' }}`, no longer renders a pill (i.e. text). This combobox type now behaves more like an `EuiFieldText`, where the selection is rendered via input `value` instead. This affected a high amount of E2E tests (both FTR and Cypress), both in terms of updating assertions and changing selections, but should **not** significantly affect user experience - see elastic/eui#7332 for more. --- ## [`v91.0.0-backport.0`](https://github.com/elastic/eui/tree/v91.0.0-backport.0) **This is a backport release only intended for use by Kibana.** - Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs. - `EuiSelectable` now allows configurable text truncation via `listProps.truncationProps` ([#7388](elastic/eui#7388)) - `EuiTextTruncate` now supports a new `calculationDelayMs` prop for working around font loading or layout shifting scenarios ([#7388](elastic/eui#7388)) **Bug fixes** - Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where scrollbar widths were not being accounted for ([#7392](elastic/eui#7392)) ## [`91.0.0`](https://github.com/elastic/eui/tree/v91.0.0) - Updated the background color of `EuiPopover`s in dark mode to increase visibility & contrast against other page/panel backgrounds ([#7310](elastic/eui#7310)) - Memoized `EuiDataGrid` to prevent unneeded re-renders ([#7324](elastic/eui#7324)) - Added a configurable `role` prop to `EuiAccordion` ([#7326](elastic/eui#7326)) - Added a configurable `role` prop to `EuiGlobalToastList` ([#7328](elastic/eui#7328)) - For greater flexibility, `EuiSuperDatePicker` now allows users to paste ISO 8601, RFC 2822, and Unix timestamps in the `Absolute` tab input, in addition to timestamps in the `dateFormat` prop ([#7331](elastic/eui#7331)) - Plain text `EuiComboBox`es now behave more like a normal text field/input. Backspacing will no longer delete the entire value, and selected values can now be double clicked and copied. ([#7332](elastic/eui#7332)) - `EuiDataGrid`'s display settings popover now allows users to clear the "Lines per row" input before typing in a new number ([#7338](elastic/eui#7338)) - Improved the UX of `EuiSuperDatePicker`'s Absolute tab for users manually typing in timestamps ([#7341](elastic/eui#7341)) - Updated `EuiI18n`s with multiple `tokens` to accept dynamic `values` ([#7341](elastic/eui#7341)) **Bug fixes** - Fixed `EuiComboBox`'s `onSearchChange` callback to pass the correct `hasMatchingOptions` value ([#7334](elastic/eui#7334)) - Fixed an `EuiSelectableTemplateSitewide` bug where the `popoverButton` behavior would break if passed a non-DOM React wrapper ([#7339](elastic/eui#7339)) **Deprecations** - `EuiPopover`: deprecated `anchorClassName`. Use `className` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: deprecated `buttonRef`. Use `popoverRef` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: removed extra `.euiPopover__anchor` div wrapper. Target `.euiPopover` instead if necessary ([#7311](elastic/eui#7311)) - Deprecated `EuiButtonGroup`'s `name` prop. This can safely be removed. ([#7325](elastic/eui#7325)) **Breaking changes** - Removed deprecated `euiPaletteComplimentary` - use `euiPaletteComplementary` Instead ([#7333](elastic/eui#7333)) **Accessibility** - Updated `type="single"` `EuiButtonGroup`s to render standard buttons instead of radio buttons under the hood, per recent a11y recommendations ([#7325](elastic/eui#7325)) - `EuiAccordion` now defaults to a less screenreader-noisy `group` role instead of `region`. If your accordion contains significant enough content to be a document landmark role, you may re-configure it back to `region`. ([#7326](elastic/eui#7326)) - Reduced screen reader noisiness when sorting `EuiDataGrid` columns via toolbar ([#7327](elastic/eui#7327)) - `EuiGlobalToastList` now defaults to a `log` role. If your toasts will always require immediate user action, consider (with caution) using the `alert` role instead. ([#7328](elastic/eui#7328)) **CSS-in-JS conversions** - Updated `$euiFontFamily` and `$euiCodeFontFamily` to match Emotion fonts ([#7332](elastic/eui#7332)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

cee-chen added 7 commits October 27, 2023 11:15

Remove label element and radio input render

7e5cac9

Remove/deprecate name prop

2383b0b

- topmost prop API will probably want a deprecation period rather than removing it wholesale

Clean up onChange prop

bbc5b0c

- by having it set as a parent conditional `onClick` and spread via `...rest`

Final props cleanup

add8659

- remove `elementProps` entirely

Clean up now-unnecessary label CSS

f97386a

- while still preserving previous focus outline appearance

[misc] replace unnecessary calc() in center offset

529ce7e

- with `mathWithUnits`

Update snapshots

2a43949

cee-chen requested a review from 1Copenut October 27, 2023 19:33

cee-chen added the accessibility label Oct 27, 2023

cee-chen marked this pull request as ready for review October 27, 2023 19:33

cee-chen requested a review from a team as a code owner October 27, 2023 19:33

cee-chen commented Oct 27, 2023

View reviewed changes

Update downstream (mostly datagrid) tests

7722854

cee-chen force-pushed the button-group-no-input branch from 1e66317 to 191b242 Compare October 27, 2023 19:39

changelog

66650a7

cee-chen force-pushed the button-group-no-input branch from 191b242 to 66650a7 Compare October 27, 2023 19:45

1Copenut approved these changes Oct 30, 2023

View reviewed changes

cee-chen merged commit a66ada1 into elastic:main Oct 30, 2023
1 check passed

cee-chen deleted the button-group-no-input branch October 30, 2023 20:14

tkajtoch mentioned this pull request Nov 7, 2023

Upgrade EUI to v91.0.0 (with backports) elastic/kibana#170716

Merged

cee-chen mentioned this pull request Nov 27, 2023

WIP elastic/kibana#172021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiButtonGroup] Remove radio group markup/behavior #7325

[EuiButtonGroup] Remove radio group markup/behavior #7325

cee-chen commented Oct 27, 2023 •

edited by 1Copenut

Loading

cee-chen Oct 27, 2023 •

edited

Loading

cee-chen Oct 27, 2023

1Copenut Oct 27, 2023

kibanamachine commented Oct 27, 2023

elasticmachine commented Oct 27, 2023

1Copenut left a comment

1Copenut Oct 27, 2023

[EuiButtonGroup] Remove radio group markup/behavior #7325

[EuiButtonGroup] Remove radio group markup/behavior #7325

Conversation

cee-chen commented Oct 27, 2023 • edited by 1Copenut Loading

Summary

Other PR notes

QA

General checklist

cee-chen Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Oct 27, 2023

Choose a reason for hiding this comment

1Copenut Oct 27, 2023

Choose a reason for hiding this comment

kibanamachine commented Oct 27, 2023

elasticmachine commented Oct 27, 2023

💚 Build Succeeded

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Oct 27, 2023

Choose a reason for hiding this comment

cee-chen commented Oct 27, 2023 •

edited by 1Copenut

Loading

cee-chen Oct 27, 2023 •

edited

Loading