Tag accessibility #4630
Open
Tag accessibility #4630
+101
−39
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-tab-a11y-fixes.surge.sh Playroom staging is available at https://kiwicom-orbit-sarka-tab-a11y-fixes.surge.sh/playroom |
|
Size Change: +65 B (+0.01%) Total Size: 463 kB
ℹ️ View Unchanged
|
sarkaaa
force-pushed
the
sarka/tab-a11y-fixes
branch
from
d131a70 to
a772c2b
Compare
Deploying orbit with
|
| Latest commit: |
60dec3d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://684974a5.orbit.pages.dev |
| Branch Preview URL: | https://sarka-tab-a11y-fixes.orbit.pages.dev |
sarkaaa
changed the title
sarkaaa
force-pushed
the
sarka/tab-a11y-fixes
branch
4 times, most recently
from
c81df40 to
b85184b
Compare
sarkaaa
commented
Mar 7, 2025
| {onRemove && ( | ||
| <div | ||
| className={cx( | ||
| "orbit-tag-close-container", | ||
| "ms-200 flex rounded-full", | ||
| "pe-200 ps-100 rounded-r-150 flex items-center justify-center", |
There was a problem hiding this comment.
I added ps-100 to make space between outline and close icon when it's focused -> visual reasons. Same for "title" part of the tag, when removable=true.
ps-0
ps-100
There was a problem hiding this comment.
It is very weird that the user is able to click outside of the icon and trigger the onRemove. Can you please check how other design systems approach this? 😕
There was a problem hiding this comment.
My first idea was to have "clickable" only the X icon, but then I was thinking about it, and it could be difficult to click that area on mobile phones. also, it can easily happen that the user will click onClick function instead of onRemove, because there won't be enough space.
I've found some articles about "minimum" target size:
- https://dequeuniversity.com/rules/axe/4.9/target-size
- https://tetralogical.com/blog/2022/12/20/foundations-target-size/
Here is an example where the "dismiss" button is 100% height of the tag. The design is quite different, but it correspond the minimum 24px size according the accessibility rules.
https://carbondesignsystem.com/components/tag/code/ (variant selector - with AI label)
There was a problem hiding this comment.
Alright, I am sold. But I would loudly present this to designers so they can think of an eventual better solution
There was a problem hiding this comment.
I'll write a note to the accessibility compliance doc.
Also, I can see that onRemove and onClick props are used in search - check sourcebot, however, they put the same function in both props. so the functionality is the same for both props, they just wanted to have a cross button in the Tag component (from what I understand). But it's something that should be managed from the UX side, I guess.
https://www.kiwi.com/cz/search/results/brno-cesko/oslo-norsko/anytime/no-return?stopNumber=0%7Etrue
There was a problem hiding this comment.
they put the same function in both props. so the functionality is the same for both props, they just wanted to have a cross button in the Tag component
Yes, but that is not an assumption we can't make for all the cases
sarkaaa
force-pushed
the
sarka/tab-a11y-fixes
branch
8 times, most recently
from
726199d to
b203237
Compare
DSil
reviewed
Mar 11, 2025
...g.ct.tsx-snapshots/visual-Tag-screenshot-Colored-DateTag-Selected-focus-1-Desktop-darwin.png
Outdated
Show resolved
Hide resolved
| {onRemove && ( | ||
| <div | ||
| className={cx( | ||
| "orbit-tag-close-container", | ||
| "ms-200 flex rounded-full", | ||
| "pe-200 ps-100 rounded-r-150 flex items-center justify-center", |
There was a problem hiding this comment.
It is very weird that the user is able to click outside of the icon and trigger the onRemove. Can you please check how other design systems approach this? 😕
LLM Analysis of PR ChangesPull Request Feedback: /pull/4630 - Tag accessibilityHere's an analysis of the provided Git diff for pull request #4630: 1. Summary of Changes: This pull request focuses on improving the accessibility of the The changes can be summarized as follows:
2. Potential Issues or Bugs:
3. Suggestions for Improvements:
4. Security Concerns:
5. Code Quality Observations:
Overall Feedback: This is a positive pull request that enhances the accessibility of the Recommendations:
By addressing these points, this pull request will significantly improve the accessibility of the Orbit |
This commit includes the fix of a11y violation and makes the interactive controls elements as a siblings in the DOM hierarchy. Also, new label labelClose was added to allow aria-label for Close button.
sarkaaa
force-pushed
the
sarka/tab-a11y-fixes
branch
from
1a5a46b to
60dec3d
Compare
LLM Analysis of PR ChangesOkay, let's analyze the provided Git diff for the Orbit Tag component pull request. Pull Request Analysis for /pull/4630: Tag accessibility1. Summary of ChangesThis pull request focuses on improving the accessibility of the Orbit Key changes include:
2. Potential Issues or Bugs
3. Suggestions for Improvements
4. Security ConcernsThere are no immediate security concerns evident in this diff. The changes primarily focus on accessibility and visual presentation, and do not seem to introduce any new attack vectors or vulnerabilities. However, as always, ensure that any user-provided strings (like 5. Code Quality Observations
Overall Feedback: This pull request is a positive step towards improving the accessibility of the Orbit
After addressing these points, the pull request should be in good shape for merging. The changes are well-intentioned and improve the component's accessibility without introducing significant issues. |
DSil
requested changes
Mar 12, 2025
Comment on lines
+109
to
+114
| {...((onClick || onRemove) && { | ||
| role: "button", | ||
| tabIndex: 0, | ||
| onClick, | ||
| onKeyDown: ev => buttonClickEmulation(ev, onClick), | ||
| })} |
There was a problem hiding this comment.
This silences the linter because it can't analyze this part of the code 😄
But actually… looking at this, I noticed a problem with our current implementation.
We are saying that any tag with onRemove is rendered as a button. But we clearly have the dismiss icon separated from the "content", in two separate elements…
So why is a Tag with onRemove but without onClick focusable, clickable, and all, if only the dismiss part is clickable?
You can see this on the default story, the Tag that says "removable". I don't think it is working correctly.
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.
Closes https://kiwicom.atlassian.net/browse/FEPLT-2269