Add option to make breadcrumbs a nav element. #12079

afercia · 2019-01-21T17:47:42Z

Summary

This PR can be summarized in the following changelog entry:

Add an option to make the Breadcrumbs contained in a <nav> element for better SEO, semantics, and accessibility.

Relevant technical choices:

Things are a bit complicated in terms of backwards compatibility because the rendered markup depends on parameters e.g. the before / after and the filters. Also, there's the related shortcode to take into consideration.

To keep things simple, I've opted for adding an user option and override the rendered markup. I guess this would need to be discussed, any feedback welcome. I couldn't think of an alternate, reasonable, way to do it.

adds an option to wrap the breadcrumbs in a <nav> element with an aria-label attribute
adds aria-current to the active item
adds a wrapper <span> to the prefix, as this text was rendered without an easy way to target it with CSS

Test instructions

Add the breadcrumbs in the single template of your theme, following the example on https://kb.yoast.com/kb/implement-wordpress-seo-breadcrumbs/

if ( function_exists( 'yoast_breadcrumb' ) ) {
	yoast_breadcrumb( '<p id="breadcrumbs">', '</p>' );
}

Add the following quick'n dirty core in your theme's functions.php to test the filters for the ID and CSS class:

add_filter( 'wpseo_breadcrumb_output_class', 'myclass' );
add_filter( 'wpseo_breadcrumb_output_id', 'myid' );
function myclass() {
	return 'classfromtheme';
}
function myid() {
	return 'idfromtheme';
}

Add the shortcode in one of your posts:
[wpseo_breadcrumb]

view the post
before activating the option:
check the breadcrumbs are wrapped in the <p> passed as parameter
check the inner <span> wrapper has the ID and class added via filters
check the last item has the aria-current attribute
check also the breadcrumbs in the post content added via the shortcode

Then, go in Search Appearance > Breadcrumbs and enable the "Use breadcrumbs as navigation" option

refresh the post
check the breadcrumbs are now within a <nav> element
the <nav> element has the ID and class added via filters
it also has aria-label="Breadcrumbs"
the inner <span> wrapper is removed
check the last item has the aria-current attribute

Note: also the breadcrumbs in the content added via the shortcode are now within a <nav> element. One one hand, users set an option so this would be expected. On the other hand, we can't predict where the shortcode will be placed into the content so there might be edge cases where layout is affected. Any suggestions welcome.

As said, with the option enabled the $before, $after, and the wpseo_breadcrumb_output_wrapper filter won't have any effect. I guess this should be documented somewhere.

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Fixes #6728

afercia · 2019-01-23T08:47:42Z

Aside: on yoast.com the breadcrumbs are already wrapped in a <nav> element: https://github.com/Yoast/yoast.com/blob/87372652f03a90c365afb454eb8d244873f479cd/site/web/app/themes/yoast-theme/html_includes/partials/breadcrumbs.php

andizer

CR done. I've asked @moorscode to take a look at the option addition part.

andizer · 2019-01-28T07:53:13Z

frontend/class-breadcrumbs.php

+
+		// Override the before and after strings when breadcrumbs are set to be within a `<nav>` element.
+		if ( WPSEO_Options::get( 'breadcrumbs-nav-wrapper' ) === true ) {
+			$before = '<nav aria-label="' . __( 'Breadcrumbs', 'wordpress-seo' ) . '"' . self::get_output_id() . self::get_output_class() . '>';


Instead of making the methods static you should be able to call this by $instance->get_output_it() and $instance->get_output_class()

I have concerns about overwriting the $before. I think it will be better to extend the $output. By overwriting the $before and $after you can possibly break implementations.

The KB article instructed to use the following snippet for ages:

if ( function_exists( 'yoast_breadcrumb' ) ) { yoast_breadcrumb( '<p id="breadcrumbs">', '</p>' ); }

So we can't add a <nav> inside a paragraph or any other element that could produce invalid markup. At this point there are two options:

either override the $before and $after

or wrap the entire output with a <nav> but even in this case there's the risk the markup won't be optimal

Open to suggestions 🙂 but as a general consideration, there are too many options and too many <span>, I'd rather simplify all the things.

P.S. there's also the wpseo_breadcrumb_output_wrapper filter problem to take into consideration.

andizer · 2019-01-28T08:03:10Z

frontend/class-breadcrumbs.php

 	 */
 	private function wrap_breadcrumb() {
 		if ( is_string( $this->output ) && $this->output !== '' ) {
-			$output = '<' . $this->wrapper . $this->get_output_id() . $this->get_output_class() . '>' . $this->output . '</' . $this->wrapper . '>';
+			$output = '<' . $this->wrapper . self::get_output_id() . self::get_output_class() . '>' . $this->output . '</' . $this->wrapper . '>';


I would love to see this change reverted. You can access private with self::$instance->get_output_id() / self::$instance->get_output_class()

andizer · 2019-01-28T08:04:25Z

inc/options/class-wpseo-option-titles.php

@@ -58,6 +58,7 @@ class WPSEO_Option_Titles extends WPSEO_Option {
 		'breadcrumbs-prefix'            => '', // Text field.
 		'breadcrumbs-searchprefix'      => '', // Text field.
 		'breadcrumbs-sep'               => '&raquo;', // Text field.
+		'breadcrumbs-nav-wrapper'       => false,


I am not sure if an option is the right solution for this.

If it is for accessibility, we might consider doing this by default and give the user the possibility to disable it by a hook. /cc @moorscode

andizer · 2019-01-28T08:05:02Z

frontend/class-breadcrumbs.php

@@ -1043,7 +1055,7 @@ private function get_output_id() {
 	 *
 	 * @return string
 	 */
-	private function get_output_class() {
+	private static function get_output_class() {


You can undo this change (see earlier comments)

andizer · 2019-01-28T08:05:20Z

frontend/class-breadcrumbs.php

@@ -1024,7 +1036,7 @@ private function wrap_breadcrumb() {
 	 *
 	 * @return string
 	 */
-	private function get_output_id() {
+	private static function get_output_id() {


This doesn't have to be static. See earlier commments

andizer · 2019-01-28T08:11:07Z

frontend/class-breadcrumbs.php

@@ -1012,7 +1024,7 @@ private function wrap_breadcrumb() {
 			$output = apply_filters( 'wpseo_breadcrumb_output', $output );

 			if ( WPSEO_Options::get( 'breadcrumbs-prefix' ) !== '' ) {
-				$output = "\t" . WPSEO_Options::get( 'breadcrumbs-prefix' ) . "\n" . $output;
+				$output = "\t" . '<span class="breadcrumbs-prefix">' . WPSEO_Options::get( 'breadcrumbs-prefix' ) . "</span>\n" . $output;


Can you explain the reason for adding the class?

Give the opportunity to style it. Previously, this was just text thrown in the middle of nowhere, without even a wrapper element.

andizer · 2019-01-28T08:17:21Z

If this pull is ready for a new round in the process, please ping. So we (I can help you) can add some unit tests

afercia · 2019-01-28T09:25:57Z

For reference: on yoast.com the <nav> element is hardcoded in the template:

afercia · 2019-01-28T09:34:44Z

Addressed first part of the code review. Pending decision on the other points.

moorscode · 2019-01-28T13:09:49Z

We do not want to burden people who setup Yoast SEO with theme-specific settings.
If this is the recommended way, which seems to be the case, we would want to enable this for everybody by default, which would cause backward compatibility problems.

Conclusion:

We do not want to add an option in the backend to provide this functionality.
We do want to provide this best-practice by modifying the page on the knowledge base: https://kb.yoast.com/kb/implement-wordpress-seo-breadcrumbs/

Also implementing this for the shortcode is the following theoretical way:

// Unhook the default Yoast shortcode implementation.
remove_shortcode( 'wpseo_breadcrumb', 'wpseo_shortcode_yoast_breadcrumb' );

function my_prefix_shortcode_yoast_breadcrumb() {
	return yoast_breadcrumb( '<nav>', '</nav>', false );
}

add_shortcode( 'wpseo_breadcrumb', 'my_prefix_shortcode_yoast_breadcrumb' );

Suggested KB article alternative, with explanation on why this is better than just a <p>, which people will be searching for as this was previously the example.

if ( function_exists('yoast_breadcrumb') ) {
  yoast_breadcrumb( '<nav id="breadcrumbs" aria-label="You are here:">', '</nav>' );
}

Suggested:
It would be nice to have an accessibility post about why this is the recommended practice, which can be linked to on the KB article.

moorscode · 2019-01-28T13:12:16Z

Looking at the PR there are two additions which look to be useful:

aria-current="page"
Ability to filter the breadcrumb-prefix

As I cannot find any requests about having this breadcrumb-prefix filterable, I don't want to add another filter based on a theoretical situation.

If the aria-current is a solid improvement, please just make a PR adding that.

afercia · 2019-01-28T13:43:50Z

Sure, will do another PR then. Only minor consideration: aria-label="You are here:" won't be translatable if the site uses multiple languages.

moorscode · 2019-01-28T15:59:04Z

Only minor consideration: aria-label="You are here:" won't be translatable if the site uses multiple languages.

For the example code I don't think we need to make it overly complex and a custom translation system needs to be implemented to make that possible as that string does not appear in WordPress itself.

Add option to make breadcrumbs a nav element.

b904881

afercia added the UI change PRs that result in a change in the UI label Jan 21, 2019

andizer suggested changes Jan 28, 2019

View reviewed changes

andizer added the needs-decision label Jan 28, 2019

Address first CR part.

47aa98f

afercia closed this Jan 28, 2019

afercia removed the needs-decision label Jan 28, 2019

afercia deleted the 6728-breadcrumbs-better-accessibility branch January 28, 2019 13:44

afercia mentioned this pull request Jan 28, 2019

Add aria-current to the breadcrumbs #12109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to make breadcrumbs a nav element. #12079

Add option to make breadcrumbs a nav element. #12079

afercia commented Jan 21, 2019

afercia commented Jan 23, 2019

andizer left a comment

andizer Jan 28, 2019

andizer Jan 28, 2019

afercia Jan 28, 2019 •

edited

andizer Jan 28, 2019

andizer Jan 28, 2019

andizer Jan 28, 2019

andizer Jan 28, 2019

andizer Jan 28, 2019

afercia Jan 28, 2019

andizer commented Jan 28, 2019

afercia commented Jan 28, 2019

afercia commented Jan 28, 2019

moorscode commented Jan 28, 2019

moorscode commented Jan 28, 2019

afercia commented Jan 28, 2019

moorscode commented Jan 28, 2019 •

edited

Add option to make breadcrumbs a nav element. #12079

Add option to make breadcrumbs a nav element. #12079

Conversation

afercia commented Jan 21, 2019

Summary

Relevant technical choices:

Test instructions

UI changes

afercia commented Jan 23, 2019

andizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia Jan 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andizer commented Jan 28, 2019

afercia commented Jan 28, 2019

afercia commented Jan 28, 2019

moorscode commented Jan 28, 2019

moorscode commented Jan 28, 2019

afercia commented Jan 28, 2019

moorscode commented Jan 28, 2019 • edited

afercia Jan 28, 2019 •

edited

moorscode commented Jan 28, 2019 •

edited