Skip to content
New issue

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

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

Already on GitHub? # to your account

Correct Aria / Markup for a current page in Breadcrumb Item #67

Merged
merged 3 commits into from
Dec 10, 2023
Merged

Correct Aria / Markup for a current page in Breadcrumb Item #67

merged 3 commits into from
Dec 10, 2023

Conversation

roybarber
Copy link
Contributor

Adding in the missing aria-current attribute for a current page. The current page should also be a blank link (linking to itself)

Example:
https://www.w3.org/WAI/ARIA/apg/patterns/breadcrumb/examples/breadcrumb/

@garyb1
Copy link
Contributor

garyb1 commented Dec 6, 2023

Good idea @roybarber.

I wasn't sure if the href should have "/" or be empty. I vaguely remember a markup validation warning for IE7 😆.

It might be good to enforce the styling here with aria-current for the active breadcrumb. See the screenshot below. The last item will now be a link so the same styling won't be there.

two breadcrumbs, one with the old implementation with a span  rendering white text, and the second rendering a breadcrumb with aria-current in an anchor element

@roybarber
Copy link
Contributor Author

Good point. I'll do a quick non link style for it.

Just an empty href makes the page link to itself. If we placed a "/" then it would link back to the home page.

@garyb1
Copy link
Contributor

garyb1 commented Dec 6, 2023

@roybarber you are correct. Not sure why but I was thinking of home link. Ignore me! 🤣

@roybarber
Copy link
Contributor Author

Updated the PR to take match the current style, with a matching hover state

@markteekman
Copy link
Member

@roybarber thanks for taking the time to make this pull request! There are actually multiple valid ways in which we can markup a breadcrumb component. We can have the last item be a <span> (like it currently is), which could/should in theory be further enhanced by changing it to <span aria-current="page">. Another valid approach is to omit the current item, if the breadcrumbs are followed by the h1 of the page. And then there's your approach, which is also valid and used in many examples online. Personally though, I don't like having links that refer to themselves using a blank link. The purpose of a link is to link the user somewhere else in the website (or to a certain anchor point). @garyb1 it would be even more 'confusing' styling the link as if it was no link at all. We should then ask ourselves if it should be something other then a link and in that case, a span makes more sense. So I'd suggest we change the current item from <span> to <span aria-current="page"> to make it more complete 🙂

@roybarber
Copy link
Contributor Author

@markteekman No problem, like anything A11y theres many ways to skin a cat, ive updated the PR to take into consideration your comments :)

@garyb1
Copy link
Contributor

garyb1 commented Dec 8, 2023

Fair and valid points @markteekman & @roybarber ... Yeah, there is a few ways of looking at it like you said. Was just pointing out that the styling would be different that is all.

Just looking at how the GOV UK team do it for their breadcrumbs. They leave the link exposed and don't leave the current item as text, which is interesting. Although they do explicitly say to use the breadcrumbs component only when you need to help users understand and move between the multiple levels of a website. it seems that omitting is also done on some pages also it so that is also another option.

A span element with aria-current="page" is perfectly valid, and similar to what the USWDS Breadcrumb component implemented.

@markteekman
Copy link
Member

@garyb1 @roybarber yeah, there are certainly more ways to solve this, even to take in consideration when to use this component. That's what can make accessibility in general complex sometimes, or lead to wrong implementations. Although I think we've got a nice upgrade right here! Maybe in the future it may be worthwhile to make the current item optional. For now though, thanks for your thoughts 🙂

@markteekman markteekman self-assigned this Dec 10, 2023
@markteekman markteekman merged commit 077f5ac into incluud:main Dec 10, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants