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

Shortcode Breadcrumbs not marked up as a nav and decorative elements not hidden from screen reader users #20680

Open
9 of 11 tasks
amberhinds opened this issue Sep 26, 2023 · 5 comments

Comments

@amberhinds
Copy link

  • I've read and understood the contribution guidelines.
  • I've searched for any related issues and avoided creating a duplicate issue.

Please give us a description of what happened

The breadcrumbs output by the shortcode is not wrapped in a labeled nav tag and the divider elements (which are decorative) are not hidden from screen readers.

To Reproduce

Step-by-step reproduction instructions

  1. Create a page that is a child page of another.
  2. Add the shortcode [wpseo_breadcrumb] in a shortcode block.
  3. Inspect the shortcode

Expected results

This is the expected code:

<div class="entry-content alignfull wp-block-post-content has-global-padding is-layout-constrained wp-block-post-content-is-layout-constrained">
   <nav aria-label="breadcrumbs">
      <span><a href="http:https://sandbox.local/">Home</a></span><span aria-hidden="true"> » </span><span><a href="http:https://sandbox.local/sample-page/">Sample Page</a></span><span aria-hidden="true"> » </span><span class="breadcrumb_last" aria-current="page">Child Page</span>
</nav>
</div>

Actual results

This is the code currently being output:

<div class="entry-content alignfull wp-block-post-content has-global-padding is-layout-constrained wp-block-post-content-is-layout-constrained"><span><span><a href="http:https://sandbox.local/">Home</a></span> » <span><a href="http:https://sandbox.local/sample-page/">Sample Page</a></span> » <span class="breadcrumb_last" aria-current="page">Child Page</span></span>
</div>

Other Notes

This request to switch to using list markup would also be a great accessibility enhancement but is not as urgent as using the proper nav tag.
#5055

Technical info

  • If relevant, which editor is affected (or editors):
  • Block Editor
  • Gutenberg Editor
  • Elementor Editor
  • Classic Editor
  • Other: It's possible it impacts all instances if you reuse this code beyond the shortcode.
  • Which browser is affected (or browsers):
  • Chrome
  • Firefox
  • Safari
  • Other: All

Used versions

  • Device you are using:
  • Operating system:
  • PHP version:
  • WordPress version:
  • WordPress Theme:
  • Yoast SEO version:
  • Gutenberg plugin version:
  • Elementor plugin version:
  • Classic Editor plugin version:
  • Relevant plugins in case of a bug:
@jeroenrotty
Copy link
Contributor

Thank you @amberhinds for reaching out.

I can see the same happening for our Breadcrumbs block. There is no nav HTML element but a div element when the block is used:
output

I'm asking internally how we can improve.

@afercia
Copy link
Contributor

afercia commented Sep 26, 2023

@amberhinds thanks for your feedback. Quickly chiming in to add my 2 cents.
I think Yoast already improved a little the breeadcrumbs accessibility a few years years ago, in 2017. See #6728 and #12109

It's a long time I don't work on the Yoast plugin code but I seem to recall we added an option to make the container a nav element. Not sure whether something has changed in the meantime. It would worth checking whether it still works as expected.

I think we should consider a couple important points:

  • Semantically, the breadcrumbs is a navigational tool and ideally it should be a nav element.
  • However, we need to consider that a nav is also an ARIA landmark, so marking it up as a nav really depends on its placement on the page.

The shortcode allows users to place the breadcrumbs wherever they want. Making ti always be a nav element (and thus a landmark) may not be appropriate in all cases.

@alexstine
Copy link

@afercia

The shortcode allows users to place the breadcrumbs wherever they want. Making ti always be a nav element (and thus a landmark) may not be appropriate in all cases.

This is one that frustrates me big time.

  1. It would be confusing to see breadcrumb links outside of a <nav> tag.
  2. It would be confusing if said <nav> tag was missing an aria-label.
  3. The breadcrumbs should output just before <main> and the world would be perfect.
  4. If the breadcrumbs are placed in the middle of the page content, there is no way to find the area passed the breadcrumbs <nav> landmark without jumping to the next landmark which on most sites would be <aside> or <footer>. This is ugly.

Ultimately, after going back and forth with this in my mind, I think you should just add the <nav> to breadcrumbs and it should be up to screen reader developers or maybe WCAG 3.0 to decide how nested landmarks are handled. For example, the Gutenberg Table of Contents block can also output a <nav> tag in the <main> which leaves us with the same exact problem. I might take this chance to open an issue on the NVDA repo and see if anyone has put any thought into handling nested landmarks. In theory, there should be some shortcut to skip to the end of a landmarks content, and it should be the inner most landmark the user is currently interacting with.

Anyway, not going to tell you how to make your plugin, this is Yoast after all, just wanted to add some thoughts about how I approach these situations.

Thanks.

@alexstine
Copy link

Wrote it up: nvaccess/nvda#15536

@afercia
Copy link
Contributor

afercia commented Sep 27, 2023

4. If the breadcrumbs are placed in the middle of the page content, there is no way to find the area passed the breadcrumbs

@alexstine thanks for your feedback and for creating the NVDA issue, interesting one.

One of the things to consider is that, historically, shortcodes could be only added inside the post content. In that scenario, a nav inside a main would have been arguable in addition to trigger the nested landmarks issue you mentioned.

Now that with the WordPress Site editor the shortcodes can be placed everywhere, even inside another group with a landmark element and even inside another mav, things are a bit more complex. I have no strong preferences on whether to make it a nav element by default but then there should be at least a warning with some meaningful info and recommended best practices for users.

Overall, I think on the Yoast side we should check a few things Cc @jeroenrotty

  • Check is the shortcode still has a working option to render a nav element.
  • Consider to make this option more relevant.
  • Check the SHortcode custo block: I'm not sure I see a way to make it a nav element, which wouldn't be aligned with what the shortcode does.
  • Consideer to render by default:
    • the breadcrumbs wrapped within a anv element: what the pros and cons are?
    • the nav element must have a meaningful aria-label e..g. breadcrumbs. Do not add the word navigation
    • when set to render a nav element, add info and recommendation on best practices in a prominent view e.g. a warning

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

No branches or pull requests

4 participants