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

fix: Fixed aria-current in BreadcrumbItem #17311

Merged
merged 5 commits into from
Sep 10, 2024

Conversation

guidari
Copy link
Contributor

@guidari guidari commented Sep 2, 2024

Closes #16749

Added prop isCurrent to the logic and also added an href to make the breadcrumb tabbable.

Testing / Reviewing

  • Use VO or JAWS to test the change

@guidari guidari requested a review from a team as a code owner September 2, 2024 12:27
Copy link

netlify bot commented Sep 2, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit b88d68d
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/66dae914eb297700087b2116
😎 Deploy Preview https://deploy-preview-17311--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 2, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b88d68d
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66dae914e99e160008d662ad
😎 Deploy Preview https://deploy-preview-17311--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@2nikhiltom 2nikhiltom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current breadcrumb now gets the focus ring via keyboard navigation and VO is announcing the status 🔥

One questions :
via keyboard navigation: the last/current breadcrumb gets focus ring, and underline.
on mouse hover : underline and cursor pointer is missing on the last/current breadcrumb

Wanted to confirm the expected behaviour, It seems alright to me

Screen.Recording.2024-09-04.at.09.30.16.mov

@guidari
Copy link
Contributor Author

guidari commented Sep 4, 2024

@2nikhiltom

One questions :
via keyboard navigation: the last/current breadcrumb gets focus ring, and underline.

I'm not sure about this one. I guess this is right.

on mouse hover : underline and cursor pointer is missing on the last/current breadcrumb

This one is correct, if the breadcrumb item has isCurrent it shouldn't have cursor pointer.

@alisonjoseph
Copy link
Member

Should the current page have the hover state, this feels odd without the cursor change. 🤔

@annawen1 annawen1 changed the title Fixed aria-current in BreadcrumbItem fix: Fixed aria-current in BreadcrumbItem Sep 5, 2024
@guidari
Copy link
Contributor Author

guidari commented Sep 5, 2024

@carbon-design-system/design

Can anyone help on this one?

@alina-jacob
Copy link
Member

@carbon-design-system/design

Can anyone help on this one?


As per the Interaction guidelines, the current page should receive no interaction, so no hover, no colour change.

image

It technically shouldn't even receive tab stop as per Accessibility guidelines
image

@guidari guidari added this pull request to the merge queue Sep 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 10, 2024
* fix: fixed aria-current

* fix: fixed test to match the new stories

* fix: fixed changes in storybook

---------

Co-authored-by: Nikhil Tomar <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 10, 2024
@guidari guidari added this pull request to the merge queue Sep 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 10, 2024
@alisonjoseph alisonjoseph added this pull request to the merge queue Sep 10, 2024
Merged via the queue into carbon-design-system:main with commit ac90647 Sep 10, 2024
23 checks passed
@carbon-automation
Copy link
Contributor

Hey there! v11.66.0 was just released that references this issue/PR.

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

Successfully merging this pull request may close these issues.

[a11y]: Breadcrumb - With Overflow Menu - JAWS is not announcing the current breadcrumb
5 participants