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

Label Breadcrumb Navigation and Item List #369

Merged
merged 2 commits into from Feb 2, 2022

Conversation

jkva
Copy link
Contributor

@jkva jkva commented Dec 6, 2021

This PR addresses the following item from PAC's "ARIA-AT App Screen Reader Accessibility Observations" document:

When present, the breadcrumb navigation isn't preceded by a heading, either visible or off-screen/screen-reader-only. The list of breadcrumbs therefore also has no accessible name, as there is no heading to label it by.


Effective changes:

  • Label the breadcrumb nav as "Breadcrumb" via aria-label ; and
  • add visually-hidden "Breadcrumb navigation" heading and aria-labelledby associate it with the ol nav items.

@jscholes the React Bootstrap BreadCrumb component does not support adding a child that's not a breadcrumb item; it will position it inside the unordered list. Hence it does not seem possible to place the h2 inside the nav while it precedes the ol.

The only possible alternative that I can see is to forego the h2 and aria-label the ol directly, which is possible.

@jkva jkva requested a review from jscholes December 6, 2021 08:41
@jscholes
Copy link
Contributor

jscholes commented Dec 6, 2021

@jkva Question: when this component is rendered, what immediately proceeds it? I'm concerned that, as is often the case with in-page/breadcrumb navs, the content straight after the list will incorrectly become a child of the breadcrumb h2, if the list isn't always followed by a heading for the next content.

If there is no guaranteed heading after the breadcrumbs, I'd vote to remove this new h2, which solves the Bootstrap issue as well. The list and nav can maintain their accessible names, and the heading structure won't be disrupted. Not my ideal approach, but it is quite neat.

Otherwise, let's talk about an alternative route forward.

Copy link
Contributor

@jscholes jscholes left a comment

Choose a reason for hiding this comment

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

See comment.

@jkva
Copy link
Contributor Author

jkva commented Dec 7, 2021

@jscholes it's proceeded by the next h2 - "Introduction". So technically this could work. However we'd be diverging from the otherwise regular pattern within the app by not having the h2 as a child of the nav. I've pushed a commit that uses aria-label on the nav and the ol.

@jkva jkva requested a review from jscholes December 7, 2021 08:49
Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

I was wondering, because I looked at the way these breadcrumbs are currently represented in the HTML, and there is an aria-label present. It was added by the react-bootstrap library, even though it's not obvious from looking at the code. So that makes me wonder if the changes here are needed or not. What do you think?

@jscholes
Copy link
Contributor

@alflennik The issue was initially raised based on the state of the rendered mark-up, so it's likely that some form of change is still required. Or, in other words, that whatever Bootstrap is doing is not sufficient and/or correct. However, if you can indicate how Boostrap is behaving, we can definitely adapt this PR if needed. What aria-label does Bootstrap add, and to what?

@evmiguel
Copy link
Contributor

evmiguel commented Feb 1, 2022

@jscholes React-Bootstrap applies the aria-label="breadcrumb" to the nav.

I think @jkva's changes are fine and would love to merge them.

@evmiguel evmiguel merged commit 9eea760 into main Feb 2, 2022
@evmiguel evmiguel deleted the label-breadcrumb-nav-and-list branch February 2, 2022 16:51
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.

None yet

5 participants