[EuiButtonGroup] Remove radio group markup/behavior#7325
[EuiButtonGroup] Remove radio group markup/behavior#7325cee-chen merged 9 commits intoelastic:mainfrom
Conversation
- 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`
| const nameIfSingle = useGeneratedHtmlId({ conditionalId: name }); | ||
|
|
||
| return ( | ||
| <fieldset |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
1e66317 to
191b242
Compare
191b242 to
66650a7
Compare
|
Preview staging links for this PR:
|
💚 Build Succeeded
|
1Copenut
left a comment
There was a problem hiding this comment.
👍 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.
| const nameIfSingle = useGeneratedHtmlId({ conditionalId: name }); | ||
|
|
||
| return ( | ||
| <fieldset |
There was a problem hiding this comment.
@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>
Summary
closes #7101
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
labelinstead ofbuttonwill need to be updated.Other PR notes
FYI, I grabbed this work because it's needed to unblock the
link or buttonservice/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
colorandsizepermutations:General checklist
- [ ] Checked in mobile- [ ] Checked in both light and dark modesand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)- [ ] Updated the Figma library counterpart