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: Adds ariaLabel attribute to navigation block #68735

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

Conversation

singhakanshu00
Copy link

@singhakanshu00 singhakanshu00 commented Jan 17, 2025

What?

fixes #68719

Why?

  • The navigation block doesn't preserve the default aria-label attributes passed from a theme pattern when making any change to the template part the pattern is used into.

How?

  • In footer.php we are adding ariaLabel as an attribute, but we haven't added this attribute to block.json of the Navigation block.

Testing Instructions

  • Activate Twenty Twenty-Four.
  • Make sure the Footer hasn't been customized. Reset any change if necessary.
  • Go to any page on the front end.
  • Inspect the 3 elements in the footer.
  • Observe they all have an appropriate aria-label: About, Privacy, Social Media.
  • Note: these are the values that come from the theme pattern footer.php.
  • Edit the footer and make any unrelated change, e.g.: change the Site title to open in a new tab and save.
  • Go to any page on the front end.
  • Inspect the 3 elements in the footer.
  • Observe the aria-label, it is preserved.

Screenshots or screencast

hey.mp4

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

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @singhakanshu00! 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.

@singhakanshu00 singhakanshu00 marked this pull request as ready for review January 17, 2025 08:38
Copy link

github-actions bot commented Jan 17, 2025

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: singhakanshu00 <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: im3dabasia <[email protected]>
Co-authored-by: afercia <[email protected]>

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

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Block] Navigation Affects the Navigation Block labels Jan 17, 2025
@Mamaduka
Copy link
Member

Thanks for contributing, @singhakanshu00!

Please use reserved keywords when to link PRs to the issues. See: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests

@carolinan
Copy link
Contributor

Why is the aria label needed as a block attribute when the aria label block support is already enabled?
I think the reason why the block support is not working needs to be researched first.

@singhakanshu00
Copy link
Author

singhakanshu00 commented Jan 17, 2025

Why is the aria label needed as a block attribute when the aria label block support is already enabled? I think the reason why the block support is not working needs to be researched first.

Hi @carolinan
I did research and here was my conclusion to why I decided to add ariaLabel as an attribute:

Hence I added an ariaLabel attribute. Let me know if I missed something.

Thanks

@Mamaduka
Copy link
Member

@carolinan is correct. @singhakanshu00, based on the details you've shared, the root cause of the issue seems to be with ariaLabel support. We should try to fix it there instead of patching the bug for a single block, as it might affect other blocks.

@fabiankaegy
Copy link
Member

Adding the supports.ariaLabel flag should automatically register that attribute for any blocks that opt into the flag. There should be no need to manually register the attribute.

If this is currently broken we need to resolve it at the root and not "fix" the cause here :)

@t-hamano
Copy link
Contributor

I think this is a bit of a tricky problem.

The underlying problem is that block support for dynamic blocks is not complete. There are two main reasons for this:

Block support schema issues

ariaLabel support, like other block support, injects attributes via blocks.registerBlockType. The ariaLabel schema references a saved HTML element. This works for static blocks, but not for dynamic blocks like the Navigation block, because the HTML is not saved.

Why is the aria label needed as a block attribute when the aria label block support is already enabled?

This is exactly why we need to add block attributes explicitly, i.e. store them as comments.

We have tried in the past to add anchor support to dynamic blocks as well as static blocks, but encountered similar issues that are still not resolved. See #51402 for details.

Server-side processing

For block support to work in dynamic blocks, registration is required on the PHP side, not just the JS side. However, there is no PHP-side processing for ariaLabel support.


To fundamentally solve this problem, I think we need to extend the ariaLabel support and make it work for both static and dynamic blocks somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation 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] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation block: aria-label disappears after any change to the pattern the block belongs to
5 participants