-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fix: Accessibility issue with links listing blocks. #(64051) #66866
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
base: trunk
Are you sure you want to change the base?
Fix: Accessibility issue with links listing blocks. #(64051) #66866
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @im3dabasia! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Hi @im3dabasia I do not believe that this is the correct solution for this problem. I also believe that the problem and use case needs to be described in more detail before a solution can be proposed. I recommend allowing some more time to discuss the issue first. The Also when there is more than one nav on a page, visitors need to be able to distinguish one nav from the other. |
|
|
||
| return sprintf( | ||
| '<p %1$s>%2$s</p>', | ||
| '<nav %1$s>%2$s</nav>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well in my test, but the nav needs a unique label. It also needs to take into the account that more than one of copy of the tag cloud block can be used on the same page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @carolinan ,
I have addressed the changes you suggested earlier. Could you please guide me on the changes I need to make based on this comment? I am unsure.
|
Thank you for the feedback, @carolinan! I'll hold off on any additional changes until there's further discussion on the issue's scope and proposed solution. |
My concern is the page list block, if that can be separated I think work on the other two blocks can continue. |
Thank you for the reply @carolinan. I will start working on the other 2 blocks but I need some guidance. In the category list block. The suggestion was to change the constant tagName to 'nav' instead of 'ul', Here I had a doubt since the function renderCategoryList() returns a array of This would result in wrong markup. I am confused here on how to approach this. Any guidance on this would be much appreciated |
|
@im3dabasia I think you can change the |
|
I found that the tag cloud has a There aren't many examples of adding aria labels that are editable by the user. The navigation block uses one, and there is a suggestion to add one to the group block.
The categories are different because it already has a label text that is used when the dropdown option is selected. |
|
Hey @carolinan I am new to contributing and just want to verify if I need to do the following: a) Enable ariaLabel support in block.json. Please let me know if I’m on the right track or if there’s anything I’m missing. |
|
Yes. |
|
Hi @carolinan, I have implemented the requested changes and added support for the aria-label, allowing the user to customize it. By default, the name is set to "Tag Cloud", but the user has the option to modify it in the Advanced section of the Inspector Controls. Attaching a video reference for the same: Screen.Recording.2024-12-06.at.4.26.43.PM.movPlease take a look at the changes, and I would appreciate any feedback you may have. Thank you! |
|
Hi @carolinan, |
| function render_block_core_tag_cloud( $attributes ) { | ||
| $smallest_font_size = $attributes['smallestFontSize']; | ||
| $unit = ( preg_match( '/^[0-9.]+(?P<unit>[a-z%]+)$/i', $smallest_font_size, $m ) ? $m['unit'] : 'pt' ); | ||
| $aria_label = isset( $attributes['ariaLabel'] ) ? $attributes['ariaLabel'] : 'Tag Cloud'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default text must be translatable, placed inside a translation function.
|
Please solve the merge conflict first :) |
05ead12 to
e27d92c
Compare
|
This comment made me realize that the About this PR: My biggest concern is backwards compatibility. This PR changes the structure of the output HTML, which may have a significant impact on themes that already apply styles to these blocks via stylesheets. If we replace the block wrapper element itself with a nav elementThe HTML will change as follows: <!-- From -->
<p class="wp-block-tag-cloud">
<a href="">Tag</a>
</p>
<!-- To -->
<nav class="wp-block-tag-cloud">
<a href="">Tag</a>
</nav>Selectors like If we wrap the block wrapper element in a
|
|
Its not great but the accessibility is more important. |
|
I agree that accessibility is important, but we cannot ignore the impact on backward compatibility for that reason. I think we need to investigate in advance how much of an impact it will have. In any case, |
|
I did a quick search on wpdirectory.net, which does not replace doing more research: |
t-hamano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we proceed with this PR, I recommend that you thoroughly investigate the impact of this change, along with accessibility feedback. For example:
- Is the global styles applied correctly?
- Are there any visual changes in the default theme?
- Are block support, especially spacing support, applied correctly?
Additionally, we should consider the impact of selectors like the following if a theme uses them:
.wp-block-categories-list > li.wp-block-categories > li.wp-block-tag-cloud > a
|
I want to reaffirm whether moving forward with this PR approach is truly important from an accessibility perspective. This PR will result in the following changes to the HTML: Tag Cloud Block<!-- From -->
<p class="wp-block-tag-cloud">
<a href="" class="tag-cloud-link" aria-label="Tag A (1 item)">Tag A</a>
<a href="" class="tag-cloud-link" aria-label="Tag B (1 item)">Tag B</a>
<a href="" class="tag-cloud-link" aria-label="Tag C (1 item)">Tag C</a>
</p>
<!-- To -->
<nav class="wp-block-tag-cloud" aria-label="Tag Cloud">
<a href="" class="tag-cloud-link" aria-label="Tag A (1 item)">Tag A</a>
<a href="" class="tag-cloud-link" aria-label="Tag B (1 item)">Tag B</a>
<a href="" class="tag-cloud-link" aria-label="Tag C (1 item)">Tag C</a>
</nav>Categories Block (as a list)<!-- From -->
<ul class="wp-block-categories-list wp-block-categories">
<li class="cat-item cat-item-1"><a href="">Category A</a></li>
<li class="cat-item cat-item-2"><a href="">Category B</a></li>
<li class="cat-item cat-item-3"><a href="">Category C</a></li>
</ul>
<!-- To -->
<nav class="wp-block-categories-list wp-block-categories">
<ul>
<li class="cat-item cat-item-1"><a href="">Category A</a></li>
<li class="cat-item cat-item-2"><a href="">Category B</a></li>
<li class="cat-item cat-item-3"><a href="">Category C</a></li>
</ul>
</nav>My concerns are:
|
I would say that at the very least the related blocks should provide feature parity with what was implemented in Core, although the Core implementation is behind opting-in for https://make.wordpress.org/core/2020/07/09/accessibility-improvements-to-widgets-outputting-lists-of-links-in-5-5/
A nav element without appropriate labeling is only acceptable if there's only one nav element on a page. |
|
If a block is duplicated and has the same label, that's OK; a block with the same content is semantically the same block, so a duplicate label is accurate. I would say that yes, they should be wrapped in However, I note that while block themes mostly declare HTML5 by default, they are omitting the navigation widgets from that declaration; is that an oversight? See: r52439 & https://github.com/WordPress/wordpress-develop/blob/b5e0c16c04edcebdf5e81c39796a9d2c22cfd6f7/src/wp-includes/theme.php#L4394 |
Aha, yes that makes sense, thank you, |
I am not sure what a navigation widget is. |
As @joedolson pointed out, it appears block themes do not support the full 'html5 support' extra arguments as |
|
But where is it documented? it is not listed here under the html5 heading. https://developer.wordpress.org/reference/functions/add_theme_support/ |
|
Yeah I would assume it was unintentionally missed because it is not clearly documented. |
I guess so. I see the REST API doesn't support it as well? This new support was intended to be fully 'opt-in' but I'm not sure why it hasn't been documented. |
|
I feel like we need an option that allows a user to just pass 'all' or true to support all HTML5 options. The need to specifically iterate each individual support makes sense for older themes to have a gradual migration path, but it just creates extra labor and complication for most modern use cases. Added a core ticket to propose this. |
|
Thanks everyone for your feedback!
Does this mean that if a theme doesn't have If so, I'm wondering if this argument applies to other blocks as well. For example:
|
No, the blocks should use modern HTML that is supported by the browsers that WordPress requires. |
|
The HTML theme support exists to make it easier for theme developers to use modern HTML, while keeping the features backwards compatible with older themes that has styles that expects a certain markup. As seen here: https://core.trac.wordpress.org/ticket/61183 |
Ideally, blocks themes (and thus the blocks they use) should output modern html5 markup and use best practices by default. as @carolinan pointed out, the |
|
Sorry for the late reply. I thought about how we could move this PR forward. I think it may be good to focus on just adding a wrapper That is, remove the I think the key here is probably to see what visual changes occur in the default themes. Let me know what you think! |
What?
Fixes #64051.
Why?
This PR addresses accessibility by ensuring screen readers recognize these blocks as navigational elements, as per issue #64051.
How?
Added
<nav>wrappers around the lists incore/page-listandcore/tag-cloudblocks. Forcore/categories, the<nav>is conditionally added when displayed as a list (not as a dropdown), improving accessibility.Screenshots or screencast