Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(tabs): update headerWidth story to use unique id for each tab child FE-4440 #4568

Merged
merged 2 commits into from
Nov 26, 2021

Conversation

edleeks87
Copy link
Contributor

fix #4517
fix #4524

Proposed behaviour

The headerWidth story has multiple Tab components with the same tabId which causes axe warnings to be generated

TabHeader now renders a div container and TabTitle renders either a button or a element. This fixes accessibility issues raised against the old implementation relating to focusable children not being read out by screen readers: tabIndex for ValidationIcon is now null and customLayout story has been updated to remove ActionPopover.

Current behaviour

Renders ul and li elements, has multiple accessibility errors on stories
No d.ts files for TabTitle and TabHeader

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Unit tests added or updated if required
  • Storybook added or updated if required
  • Typescript d.ts file added or updated if required

QA

  • Tested in CodeSandbox/storybook
  • Add new Cypress test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

Test all Tabs stories for accessibility errors and to confirm no regressions after refactor away from ul li

The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by codesandbox[bot].

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 17, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 42be1eb:

Sandbox Source
carbon-quickstart Configuration
carbon-quickstart PR

@cypress
Copy link

cypress bot commented Nov 17, 2021



Test summary

1817 0 3 0Flakiness 0


Run details

Project carbon
Status Passed
Commit 4055d865eb
Started Nov 26, 2021 9:39 AM
Ended Nov 26, 2021 9:45 AM
Duration 05:38 💡
OS Linux Debian - 10.9
Browser Chrome 91

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@edleeks87 edleeks87 force-pushed the FE-4440-tab-access-issues branch 2 times, most recently from 9297cb0 to ada2e58 Compare November 17, 2021 16:46
samtjo
samtjo previously approved these changes Nov 18, 2021
@DipperTheDan DipperTheDan marked this pull request as ready for review November 18, 2021 13:49
@DipperTheDan DipperTheDan requested review from a team as code owners November 18, 2021 13:49
DipperTheDan
DipperTheDan previously approved these changes Nov 18, 2021
@edleeks87 edleeks87 marked this pull request as draft November 18, 2021 15:38
@edleeks87 edleeks87 dismissed stale reviews from DipperTheDan and samtjo via 5b2057b November 18, 2021 15:39
@edleeks87 edleeks87 force-pushed the FE-4440-tab-access-issues branch 3 times, most recently from 8644e7c to 713e0a9 Compare November 18, 2021 16:20
DipperTheDan
DipperTheDan previously approved these changes Nov 18, 2021
samtjo
samtjo previously approved these changes Nov 19, 2021
@edleeks87 edleeks87 marked this pull request as ready for review November 22, 2021 11:38
@DlgSHi
Copy link
Contributor

DlgSHi commented Nov 23, 2021

chromatic changes were NOT accepted

@edleeks87 edleeks87 dismissed stale reviews from samtjo and DipperTheDan via 8ebb263 November 23, 2021 14:41
@edleeks87 edleeks87 force-pushed the FE-4440-tab-access-issues branch from fc0449a to 8ebb263 Compare November 23, 2021 14:41
DipperTheDan
DipperTheDan previously approved these changes Nov 23, 2021
samtjo
samtjo previously approved these changes Nov 23, 2021
@harpalsingh
Copy link
Member

I had a look at the before/after on this update and its not a problem for me, the visuals look fine. all good ✅

@edleeks87 edleeks87 dismissed stale reviews from samtjo and DipperTheDan via 15fb064 November 26, 2021 09:23
@edleeks87 edleeks87 force-pushed the FE-4440-tab-access-issues branch 2 times, most recently from 15fb064 to 4055d86 Compare November 26, 2021 09:35
The story has multiple `Tab` components witht the same `tabId` which causes axe warnings to be
generated

fix #4524
…l and li elements

`TabHeader` now renders a `div` container and `TabTitle` renders either a `button` or `a` element.
This fixes accesibility issues raised against the old implementation relating to focusable children
not being read out by screen readers: `tabIndex` for `ValidataionIcon` is now null and
`customLayout` story has been updated to remove `ActionPopover`.

fix #4517
@edleeks87 edleeks87 force-pushed the FE-4440-tab-access-issues branch from 4055d86 to 42be1eb Compare November 26, 2021 10:21
@edleeks87 edleeks87 merged commit f7c8378 into master Nov 26, 2021
@edleeks87 edleeks87 deleted the FE-4440-tab-access-issues branch November 26, 2021 10:30
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 101.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Tab Accessibility violation with header width Tab axe warning when rendering a link
6 participants