Tooltip accessibility improvements #4639
Open
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
Deploying orbit with
|
| Latest commit: |
0572b7c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://586102f4.orbit.pages.dev |
| Branch Preview URL: | https://domi-tooltip.orbit.pages.dev |
|
Storybook staging is available at https://kiwicom-orbit-domi-tooltip.surge.sh Playroom staging is available at https://kiwicom-orbit-domi-tooltip.surge.sh/playroom |
|
Size Change: +60 B (+0.01%) Total Size: 463 kB
ℹ️ View Unchanged
|
|
I was taking look at how we could refactor Tooltip to not wrap the trigger element with the button role and it seems that other component libraries are using e.g. cloning the element or compound components, but this is rather complicated task so I crated task to tackle this in the future as advised https://kiwicom.atlassian.net/browse/FEPLT-2352 I was also taking look at the "ARIA commands must have an accessible name" violation in Radio and Checkbox components but I would say those can be fixed in those components directly without changes needed to Tooltip component. |
domihustinova
commented
Mar 3, 2025
| @@ -16,7 +16,7 @@ const TooltipWrapper = React.forwardRef< | |||
| className={cx( | |||
| "orbit-tooltip-wrapper", | |||
| "max-w-full cursor-auto", | |||
| "focus:outline-none active:outline-none [&_:disabled]:pointer-events-none", | |||
| "focus:outline-offset-1 active:outline-offset-1 [&_:disabled]:pointer-events-none", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
If we adjust the height of this wrapper to be h-fit-content I believe it works well 🤔 can you confirm?
There was a problem hiding this comment.
I've tried your suggestion and it works well for the checkbox.
However, in the radio component we have double-focus and the outline styles look broken
Edit: We also have double focus in the checkbox when it is not disabled 😞
There was a problem hiding this comment.
I think the whole tooltip could be revamped. But not now. For now, I think we can accept the double focus 😕 unless you can think of something else, I'd move forward with the current solution
There was a problem hiding this comment.
@DSil How about setting outline-none for those two classes inside Checkbox and Radio?
Suggested change
| "focus:outline-offset-1 active:outline-offset-1 [&_:disabled]:pointer-events-none", | |
| "focus:outline-offset-1 active:outline-offset-1 [&_:disabled]:pointer-events-none", | |
| "[&:has(.orbit-checkbox-icon-container,.orbit-radio-icon-container)]:focus:outline-none [&:has(.orbit-checkbox-icon-container,.orbit-radio-icon-container)]:active:outline-none", |
WDYT?
There was a problem hiding this comment.
It only fixes a visual problem, right? We still can hit the tab key twice, no?
There was a problem hiding this comment.
Correct. That would require some refactoring in those two components or more refactoring of Tooltip Wrapper.
There was a problem hiding this comment.
Right. I am ok with just fixing the visual issue, yes
DSil
reviewed
Mar 4, 2025
| @@ -16,7 +16,7 @@ const TooltipWrapper = React.forwardRef< | |||
| className={cx( | |||
| "orbit-tooltip-wrapper", | |||
| "max-w-full cursor-auto", | |||
| "focus:outline-none active:outline-none [&_:disabled]:pointer-events-none", | |||
| "focus:outline-offset-1 active:outline-offset-1 [&_:disabled]:pointer-events-none", | |||
There was a problem hiding this comment.
If we adjust the height of this wrapper to be h-fit-content I believe it works well 🤔 can you confirm?
domihustinova
force-pushed
the
domi/tooltip
branch
from
ba5cfe1 to
da99ef6
Compare
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
There was a problem hiding this comment.
Key Issues
The removal of aria-describedby when the tooltip is hidden creates an accessibility issue, as screen readers need this attribute to understand the relationship between elements and their descriptions, ensuring users are aware of additional information.
| @@ -115,7 +115,7 @@ const TooltipPrimitive = ({ | |||
| onFocus={handleIn} | |||
| onBlur={handleOut} | |||
| ref={setReferenceElement} | |||
| aria-describedby={enabled ? id || tooltipId : undefined} | |||
| aria-describedby={shown ? id || tooltipId : undefined} | |||
There was a problem hiding this comment.
🐛 Possible Bug
Changing aria-describedby to only be present when the tooltip is shown creates an accessibility issue. Screen readers need to know about the relationship between the trigger element and its description even when the tooltip is hidden. This ensures users can discover the presence of additional information before interacting with the element.
Suggested change
| aria-describedby={shown ? id || tooltipId : undefined} | |
| aria-describedby={enabled ? id || tooltipId : undefined} |
domihustinova
force-pushed
the
domi/tooltip
branch
from
da99ef6 to
0572b7c
Compare
LLM Analysis of PR ChangesPull Request Feedback: Tooltip accessibility improvements (/pull/4639)Thank you for submitting this pull request to improve the accessibility of the Orbit component library! Here's an analysis of the changes based on the provided diff. 1. Summary of the changes: This pull request encompasses a wide range of accessibility improvements across the Orbit component library, going beyond just Tooltip. The key changes can be summarized as follows:
2. Potential issues or bugs you notice:
3. Suggestions for improvements:
4. Any security concerns:
5. Code quality observations:
Overall Feedback: This is a significant and valuable pull request that demonstrably improves the accessibility of the Orbit component library. The changes show a strong commitment to making the library more inclusive. However, the introduction of breaking changes requires careful consideration of the migration process for users. Addressing the suggestions above, particularly regarding codemods (or migration alternatives), tooltip Thank you for your hard work on these important accessibility improvements! |
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.
Added focus indicator as a visible focus indicator is a requirement for all focusable elements and removing it is a violation of Success Criterion (SC) 2.4.7: Focus Visible (Level A), this includes tooltip trigger elements.
Also fixed stories (e.g. removed links from tooltips as tooltip content is not focusable) and enhanced Accessibility tab.
Closes https://kiwicom.atlassian.net/browse/FEPLT-2247
✨
Description by Callstackai
This PR improves tooltip accessibility by adding a focus indicator and ensuring that tooltip trigger elements are non-interactive components. It also removes links from tooltips and enhances the Accessibility tab.
Diagrams of code changes
sequenceDiagram participant User participant Tooltip participant Button participant ScreenReader User->>Tooltip: Interacts with tooltip trigger Note over Tooltip: Renders with span wrapper<br/>role="button" alt When Tooltip is shown Tooltip->>Button: Renders as non-interactive<br/>using asComponent="div" Tooltip-->>ScreenReader: Announces tooltip content<br/>via aria-describedby end alt When Button is disabled Button->>Tooltip: Applies pointer-events-none end alt When focused Tooltip->>Tooltip: Applies focus styles<br/>with outline-offset endFiles Changed
Buttoncomponent to useasComponent='div'for accessibility.Textinstead ofTextLinkfor accessibility.aria-describedbyto useshowninstead ofenabled.asComponent='div'for accessibility.aria-describedbyto useshowninstead ofenabled.This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.mdx. See list of supported languages.