[EuiButtonGroup] Add support for `EuiToolTip` for button tooltips #7461

davismcphee · 2024-01-14T02:34:24Z

Summary

This PR adds support to EuiButtonGroup for displaying EuiToolTip components as an alternative to the default browser title attribute:

Notes:

Buttons will either use a title attribute containing their label (same as existing behaviour) or a custom tooltip if toolTipContent is passed.
Custom tooltip props are also supported using toolTipProps to allow customizing positions, delays, etc.
Clicking a button dismisses its tooltip until the button is rehovered or refocused, otherwise the tooltip would remain open when a button's state is toggled until manually unfocused.
Some of the style changes are breaking and may require updates to Kibana-specific overrides since supporting EuiToolTip required modifying how some of the CSS selectors work.

Resolves #6313.

QA

General checklist

Browser QA
- Checked in both light and dark modes
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
Docs site QA
- Added documentation
- Props have proper autodocs (using @default if default values are missing) and playground toggles
  ~~- [ ] Checked Code Sandbox works for any docs examples~~
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 - N/A

davismcphee · 2024-01-14T02:48:39Z

src/components/button/button_group/button_group.tsx

          return (
            <EuiButtonGroupButton
-              key={index}
+              key={option.id}


I switched from using index to option.id for the key since using index would cause the wrong tooltips to be displayed when modifying the options array of an existing button group (something we do in Discover).

davismcphee · 2024-01-14T02:53:01Z

src/components/button/button_group/button_group_button.tsx

+
+  const onClickOverride: React.MouseEventHandler<HTMLButtonElement> = (e) => {
+    // Blur the tooltip so it doesn't stick around after click until rehovered/refocused
+    tooltipRef.current?.onBlur();


I'm using onBlur instead of hideToolTip for this since hideToolTip does not unset the internal focus state of the component, causing the tooltip to reappear the next time the component is rendered (e.g. when un-hovering the button, which triggers onMouseOut).

src/components/button/button_group/button_group.tsx

cee-chen · 2024-01-22T23:57:30Z

@davismcphee I know I'm asking for larger/higher level changes, so LMK at any point if you'd prefer us to simply take over the PR instead of going through tedious feedback rounds! We're happy with whatever works for you ❤️‍🔥 This is a super amazing starting point either way!

davismcphee

@cee-chen Thanks for the feedback! I don't mind making the requested changes here as long as you're ok with it taking a couple days for me to get back around to 🙂

About the requested prop changes, the reason I originally went with title as the prop name was so that existing usages in Kibana would automatically start using EuiToolTip on upgrade, but I don't feel strongly about this and would be fine with updating existing usages as needed instead. And in that case, do you think it would make sense to continue passing title down with rest as it was before to continue supporting the existing behaviour, or maybe you have another opinion about how to approach it?

Also this is what I was thinking for updated props, wdyt?

toolTipContent?: EuiToolTipProps['content'];
toolTipProps?: Partial<Omit<EuiToolTipProps, 'content' | 'children'>>;

src/components/button/button_group/button_group.tsx

cee-chen · 2024-01-24T16:32:11Z

toolTipContent and toolTipProps and their typings look perfect - thanks Davis!! ❤️ No rush on this at all, feel free to take all the time you need on this! And if something comes up on your end and you find yourself busy with other work, no worries at all, just give us a ping and we're happy to help out!

cee-chen · 2024-02-05T16:05:54Z

Moving this back to draft for triage purposes while it's still in progress!

…r title attribute

…ent and toolTipProps props for tooltips, and restore default title attribute behaviour

davismcphee · 2024-02-24T06:13:39Z

@cee-chen Well, apparently my couple of days turned into a month... But I finally got around to making the requested changes, and updated the documentation with a new section for button group tooltips. Just in time for us all to be gone to EAH for a week 😄

cee-chen · 2024-03-04T19:59:48Z

Sorry for the wait Davis! I'm planning on reviewing this PR this week. Any objections if I push up changes directly to this branch/PR?

davismcphee · 2024-03-04T23:04:53Z

No problem, and no objections at all!

src/components/button/button_group/button_group_button.tsx

cee-chen · 2024-03-09T07:30:11Z

src/components/button/button_group/button_group_button.tsx

+  const onClickOverride: React.MouseEventHandler<HTMLButtonElement> = (e) => {
+    // Blur the tooltip so it doesn't stick around after click until rehovered/refocused
+    toolTipRef.current?.onBlur();
+    onClick(e);
+  };


[This comment is an explanation for a change I'm pushing up to this PR]

In generally I'm really really not a fan of blurring focus, ever. It's an major accessibility issue for keyboard users (strands them to the top of the page and they have to tab all the way back to wherever they were) and it absolutely shouldn't be a default in EUI.

I get the desire to try and make the UX nicer for mouse users but I simply don't think it's worth it as there's just too many edge cases around it (what happens if a user hovers and clicks before the tooltip animates in? -- they'll literally never see the tooltip content).

I'm removing this logic from EUI, but I'll provide an example in Storybook of how a consumer could implement this on their own via toolTipProps if necessary. Again, I really don't love it personally and I can't recommend it, so the code is mostly just there to show how it could be done.

Change with example Storybook:

00d204a

https://eui.elastic.co/pr_7461/storybook?path=/story/euibuttongroup--with-tooltips

@MichaelMarcialis, while I have you here, I'd be curious what your thoughts are on the UI/UX of EuiToolTip preserving the tooltip on click/button focus vs on hover only.

Hmm...good question. I imagine for the sake of accessibility for keyboard and screenreader users, we'll need to show the tooltips both on hover and focus events, not just hover events. While it could be seen as a small annoyance for mouse-based users, I'm not sure it can be avoided. CCing @1Copenut though, in case he has other thoughts.

Yes, we'll need to show the tooltips on hover and focus events. By removing tooltips from focus events, we'll put ourselves in direct conflict with SC 2.1.1 Keyboard:

All functionality of the content is operable through a keyboard interface without requiring specific timings for individual keystrokes, except where the underlying function requires input that depends on the path of the user's movement and not just the endpoints.

I did a quick test of Cee's Storybook solution. It felt okay from a keyboard navigation perspective once I spent a few minutes gleaning the difference in the middle button tooltip and the dismissable tooltip. We will need to ensure tooltips are announced by associating them with the button like we do for regular button tooltips. Safari + VO didn't announce the tooltip, but that should be easily adjusted.

To be honest if we can get away with it, I'd rather tell consumers "Hey please don't do this, it's an a11y issue" instead and delete that part of the Storybook example entirely 😅

@davismcphee Do you mind chiming in on the impetus for that added code? Was it a UX request from a designer or customer?

Sorry for the delay, catching up on this now. To clarify, this code should not have blurred focus of the button or hid the tooltip on focus. It should have hidden the tooltip after the button was clicked while retaining focus of the button. I was pretty sure I had tested this, but if it did anything else, that was unintentional. I certainly wouldn't want to strand keyboard users after clicking the button.

I also didn't think it was an a11y concern because in my testing the buttons were still announced by the screenreader due to their hidden text content. But while our use case is just showing the button label as a tooltip and hiding the native browser tooltip, I understand that the tooltip content could be different than the hidden text, so maybe there's a concern there.

And the behaviour was not at the request of a designer or customer. I was just trying to more closely match the behaviour of native browser tooltips which become hidden when buttons are clicked. Personally I think keeping the tooltip visible after clicking a button to, for example, bold a line of text may be annoying and obstructive to users (and inconsistent with most other UIs I interact with, e.g. the GitHub comment editor I'm using right now). But I don't feel too strongly about it as long as there's a way for us to hide the tooltip on click if it's requested when I open the Kibana PR.

To clarify, this code should not have blurred focus of the button or hid the tooltip on focus.

Ah, you're totally correct on that, apologies Davis! I think my very tired eyes saw blur() and called it quits 🤦

I also didn't think it was an a11y concern because in my testing the buttons were still announced by the screenreader due to their hidden text content

It feels maybe a bit of a weird gray area to me because the tooltip only applies aria-describedby when the tooltip is visible. So clicking it technically removes the extra description to screen readers as well, although I'm not sure how much it matters functionally. I think I'd still prefer to err on the side of caution and not implement any extra default behavior around hiding the tooltip possible, or at least wait until it's a specific request from a designer.

If no objections I might just go ahead and revert the example in the storybook for now, but it's still here in github history if we need to refer back to it at some point.

That all makes sense to me, and it's something we can address later if it's raised as an issue. Also ++ from my side on removing the storybook. No need to document a pattern you don't endorse 🙂

- props docs copy - simplify tests, import order

- make tooltip conditional so it only renders the extra anchor wrapper if `toolTipContent` exists - tweak conditional styles conditionally to target button vs tooltip anchor as needed - DRY out uncompressed border logic

- but allow consumers to opt in on disabling the `title` if they want to, depending on their copy and use case

- this is not an accessible default and EUI should not be baking this in. I've provided an example of how consumers could override this via their own state if they absolutely had to, although it's still not my favorite and has UX friction if users click super quickly, etc

+ add missing ids to button group subsections for easier linking

cee-chen · 2024-03-13T02:01:29Z

src/components/button/button_group/button_group_button.tsx

+  return toolTipContent ? (
+    <EuiToolTip
+      content={toolTipContent}
+      delay="long"


@MichaelMarcialis One final quick question before merge - any chance I could get your 2c on long being the default delay length for button group tooltips? I know we literally recommend in our docs/guidelines using a long delay for repeated components, but to be honest with you, the long delay feels so long I'm not sure how many users will even see it. Am I way off in wondering that?

@cee-chen: Good question. I've personally always disliked and avoided using the long delayed tooltip unless some rare edge-case scenario demanded it. In my opinion, the default tooltip delay for all circumstances should always be regular and only changed when needed (which I believe is the minority of cases based on my experience).

Gotcha - so should I:

Go ahead and remove the default long delay for button group tooltips?

(In another PR) Should we tweak the documented guidelines we provide around long delays?

I'd say yes on both fronts.

cee-chen

Alrighty, with that, I think this PR is ready to go! Thanks y'all for the wonderful collaboration and teamwork, and a huge shout out again to Davis for his fantastic EUI contributions! 💖

kibanamachine · 2024-03-14T19:08:28Z

Preview staging links for this PR:

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

elasticmachine · 2024-03-14T19:13:01Z

💚 Build Succeeded

Buildkite Build
Commit: 24ee14b

History

💚 Build #1491 succeeded 8598331
💚 Build #1464 succeeded d1c076a
💚 Build #1429 succeeded 2a3872d
💚 Build #1428 succeeded a1a5835
💚 Build #1295 succeeded 0f392d5
💔 Build #1276 failed 4a89259

cc @cee-chen

`v93.3.0`⏩ `v93.4.0` --- ## [`v93.4.0`](https://github.com/elastic/eui/releases/v93.4.0) - Added the following properties to `EuiButtonGroup`'s `options` configs: `toolTipContent`, `toolTipProps`, and `title`. These new properties allow wrapping buttons in `EuiToolTips`, and additionally customizing or disabling the native browser `title` tooltip. ([#7461](elastic/eui#7461)) - Enhanced `EuiResizeObserver` and `useResizeObserver`'s performance to not trigger page reflows on resize event ([#7575](elastic/eui#7575)) - Updated `EuiSuperUpdateButton` to support custom button text via an optional `children` prop ([#7576](elastic/eui#7576)) **Bug fixes** - Fixed `EuiFlyout` to not repeatedly remove/add a body class on resize ([#7462](elastic/eui#7462)) - Fixed `EuiToast` title text to wrap instead of overflowing out of the container ([#7568](elastic/eui#7568)) - Fixed a visual bug with `EuiHeaderBreadcrumbs` with popovers ([#7580](elastic/eui#7580)) **Deprecations** - Deprecated `euiPalettePositive` and `euiPaletteNegative` in favour of a more culturally inclusive `euiPaletteGreen` and `euiPaletteRed` ([#7570](elastic/eui#7570)) - Deprecated all charts theme exports in favor of `@elastic/charts` exports: ([#7572](elastic/eui#7572)) - Deprecated `EUI_CHARTS_THEME_<DARK|LIGHT>` in favor of `<DARK|LIGHT>_THEME` from `@elastic/charts`. ([#7572](elastic/eui#7572)) - Deprecated `EUI_SPARKLINE_THEME_PARTIAL` in favor of `useSparklineOverrides` theme from the kibana `charts` plugin `theme` service. **Accessibility** - Updated `EuiModal` to set an `aria-modal` attribute and a default `dialog` role ([#7564](elastic/eui#7564)) - Updated `EuiConfirmModal` to set a default `alertdialog` role ([#7564](elastic/eui#7564)) - Fixed `EuiModal` and `EuiConfirmModal` to properly trap Safari+VoiceOver's virtual cursor ([#7564](elastic/eui#7564))

davismcphee force-pushed the button-group-tooltips branch from b04829e to 25bbc76 Compare January 14, 2024 02:42

davismcphee changed the title ~~[EuiButtonGroup] Add support for showing EuiTooltip for button titles~~ [EuiButtonGroup] Add support for showing EuiTooltip for button titles Jan 14, 2024

davismcphee force-pushed the button-group-tooltips branch from 25bbc76 to 4a89259 Compare January 14, 2024 02:46

davismcphee commented Jan 14, 2024

View reviewed changes

davismcphee force-pushed the button-group-tooltips branch from ae5a398 to 0f392d5 Compare January 19, 2024 03:22

davismcphee marked this pull request as ready for review January 22, 2024 13:41

davismcphee requested a review from a team as a code owner January 22, 2024 13:41

cee-chen self-requested a review January 22, 2024 18:24

cee-chen reviewed Jan 22, 2024

View reviewed changes

src/components/button/button_group/button_group.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Jan 22, 2024

View reviewed changes

src/components/button/button_group/button_group.tsx Outdated Show resolved Hide resolved

davismcphee commented Jan 24, 2024

View reviewed changes

src/components/button/button_group/button_group.tsx Outdated Show resolved Hide resolved

src/components/button/button_group/button_group.tsx Outdated Show resolved Hide resolved

cee-chen marked this pull request as draft February 5, 2024 16:05

davismcphee added 6 commits February 23, 2024 18:56

Update EuiButtonGroup to use EuiTooltip instead of the default browse…

e1139c0

…r title attribute

Update button group snapshots

d935cfb

Update unit tests and snapshots

2a9d44a

Add unit tests for button group tooltips

d5d6e96

Document new prop defaults and add changelog entry

12f4a69

Update React 16 and 17 snapshots

71a2f05

davismcphee force-pushed the button-group-tooltips branch from 0f392d5 to 71a2f05 Compare February 24, 2024 04:09

davismcphee added 4 commits February 24, 2024 00:10

Switch from title, titleDelay, and titlePosition props to toolTipCont…

5336e36

…ent and toolTipProps props for tooltips, and restore default title attribute behaviour

Revert changes to column_sorting tests

a1a5835

Update changelog entry

fabad0c

Add button group tooltip examples

2a3872d

davismcphee changed the title ~~[EuiButtonGroup] Add support for showing EuiTooltip for button titles~~ [EuiButtonGroup] Add support for showing EuiToolTip for button titles Feb 24, 2024

davismcphee changed the title ~~[EuiButtonGroup] Add support for showing EuiToolTip for button titles~~ [EuiButtonGroup] Add support for EuiToolTip for button tooltips Feb 24, 2024

davismcphee marked this pull request as ready for review February 24, 2024 06:10

cee-chen self-assigned this Mar 7, 2024

Merge branch 'main' into button-group-tooltips

6106f82

cee-chen reviewed Mar 9, 2024

View reviewed changes

src/components/button/button_group/button_group_button.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Mar 9, 2024

View reviewed changes

Misc cleanup

5be0af3

- props docs copy - simplify tests, import order

cee-chen force-pushed the button-group-tooltips branch from ee6bfbd to a5410e3 Compare March 9, 2024 08:00

cee-chen added 4 commits March 9, 2024 00:05

Clean up rendered DOM & CSS approach

3b3761d

- make tooltip conditional so it only renders the extra anchor wrapper if `toolTipContent` exists - tweak conditional styles conditionally to target button vs tooltip anchor as needed - DRY out uncompressed border logic

Revert opinionated default title unset on buttons with tooltips

d2e41ea

- but allow consumers to opt in on disabling the `title` if they want to, depending on their copy and use case

More copy tweaks

d1c076a

+ add missing ids to button group subsections for easier linking

cee-chen force-pushed the button-group-tooltips branch from a5410e3 to d1c076a Compare March 9, 2024 08:07

Revert custom tooltip click UX storybook example

8598331

cee-chen reviewed Mar 13, 2024

View reviewed changes

[design feedback] Regular tooltip delay

24ee14b

cee-chen force-pushed the button-group-tooltips branch from c859317 to 24ee14b Compare March 14, 2024 18:40

cee-chen approved these changes Mar 14, 2024

View reviewed changes

cee-chen enabled auto-merge (squash) March 14, 2024 18:41

cee-chen merged commit f10c46d into elastic:main Mar 14, 2024
6 of 7 checks passed

davismcphee deleted the button-group-tooltips branch March 15, 2024 00:17

cee-chen mentioned this pull request Mar 18, 2024

Upgrade EUI to v93.4.0 elastic/kibana#178890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiButtonGroup] Add support for `EuiToolTip` for button tooltips #7461

[EuiButtonGroup] Add support for `EuiToolTip` for button tooltips #7461

davismcphee commented Jan 14, 2024 •

edited by cee-chen

Loading

davismcphee Jan 14, 2024

davismcphee Jan 14, 2024

cee-chen commented Jan 22, 2024

davismcphee left a comment •

edited

Loading

cee-chen commented Jan 24, 2024

cee-chen commented Feb 5, 2024

davismcphee commented Feb 24, 2024

cee-chen commented Mar 4, 2024

davismcphee commented Mar 4, 2024

cee-chen Mar 9, 2024

cee-chen Mar 9, 2024

MichaelMarcialis Mar 11, 2024

1Copenut Mar 12, 2024

cee-chen Mar 12, 2024 •

edited

Loading

davismcphee Mar 13, 2024

cee-chen Mar 13, 2024 •

edited

Loading

davismcphee Mar 13, 2024

cee-chen Mar 13, 2024

MichaelMarcialis Mar 14, 2024

cee-chen Mar 14, 2024

MichaelMarcialis Mar 14, 2024

cee-chen left a comment

kibanamachine commented Mar 14, 2024

elasticmachine commented Mar 14, 2024

[EuiButtonGroup] Add support for EuiToolTip for button tooltips #7461

[EuiButtonGroup] Add support for EuiToolTip for button tooltips #7461

Conversation

davismcphee commented Jan 14, 2024 • edited by cee-chen Loading

Summary

QA

General checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Jan 22, 2024

davismcphee left a comment • edited Loading

Choose a reason for hiding this comment

cee-chen commented Jan 24, 2024

cee-chen commented Feb 5, 2024

davismcphee commented Feb 24, 2024

cee-chen commented Mar 4, 2024

davismcphee commented Mar 4, 2024

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 Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Mar 13, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 14, 2024

elasticmachine commented Mar 14, 2024

💚 Build Succeeded

History

[EuiButtonGroup] Add support for `EuiToolTip` for button tooltips #7461

[EuiButtonGroup] Add support for `EuiToolTip` for button tooltips #7461

davismcphee commented Jan 14, 2024 •

edited by cee-chen

Loading

davismcphee left a comment •

edited

Loading

cee-chen Mar 12, 2024 •

edited

Loading

cee-chen Mar 13, 2024 •

edited

Loading