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

Fix: Accessibility issue with links listing blocks. #(64051) #66866

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

im3dabasia
Copy link
Contributor

@im3dabasia im3dabasia commented Nov 8, 2024

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 in core/page-list and core/tag-cloud blocks. For core/categories, the <nav> is conditionally added when displayed as a list (not as a dropdown), improving accessibility.

Screenshots or screencast

Before After
Before After

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 8, 2024
Copy link

github-actions bot commented Nov 8, 2024

👋 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.

@im3dabasia im3dabasia marked this pull request as ready for review November 8, 2024 11:56
@im3dabasia im3dabasia requested a review from ajitbohra as a code owner November 8, 2024 11:56
Copy link

github-actions bot commented Nov 8, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: im3dabasia <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: Hug0-Drelon <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@skorasaurus skorasaurus added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 8, 2024
@carolinan
Copy link
Contributor

carolinan commented Nov 10, 2024

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 <nav> element is intended to be used for navigations, and all lists of links are not necessarily navigations.

Also when there is more than one nav on a page, visitors need to be able to distinguish one nav from the other.
Consider a scenario where the user adds both a tag cloud and categories block in the same post content or template.
The navigations need to have labels describing their purpose.

@@ -41,7 +41,7 @@ function render_block_core_tag_cloud( $attributes ) {
$wrapper_attributes = get_block_wrapper_attributes();

return sprintf(
'<p %1$s>%2$s</p>',
'<nav %1$s>%2$s</nav>',
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@carolinan carolinan added the [Type] Enhancement A suggestion for improvement. label Nov 10, 2024
@im3dabasia
Copy link
Contributor Author

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.

@carolinan
Copy link
Contributor

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.

@im3dabasia
Copy link
Contributor Author

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 <li>( renderCategoryList calls renderCategoryListItem() internally). If I change the TagName to 'nav' then the generated markup would be

<nav>
<li> </li>
<li> </li>
<li> </li>
</nav>

This would result in wrong markup. I am confused here on how to approach this. Any guidance on this would be much appreciated

@carolinan
Copy link
Contributor

@im3dabasia I think you can change the <nav> element that you added around renderCategoryList() to a <ul>, and then change <ul> in the TagName constant to a nav?

@im3dabasia im3dabasia requested a review from carolinan December 3, 2024 03:34
@carolinan
Copy link
Contributor

I found that the tag cloud has a format parameter where developers can choose between an unordered list and a flat list of items. But both formats can be inside a <nav> without any validation problems. https://developer.wordpress.org/reference/functions/wp_tag_cloud/

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.

  1. ariaLabel is a block support, it needs to be set to true in block.json.
  2. After updating the block support, the documentation also needs to be updated. npm run docs:build.
  3. The option needs to be a text input field in the Advanced section of the InspectorControls.
  4. The option needs a suitable default value for when the user does not customize the label.
  5. The option needs a suitable label. The label for the option in the navigation block is "Menu name" , so perhaps "Tag Cloud name"?

The categories are different because it already has a label text that is used when the dropdown option is selected.
This label text could be re-used as the aria label on the nav.

@im3dabasia
Copy link
Contributor Author

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.
b) Add a text input field for the aria-label in the Advanced section of InspectorControls, ensuring it has appropriate default values and labels.
c) Update the documentation after the changes by running npm run docs:build.

Please let me know if I’m on the right track or if there’s anything I’m missing.

@carolinan
Copy link
Contributor

Yes.
But also be prepared that there may be more discussions and reviews since this is not a common pattern in the existing blocks.

@carolinan carolinan added [Block] Tag Cloud Affects the Tag Cloud Block [Block] Categories Affects the Categories Block labels Dec 3, 2024
@im3dabasia
Copy link
Contributor Author

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.mov

Please take a look at the changes, and I would appreciate any feedback you may have.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Categories Affects the Categories Block [Block] Tag Cloud Affects the Tag Cloud Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility issue with links listing blocks.
3 participants