Collapse: fix a11y violation (interactive nested elements) #4553
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-collapse.surge.sh |
|
Size Change: +39 B (+0.01%) Total Size: 459 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
0264f8f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0ff76c0f.orbit.pages.dev |
| Branch Preview URL: | https://sarka-a11y-collapse.orbit.pages.dev |
sarkaaa
force-pushed
the
sarka/a11y-collapse
branch
6 times, most recently
from
8f0502d to
60e7a82
Compare
sarkaaa
changed the title
sarkaaa
changed the title
sarkaaa
force-pushed
the
sarka/a11y-collapse
branch
2 times, most recently
from
2879382 to
2e0e87e
Compare
sarkaaa
commented
Jan 9, 2025
| </div> | ||
| <Stack inline grow={false} align="center" spacing="none"> | ||
| {actions && ( | ||
| <div aria-labelledby={labelID} className="mx-300 flex items-center"> |
There was a problem hiding this comment.
The div is visible only when actions !== null (because of the set x-margin). I also set the aria-labelledby attribute to inform the user about the connection between the action button/link and the current collapse element. I removed the "eslint ignoring comment" as we don't need the onClick function on the div.
There was a problem hiding this comment.
The div is visible only when actions !== null (because of the set x-margin).
Makes sense! although I'm not sure I understand the "set x-margin" part.
I also set the aria-labelledby attribute to inform the user about the connection between the action button/link and the current collapse element.
I'm not sure if this is correct 🤔. The div with labelID only collapses/expands the content whereas actions might trigger a completely different functionality (e.g. clear some collapsed filters).
@DSil I'm interested in your opinion here.
I removed the "eslint ignoring comment" as we don't need the onClick function on the div.
Perfect! The fewer suppressed warnings, the better.
There was a problem hiding this comment.
I checked with @DSil offline, and we don't need the aria-labelledby on the actions wrapper
There was a problem hiding this comment.
Regarding using labelled-by in static div: I think you're correct and we probably should get rid of it. I think
additional info/text about the link (action component) should be set to this component itself? 🤔 but I'm not 100% sure.
NOTE 💡 : Yesterday I've received a newsletter with a link leading to this article about using aria-label, aria-labelledby.
sarkaaa
commented
Jan 9, 2025
sarkaaa
force-pushed
the
sarka/a11y-collapse
branch
from
2e0e87e to
2d95bc1
Compare
sarkaaa
commented
Jan 9, 2025
| test("First Collapse opens on label click and last Collapse opens on arrow button click", async ({ | ||
| mount, | ||
| }) => { | ||
| const component = await mount(<CollapseStory />); |
There was a problem hiding this comment.
I included visual tests of collapsed component on click for wrapper & arrow button within one test, because I wanted to avoid the huge number of screenshots.
sarkaaa
force-pushed
the
sarka/a11y-collapse
branch
from
7203d3c to
135ffa8
Compare
sarkaaa
force-pushed
the
sarka/a11y-collapse
branch
3 times, most recently
from
bb0a0f2 to
c8e076c
Compare
sarkaaa
force-pushed
the
sarka/a11y-collapse
branch
2 times, most recently
from
5578a1f to
cb02521
Compare
packages/orbit-components/src/Collapse/__tests__/index.test.tsx
Outdated
Show resolved
Hide resolved
sarkaaa
force-pushed
the
sarka/a11y-collapse
branch
from
cb02521 to
0264f8f
Compare
sarkaaa
commented
Jan 16, 2025
| ); | ||
|
|
||
| const toggleButton = | ||
| triggerButton === "label" |
There was a problem hiding this comment.
I kept it.each loop and added a condition to distinguish between "label" button and "arrow" button. It was tough to find a way how to distinguish these two elements and avoid flakiness, in the end I used expanded: false option which is available only for arrow button. The label button is targeted byText.
There was a problem hiding this comment.
I knew it was challenging 😏 but well done!!
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.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.tsCloses https://kiwicom.atlassian.net/browse/FEPLT-2186
✨
Description by Callstackai
This PR fixes an accessibility violation by addressing interactive nested elements in the Collapse component.
Diagrams of code changes
sequenceDiagram actor User participant Label participant IconButton participant CollapseComponent participant Content Note over Label,IconButton: Two ways to trigger collapse alt Click Label User->>Label: Clicks label Label->>CollapseComponent: Triggers handleClick() CollapseComponent->>Content: Toggles visibility else Click Icon Button User->>IconButton: Clicks button IconButton->>CollapseComponent: Triggers handleClick() CollapseComponent->>Content: Toggles visibility end Note over CollapseComponent: Updates aria-expanded state Note over Content: Content slides in/outFiles Changed
Closes https://kiwicom.atlassian.net/browse/FEPLT-2186