Tooltip labeling #3094

KenAJoh · 2024-08-14T06:29:04Z

Description

Resolves #3067

Really liked how MUI handled this interaction:
https://mui.com/material-ui/react-tooltip/#accessibility

By default now, the child element (i.e Button that Tooltip wraps around) will get title-attrb set to the Tooltip content when closes. When open, it will remove title and use aria-describedby. By doing this the Tooltip will act as an extra description for the element, without overriding the button-content itself.

If the user sets describeChild={false} now, the child element will instead get aria-label set to the Tooltip-content. This will remain static between open-states. By doing this the Tooltip will always act as the label/name for the child-element. This is especially useful for Icon-buttons and similar elements.

After merge i will update Tooltip docs with some notes on this prop and how to use it.

Discussion

I have now set the prop describeChild to default True, closer to todays implementation to reduce breaking changes. Issue is it still adds a small potential "breaking" change since

title attrb is added

Wrong use of tooltip today, example those who print "kopiert" or a state update in the tooltip today will incorrectly label the element by default
Might break some tests based on how they were locating elements before (low chance i recon)

aria-describedby is not merged with child-prop

If child-element has aria-describedby, it will not merge with Tooltip aria-describedby. Might argue this shouldn't have been implemented in the first place, but might break some tests

Should `describeChild` be default false?

Since adding this change might have some breaking changes anyways, should it just be set to default false? All current Tooltip child-elements will get an aria-label with the Tooltip contents, but might argue that would be an improvement all around except in a few cases. Some tests might break, and some teams will have to update their Tooltip. Does this change fit into one of those small major-updates we have discussed?

changeset-bot · 2024-08-14T06:29:07Z

🦋 Changeset detected

Latest commit: 8f7d4d5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@navikt/ds-react	Major
@navikt/ds-css	Major
@navikt/ds-tokens	Major
@navikt/ds-tailwind	Major
@navikt/aksel-icons	Major
@navikt/aksel	Major
@navikt/aksel-stylelint	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-08-14T06:33:10Z

Storybook demo

Endringer til review: 5

b5c7cba5e | 88 komponenter | 139 stories

JulianNymark · 2024-08-14T07:17:37Z

I'll have to check more when I get to the office, but looks good so far. Also if the choice is between breaking change anyway vs breaking change + improvements I'd go for the latter 🙌

larseirikhansen · 2024-08-14T08:09:26Z

Do i understand this correct: The question is whether describeChild should be false or true by default on Tooltip?

If describeChild is set to true by default, it's less breaking:

a button "kopier meg" with a (bad) tooltip "kopiert!" will read "kopier meg kopiert!", so you also get the text content of the button read. Should be used when the button has text content.

If describeChild is set to false by default, it potentially has more breaking changes:

a button "kopier meg" with a (bad) tooltip "kopiert!" will only read "kopiert!", so it high jacks the screen reader content.

KenAJoh · 2024-08-14T08:11:10Z

Do i understand this correct: The question is whether describeChild should be false or true by default on Tooltip?

If describeChild is set to true by default, it's less breaking:

a button "kopier meg" with a (bad) tooltip "kopiert!" will read "kopier meg kopiert!", so you also get the text content of the button read. Should be used when the button has text content.

If describeChild is set to false by default, it potentially has more breaking changes:

a button "kopier meg" with a (bad) tooltip "kopiert!" will only read "kopiert!", so it high jacks the screen reader content.

Yep thats a perfect summary of change/issue 💯

HalvorHaugan · 2024-08-14T09:39:06Z

@navikt/core/react/src/tooltip/Tooltip.tsx

+   * Important: This will default to false in future major-versions
+   * @default true
+   */
+  describeChild?: boolean;


Is "describesChild" clearer? (Added "s")

I notice the comment for this will then have lied about the future
"This will default to false in future major-versions" (so we should remove that 😅 )

@navikt/core/react/src/tooltip/Tooltip.tsx

@navikt/core/react/src/tooltip/tooltip.stories.tsx

aksel.nav.no/website/pages/eksempler/tooltip/labeling.tsx

HalvorHaugan · 2024-08-14T10:24:43Z

My only "concern" about setting it to false by default, is that devs might not remember (or know) to change it to true when needed. Having it true by default will sometimes lead to repeated info for screen readers, but at least they will not miss any info? But I guess that's an edge case.

Also, what about voice control? With describeChild=false it would be very important that the text in the Tooltip starts with the name text as the button does. (Btw, how is a11y assured for voice control users when only an icon is visible?)

KenAJoh · 2024-08-14T11:08:33Z

Having it true by default will sometimes lead to repeated info for screen readers, but at least they will not miss any info? But I guess that's an edge case.

Based on the implementations looked through, i would argue that more info is left out for the screen-readers today with the avid use of Tooltip around non-interactive elements. By setting default to false, those cases would be "fixed" to a certain degree. But i guess it's a give and take situation where both options with either default true or false each have some positives and negatives. Adding some more specific documentation with a post or two on Slack while keeping default to true might be the least intrusive tho.

Also, what about voice control? With describeChild=false it would be very important that the text in the Tooltip starts with the name text as the button does. (Btw, how is a11y assured for voice control users when only an icon is visible?)

Good question, i can do some testing and ask Morten 👍

KenAJoh · 2024-08-14T11:54:45Z

@HalvorHaugan Did some testing with Mac voicecontrols:
Sandbox for reference

Screen.Recording.2024-08-14.at.13.46.28.mov

For icons using title or aria-label acts the same
For buttons with text and aria-label, aria-label takes precedence
For buttons with text and title, text takes precedence

Since its shows a tooltip with the "accessible name" that is needed to trigger button i can't see it being a problem. Unless... the text is super long. As for that case, there is a setting where instead of labels, each element gets a number to remove this problem all together.

Screen.Recording.2024-08-14.at.13.52.48.mov

This is ofc only based on my testing so will need to talk to someone more experienced for how this is actually done.

KenAJoh · 2024-08-15T06:59:18Z

As long as the user follows this guideline: https://www.w3.org/TR/WCAG22/#label-in-name when combining a visual label with a tooltip (aria-label) it should pass requirements. This is not something we can control, but will need to document on the Tooltip documentation.

HalvorHaugan · 2024-08-15T07:35:06Z

As long as the user follows this guideline: https://www.w3.org/TR/WCAG22/#label-in-name when combining a visual label with a tooltip (aria-label) it should pass requirements. This is not something we can control, but will need to document on the Tooltip documentation.

Yes, I think my point was that it's not intuitive that the tooltip "overrides" the content by default. But I guess it's fine since this is typically not a problem based on how it's normally used, combined with good docs 👍

aksel.nav.no/website/pages/eksempler/tooltip/labeling.tsx

HalvorHaugan · 2024-08-16T12:40:39Z

aksel.nav.no/website/pages/eksempler/tooltip/placement.tsx

+      <Tooltip content="Skriv ut dokument" describesChild>
+        <Button icon={<ArrowUpIcon aria-hidden />} />
      </Tooltip>
-      <Tooltip content="Skriv ut dokument" placement="right">
-        <Button icon={<ArrowRightIcon title="demo knapp" />} />
+      <Tooltip content="Skriv ut dokument" placement="right" describesChild>
+        <Button icon={<ArrowRightIcon aria-hidden />} />
      </Tooltip>
-      <Tooltip content="Skriv ut dokument" placement="bottom">
-        <Button icon={<ArrowDownIcon title="demo knapp" />} />
+      <Tooltip content="Skriv ut dokument" placement="bottom" describesChild>
+        <Button icon={<ArrowDownIcon aria-hidden />} />
      </Tooltip>
-      <Tooltip content="Skriv ut dokument" placement="left">
-        <Button icon={<ArrowLeftIcon title="demo knapp" />} />
+      <Tooltip content="Skriv ut dokument" placement="left" describesChild>
+        <Button icon={<ArrowLeftIcon aria-hidden />} />
      </Tooltip>


Shouldn't describesChild be false in these cases? 🤔 (Same in some of the other examples)

Looks like i flipped it when adding it to demo 🙈 I updated defaultValue to false since this will be a breaking change anyways, and updated the examples 👍

Did you forget to update aksel.nav.no/website/pages/eksempler/togglegroup/with-tooltip.tsx?

Messed up the rebase 💥

Co-authored-by: Halvor Haugan <83693529+HalvorHaugan@users.noreply.github.com>

… into tooltip-describeChild

HalvorHaugan · 2024-08-26T14:09:46Z

.changeset/little-files-cough.md

Major?

Text should be updated.

👍 Updated to major and added a TODO. Will write a section for v7 here https://aksel.nav.no/grunnleggende/kode/migrering.

For now i will create a feature-branch we can merge v7 changes into and change the base of this branch to that one.

* Tooltip labeling (#3094) * feat: Better labeling of Tooltip * feat: Better keyshortcuts for screenreaders in Tooltip * memo: Updated story * bug: Better error logging for example-parsing * refactor: describeChild -> describesChild * fire: Removed extra story * bug: removed extra tabIndex * memo: Update Tooltip demo description * Update aksel.nav.no/website/pages/eksempler/tooltip/labeling.tsx Co-authored-by: Halvor Haugan <83693529+HalvorHaugan@users.noreply.github.com> * refactor: Updated defaultValue for describesChild * bug: removed Tooltip prop from ToggleGroup --------- Co-authored-by: Halvor Haugan <83693529+HalvorHaugan@users.noreply.github.com> * Tailwind: Fixed 'screen' -> 'screens' bug to properly adopt breakpoints. (#3119) * 💥 Screen -> Screens in tailwind-config * refactor: Adopt updated screens-config in Aksel.nav.no project * Icons: Removed old and renamed icons (#3120) * 💥 Removed renamed icons * ErrorSummary: Add fallback for heading + better focus handling (#3140) Co-authored-by: Ken <ken.aleksander@gmail.com> Co-authored-by: Ken <26967723+KenAJoh@users.noreply.github.com> Co-authored-by: Halvor Haugan <83693529+HalvorHaugan@users.noreply.github.com> * feat: Added composeEventHandlers to onFocus in ErrorSummary * memo: Removed inaccurate description for tooltip demo * feat: Omitted tabIndex from ErrorSummary --------- Co-authored-by: Ken <26967723+KenAJoh@users.noreply.github.com> Co-authored-by: Ken <ken.aleksander@gmail.com>

KenAJoh added 4 commits August 14, 2024 08:01

feat: Better labeling of Tooltip

dafef75

feat: Better keyshortcuts for screenreaders in Tooltip

d0909f9

memo: Updated story

35611f0

memo: changeset

d5e2a94

KenAJoh added diskusjon 🧐 klar for gjennomgang 🔍 labels Aug 14, 2024

bug: Better error logging for example-parsing

7f42652