Sarka/a11y fixes tooltip #4515
Open
Sarka/a11y fixes tooltip #4515
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook staging is available at https://kiwicom-orbit-sarka-a11y-fixes-tooltip.surge.sh |
|
Size Change: +92 B (+0.02%) Total Size: 459 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
49b9c60
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e431d9a6.orbit.pages.dev |
| Branch Preview URL: | https://sarka-a11y-fixes-tooltip.orbit.pages.dev |
sarkaaa
force-pushed
the
sarka/a11y-fixes-tooltip
branch
2 times, most recently
from
3eafc13
to
96a689b
Compare
sarkaaa
force-pushed
the
sarka/a11y-fixes-tooltip
branch
4 times, most recently
from
4b6aa46
to
8e6a1ff
Compare
sarkaaa
commented
Nov 15, 2024
| > | ||
| <Text>Orbit</Text> | ||
| <Text id="orbit-app">Orbit</Text> |
There was a problem hiding this comment.
I am not sure about this whole file, in case it's necessary, I can revert it back.
sarkaaa
force-pushed
the
sarka/a11y-fixes-tooltip
branch
3 times, most recently
from
6d555a6
to
3e4ac61
Compare
DSil
reviewed
Nov 19, 2024
| | aria-label | `string` | Adds `aria-label` attribute to the rendered tooltip element. Announced by screen readers. | | ||
| | aria-labelledby | `string` | Id(s) of elements that announce the component to screen readers. | | ||
|
|
||
| Mentioned props are optional, but highly recommended to use to ensure the best accessibility experience for all users. |
There was a problem hiding this comment.
Suggested change
| Mentioned props are optional, but highly recommended to use to ensure the best accessibility experience for all users. | |
| The mentioned props are optional, but highly recommended to use to ensure the best accessibility experience for all users. |
|
|
||
| Mentioned props are optional, but highly recommended to use to ensure the best accessibility experience for all users. | ||
|
|
||
| The `aria-label` prop can be used to provide a label for the tooltip that is announced by screen readers. |
There was a problem hiding this comment.
Suggested change
| The `aria-label` prop can be used to provide a label for the tooltip that is announced by screen readers. | |
| The `aria-label` prop can be used to provide a label for the tooltip that is announced by screen readers but is not visible on the screen. |
Maybe we should explain the purpose of this attribute
|
|
||
| The `aria-label` prop can be used to provide a label for the tooltip that is announced by screen readers. | ||
|
|
||
| The `aria-labelledby` prop can reference multiple ids, separated by a space. Value of the attribute is not announced by screen reader, content of connected |
There was a problem hiding this comment.
Suggested change
| The `aria-labelledby` prop can reference multiple ids, separated by a space. Value of the attribute is not announced by screen reader, content of connected | |
| The `aria-labelledby` prop can reference multiple ids, separated by a space. The value of the attribute is not announced by the screen reader, but the content of the referred element is. |
There was a problem hiding this comment.
Suggested change
| The `aria-labelledby` prop can reference multiple ids, separated by a space. Value of the attribute is not announced by screen reader, content of connected | |
| The `aria-labelledby` prop can reference multiple ids, separated by a space. The value of the attribute is not announced by the screen reader, but the content of the referred element(s) is. |
| The `aria-label` prop can be used to provide a label for the tooltip that is announced by screen readers. | ||
|
|
||
| The `aria-labelledby` prop can reference multiple ids, separated by a space. Value of the attribute is not announced by screen reader, content of connected | ||
| element does. |
There was a problem hiding this comment.
If you accept the suggestion above, this can be deleted.
| The `aria-labelledby` prop can reference multiple ids, separated by a space. Value of the attribute is not announced by screen reader, content of connected | ||
| element does. | ||
|
|
||
| The conjugation of these properties allows to provide a content of tooltip component to users of assistive technologies. |
There was a problem hiding this comment.
This is not exactly true. What is the purpose of an aria-label for a tooltip trigger? Is it to announce the tooltip content? Or to announce that it triggers a tooltip?
Honest question, I didn't research, but this sentence is not clear to me.
There was a problem hiding this comment.
When aria-label is filled in, the screen reader announces the value of that attribute. AFAIK based on my research, the value should be the "name of the tooltip" (eg More info). Not sure if it's the desired reply for your question, but feel free to ask/add a note, if it's not clear.
There was a problem hiding this comment.
But if the tooltip trigger is already the text "More info" we don't need an aria label for it to be read. This is from Storybook production. The screen reader clearly announces the text of the trigger and the text of the content, with no aria attributes involved
There was a problem hiding this comment.
Yes, that's true that the "children" text (Cras elementum in this case) is read without and the aria-label is not needed at all.
In case the children element is not readable, but it is (for example) and icon, then the aria-label should be read.
I removed The conjugation of these properties allows... in the end.
|
|
||
| Note that attribute `aria-labelledby` is connected with `id` attribute of the component that is the base for the Tooltip. Without that connection, the `aria-labelledby` attribute is ignored and not read by screen reader. | ||
|
|
||
| The content of attribute `aria-labelledby` is prioritized before the content of `aria-label`. In case `aria-labelledby` is not filled in, |
There was a problem hiding this comment.
Suggested change
| The content of attribute `aria-labelledby` is prioritized before the content of `aria-label`. In case `aria-labelledby` is not filled in, | |
| The content of attribute `aria-labelledby` is prioritized over the content of `aria-label`. |
I believe only this is necessary (the line below can also be deleted).
| The content of attribute `aria-labelledby` is prioritized before the content of `aria-label`. In case `aria-labelledby` is not filled in, | ||
| then the content of `aria-label` is read by screen reader. | ||
|
|
||
| Be sure to include all relevant information on all of the properties that are announced by screen readers. |
There was a problem hiding this comment.
I don't understand what this sentence means 😕
There was a problem hiding this comment.
I'll rewrite it to:
Be sure to include all relevant information that is announced by screen readers.
or
Be sure to fill in all relevant attributes whose values are announced by the screen readers.
My point was to note that all attributes that are announced by screen readers should be filled in to reach the a11y goal.
There was a problem hiding this comment.
But that's not necessarily true. If the tooltip is able to announce itself, maybe we don't need any aria-.
There was a problem hiding this comment.
yes, I get the point (from your comment below).
In that case, I think we can delete this sentence at all. WDYT?
There was a problem hiding this comment.
Yes, I think it's a general accessibility sentence and not really tooltip focused, so we can omit it from here
|
|
||
| The conjugation of these properties allows to provide a content of tooltip component to users of assistive technologies. | ||
|
|
||
| For example, the following code snippet shows how to use these properties: |
There was a problem hiding this comment.
It would be nice to know what is announced by screen readers
There was a problem hiding this comment.
tbh, it's not clear to me what do you mean exactly.
I removed the The conjugation of these... sentence at all, it looks redundant to me in the whole doc specification.
|
|
||
| ### Examples | ||
|
|
||
| 1. Prop `id` in `<Text>` component is not filled in, content of `aria-label` is read by screen reader. Then the content inside the tooltip is read by screen reader. Content of `aria-labelledby` is ignored. |
There was a problem hiding this comment.
This is misleading. The content of aria-label is read because there is no element with the id passed on aria-labelledby. It is not because Text has no id.
It is also important to note that the paragraph inside the content is also announced (although it shouldn't…). So this is a wrong implementation that we currently have and that we will need to fix. But until that happens I feel we should be clear about it, and adjust the documentation when the time comes.
There was a problem hiding this comment.
Can you elaborate on the problem here, please? By "paragraph inside the content" you mean specifically the "Lorem ipsum dolor sit amet" text from the example? Or that the word "paragraph" as an element/role is being announced? Or what is the problem we need to fix?
There was a problem hiding this comment.
The "Lorem ipsum dolor sit amet" is also announced because the tooltip is focused when it's open (it shouldn't, but that's an issue for another moment)
There was a problem hiding this comment.
Isn't that the desired behavior to announce the tooltip content (at least I was under the impression it is, from reading the W3 materials)? Can you share some resources on this, please?
There was a problem hiding this comment.
I have found some examples, and the content is read by a screen reader. Why the content shouldn't be announced by the reader?
https://dequeuniversity.com/library/aria/tooltip
https://inclusive-components.design/tooltips-toggletips/
There was a problem hiding this comment.
The content of the tooltip must be announced, but not the way we are doing it, I suppose. Now with the migration to floating-ui things are a bit better, but in the past they were announced because we forced focus on the element, which should not be the case…
I am just saying that: if we have the content already being announced naturally (even if the implementation isn't perfect), what is the point of the aria-label? What should one put on the aria-label to be announced that is not announced by the content itself?
There was a problem hiding this comment.
yeah, I know what you mean now. In the examples I have found before, aria-label should contain the name of the tooltip (like More info, etc..), but I get the point that aria-label is not needed (?) in this case when the tooltip content itself is read anyway - reference element includes readable text.
On the other side, I'm not sure if we will still get the Required ARIA attributes must be provided violation without having this attribute (and having labelledby, I have to check).
There was a problem hiding this comment.
I get the point that aria-label is not needed (?) in this case when the tooltip content itself is read anyway.
Now I am confused by you 😅
From my understanding, the aria-label/aria-labelledby is not needed when reference element contains understandable text:
but is needed when we use the icon:
But maybe if the icon has accessible text already, aria-label on the Tooltip reference element is not needed at all?
There was a problem hiding this comment.
yes, that's what I mean, sorry for the confusion. :D
There was a problem hiding this comment.
Back to the first comment, I made some changes in the file.
| </Tooltip> | ||
| ``` | ||
|
|
||
| 2. Prop `id` in `<Text>` component is filled in, content of this component (`Hello world`) is read by screen reader. Then the content inside the tooltip is read by screen reader. Content of `aria-label` is not read at all. |
There was a problem hiding this comment.
Suggested change
| 2. Prop `id` in `<Text>` component is filled in, content of this component (`Hello world`) is read by screen reader. Then the content inside the tooltip is read by screen reader. Content of `aria-label` is not read at all. | |
| 2. Prop `id` in `<Text>` component is filled in with the same value passed to `aria-labelledby`. The content of this component (`Hello world`) is read by the screen reader. The value of `aria-label` is not read at all. |
| @@ -0,0 +1,84 @@ | |||
| --- | |||
There was a problem hiding this comment.
Shouldn't this commit be called something like docs: add accessibility docs to Tooltip component?
|
|
||
| Be sure to include all relevant information on all of the properties that are announced by screen readers. | ||
|
|
||
| ### Examples |
There was a problem hiding this comment.
Numbering in your examples is broken, both examples have 1., can you take a look at that, please?
| - /components/tooltip/accessibility/ | ||
| --- | ||
|
|
||
| # Accessibility |
There was a problem hiding this comment.
I will post my comment here because it is connected to multiple places. I am super confused what is the purpose of the aria-label and aria-labelledby props in this component so I have a lot of questions 🙈
-
In the current implementation,
TooltipWrapperhas the rolebutton, is that correct? Do you know why it has this role? I was trying to use screen reader and the "button" announce was always very confusing. -
If there is a text provided as a children, there are no a11y violations - is it enough like this or should the
aria-label(oraria-labelledby) be provided anyway, maybe with prefix "tooltip", that there is a tooltip? All of the violations with tooltip I saw were caused by the triggering (reference) element being icon - is this one of the examples where should thearia-label(oraria-labelledby) be provided or it should really be provided always, e.g. to avoid (double) announce of the content of reference element (I am kinda repeating myself here but you get what I am trying to ask 😅)? Should this be mentioned? The double announce happened to me on booking page with the self-transfer note, will we fix this? -
Have you take a look at the aria-details property? Is it something we should also add here? If I understand correctly, it might be useful when there is more complex content inside the tooltip (e.g. lists) so the content of the element referenced by
aria-detailsis not flattened to a string when presented to assistive technology users. I am asking as that seems to be a common usage of tooltips across our pages.
- Two of three examples are the same, maybe one can be removed? I am not sure what should be the correct behavior with the children and what we need to fix, but if the
aria-label(oraria-labelledby) are not provided and children is text, component is already accessible (or at least it doesn't violate anything) and these props don't need to be added. Should we mention that or is that just adding the expected things to the documentation (and what Daniel said is a bad precedent)? Maybe we could mention what should be passed toaria-labelideally or how should the element(s) being referenced by thearia-labelledbylook like - should it sum up the content of the tooltip? Or just make introduction to tooltip content? Maybe the example can be real (as in the Seat component documentation where it mentions seat for a specific passenger) instead of just lorem ipsum text. Current examples also suggests that thearia-labelledbyneeds to be connected with the reference element but that't not true, no? Maybe we can use it with different element than children in the example?
There was a problem hiding this comment.
-
I've found that the triggering component should be interactive - it was written on more websites, but I can't find it right now. I've found some sources talking about the
<a>element, but AFAIK therole=buttonshould be a general role for this. On the other side, we don't know what devs (will) put inside as a children component, so it might escalate other violation issues (?) -
Example: in case there will be a<button>element as children, then we can get theInteractive controls must not be nested. cc @DSil -
I think it was discussed above in the comment. ⏫
-
No, I haven't checked this aria-details property, tbh. It's hard to say if it makes sense to add this prop, on the other side, according to the articles about tooltips, the tooltip content should be simple, without any images/links/whatever, on the other side, we have various content in our tooltips across the Kiwi pages. 🤔
I'm confused and lost about which approach should be better, tbh. -
Yes, the two examples are the same, as I wanted to describe the further behavior. But I can take a look at it, and polish it, ofc.
| @@ -19,7 +19,7 @@ After adding import into your project you can use it simply like: | |||
| Table below contains all types of the props available in the TooltipPrimitive component. | |||
|
|
|||
| | Name | Type | Default | Description | | |||
| | :------------------- | :---------------------------- | :------- | :------------------------------------------------------------------------------------------------------------------------- | | |||
| | :------------------- | :---------------------------- | :------- | :------------------------------------------------------------------------------------------------------------------------- | --- | | |||
There was a problem hiding this comment.
This table is broken, can you take a look at that, please?
| @@ -188,7 +202,7 @@ export const WithImageInside: Story = { | |||
| }; | |||
|
|
|||
| export const Playground: Story = { | |||
| render: ({ children, content, ...args }) => ( | |||
| render: ({ children, content, "aria-labelledby": ariaLabelledby, ...args }) => ( | |||
There was a problem hiding this comment.
I was thinking, maybe we can fix the stories a bit (applies to all 3 files) - since it's not recommended to use interactive elements inside the tooltip, we would remove the link from there. And we could also either remove the stories with tooltip on icon or add aria- attributes there as it makes more sense to have it there than here in Playground where it's already accessible with text. WDYT?
There was a problem hiding this comment.
yes, I was also considering aligning the stories based on the updated tooltip and our observation.
sarkaaa
force-pushed
the
sarka/a11y-fixes-tooltip
branch
4 times, most recently
from
810823a
to
84ecd4a
Compare
|
Hi guys 👋 Thank you for the exhausting CR. I made some changes based on the comments (excluding SB stories cleanup). Feel free to take a look. Thank you! ❤️ |
sarkaaa
force-pushed
the
sarka/a11y-fixes-tooltip
branch
3 times, most recently
from
e1763c2
to
253d109
Compare
sarkaaa
force-pushed
the
sarka/a11y-fixes-tooltip
branch
from
253d109
to
49b9c60
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New props
aria-labelandaria-labelledbywere added to tooltipPrimitive/tooltip/primitiveModalDialog components. Using these props, we are further to our goal in a11y.Closes https://kiwicom.atlassian.net/browse/FEPLT-2156