wordpress-seo icon indicating copy to clipboard operation
wordpress-seo copied to clipboard

Breadcrumbs: HTML wrap filtering improved (Accessibility purpose)

Open geoffreycrofte opened this issue 1 year ago • 4 comments

Context

This PR brings some new filters to help developers customize the HTML output of the breadcrumbs.

Following accessibility good practice, a breadcrumbs is a nav element with a distinguisable label. I need to be able to edit the default div element to replace it, and also add some custom attributes like an explicit role or aria-label.

Adding these filters should help me reach my goals.

PS. my first time contributing here, sorry if I didn't take the right initial branch, the readme don't tell much about that. (unless I missed the info)

Summary

This PR can be summarized in the following changelog entry:

  • changelog: enhancement - adds a wpseo_breadcrumb_wrapper_classname filter to customize the wrapper classname
  • changelog: enhancement - adds a wpseo_breadcrumb_wrapper_attributes filter to customize the wrapper attributes list
  • changelog: enhancement - adds a wpseo_breadcrumb_wrapper_element filter to customize the wrapper HTML element

Relevant technical choices:

  • I tried to follow the existing code base found on near code
  • I documented the new hooks
  • I kept the default behavior (meaning if you don't use the hooks, you won't see changes)
  • I've changed the block's PHP files, but I sincerely don't know if it needs more than that.

Test instructions

To test the hooks and changes, you can use this code:

add_filter( 'wpseo_breadcrumb_wrapper_element', 'rework_yoast_bc_wrapper_html_element' );
function rework_yoast_bc_wrapper_html_element() {
  return 'nav';
}
 
add_filter( 'wpseo_breadcrumb_wrapper_classname', 'rework_yoast_bc_wrapper_html_classname' );
function rework_yoast_bc_wrapper_html_classname( $default ) {
  return $default . " " . 'my-new-class';
}
 
add_filter( 'wpseo_breadcrumb_wrapper_attributes', 'rework_yoast_bc_wrapper_html_attributes' );
function rework_yoast_bc_wrapper_html_attributes() {
  return array(
          'aria-label' => __("Breadcrumbs"),
          'role'       => 'navigation'
        );
}

Relevant test scenarios

  • [ ] Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • [ ] Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • Breadcrumbs block: double check that default behavior is still there.
  • Try the code provided above to check the HTML editions on the breacrumbs block in front-end

Documentation

  • [x] I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

  • [x] I have tested this code to the best of my abilities.
  • [x] During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • [ ] I have added unit tests to verify the code works as intended.
  • [x] I have written this PR in accordance with my team's definition of done.
  • [x] I have checked that the base branch is correctly set. (I used the Trunk default branch)

Innovation

  • [x] No innovation project is applicable for this PR.
  • [ ] This PR falls under an innovation project. I have attached the innovation label.
  • [ ] I have added my hours to the WBSO document.

geoffreycrofte avatar May 22 '24 11:05 geoffreycrofte

Thank you so much @shadivakhande90 for the approval 😃

geoffreycrofte avatar Jun 20 '24 16:06 geoffreycrofte

Hello folks, I wonder what are the next steps now? Thank you.

geoffreycrofte avatar Jun 29 '24 11:06 geoffreycrofte

Hello folks, Do you need anything else from me?

This PR is "just" some new filters for more flexibility, doesn't remove anything and doesn't change default behaviour. And also has no conflict. Should be a smooth merge, don't you think?

Happy 2nd month birthday 🎂 to my PR! :D

geoffreycrofte avatar Jul 22 '24 07:07 geoffreycrofte

Hey @geoffreycrofte, sorry for the delay, we've been very busy with the latest features of our plugins. We'll try to take a look at your proposal in the upcoming weeks.

enricobattocchi avatar Aug 23 '24 11:08 enricobattocchi

Hi @geoffreycrofte, thanks for your suggestion! Fortunately, we already have some filters for the breadcrumbs output that can be used to get what you need. Considering that at the moment the output is like this:

<div class="yoast-breadcrumbs">
    <span>
        <span><a href="http://domain.tld">Home</a></span> » <span><a href="http://domain.tld/category/uncategorized/">Uncategorized</a></span> » <span class="breadcrumb_last" aria-current="page">Post Title</span>
   </span>
</div>

you would want to replace the first, outer span (instead of the outer div) with a nav, which you can do with the wpseo_breadcrumb_output_wrapper filter. To apply a class to it, you can use the wpseo_breadcrumb_output_class. And while we don't have a filter for the custom attributes, the overarching wpseo_breadcrumb_output filter allows you to freely edit anything in the final output, with some string manipulation if needed.

To achieve what you added in the test instructions, you can already use this snippet in your theme's functions.php:

add_filter( 'wpseo_breadcrumb_output_wrapper', function( $tag ){
	return 'nav';
});

add_filter( 'wpseo_breadcrumb_output_class', function( $classname ){
	return 'my-class';
});

add_filter( 'wpseo_breadcrumb_output', function( $output ){
	return str_replace( 'class="my-class"', 'class="my-class" aria-label="Breadcrumbs" role="navigation"', $output );
});

Let us know your experience with that!

enricobattocchi avatar Sep 05 '24 08:09 enricobattocchi

Hello @enricobattocchi thank you for your time in analyzing my proposition.

Unfortunately, what you propose doesn't match my needs and feel a little bit not future proof. Parsing the output to add custom attributes feels unsafe for many reasons. (other plugins conflicts, for instance).

It also seems that we are not talking about the same wrapper, which, as of today, lack the ability to be filtered. (

element)

If you would be kind enough to re-evaluate my proposition, it would be awesome. Thanks

geoffreycrofte avatar Oct 23 '24 13:10 geoffreycrofte