Add aria-label attribute to navigation block only when it is open
#54816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Type] Bug
|
Size Change: +26.2 kB (+2%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
SantosGuillamot
added
the
Backport to WP 6.7 Beta/RC
luisherranz
requested changes
Oct 2, 2023
| @@ -698,7 +698,7 @@ function render_block_core_navigation( $attributes, $content, $block ) { | |||
| if ( $should_load_view_script ) { | |||
| $nav_element_directives = ' | |||
| data-wp-interactive | |||
| data-wp-context=\'{ "core": { "navigation": { "overlayOpenedBy": {}, "type": "overlay", "roleAttribute": "" } } }\' | |||
| data-wp-context=\'{ "core": { "navigation": { "overlayOpenedBy": {}, "type": "overlay", "roleAttribute": "", "ariaLabel": "' . __( 'Menu' ) . '" } } }\' | |||
There was a problem hiding this comment.
We should escape this because translations might include not supported characters (like quotes or brackets…). Or just move the whole context into a $context variable and use wp_json_encode( $context ) instead.
There was a problem hiding this comment.
That makes sense 🙂 I added the esc_attr__ function. I hope that is enough. If you feel it is clearer, I'm happy to mode it to a $context variable.
There was a problem hiding this comment.
I think that one turns things like " into ", and it needs to be \". But I'm not an expert on this, so maybe I'm wrong. Can you check if JSON.parse() turns the escaped items (like " or >) back to its original form?
There was a problem hiding this comment.
Oh, you're right. esc_attr__ doesn't seem to be working fine. If I include a quote " inside the string, it is not able to parse it and it throws an error. I believe we have two options:
- Use wp_slash or directly addslashes.
- Move it to a variable and use
wp_json_encodeas you suggested. We are using it in the query block: link.
I've tested both approaches and they seem to work. Any preference?
There was a problem hiding this comment.
wp_json_encode seems more explicit and secure 🙂
There was a problem hiding this comment.
Thanks for taking a look at this, @SantosGuillamot. I can see in the video you shared that the problem arises when the data-wp-context directive runs in PHP.
What I don't fully understand is that it seems to be an issue serializing the context as JSON (instead of parsing)... 🤔
There was a problem hiding this comment.
I looked at the HTML when using it in the navigation block.
In the client
- Without the
JSON_HEX_APOS, the HTML isdata-wp-context="{"text":"I" m="" a="" string"}'="". - With
JSON_HEX_APOS, the HTML isdata-wp-context="{"text":"I\u0027m a string"}".
In the server
- Without the
JSON_HEX_APOS, the HTML isdata-wp-context='{"text":"I'm a string"}'. - With
JSON_HEX_APOS, the HTML isdata-wp-context='{"text":"I\u0027m a string"}'.
So something seems definitely broken, right?
There was a problem hiding this comment.
Okay, I've added the flags to both the navigation and the query blocks. Let me know if something else is needed.
There was a problem hiding this comment.
For context, the "Fatal error" that can be seen in #54816 (comment) is caused by a problem insidegutenberg_interactivity_process_wp_context implementation, which is calling $context->rewind_context() even when no context where added on a potential failure. That could empty the context stack and make subsequent $context->set_context( $new_context ) calls fail.
I'll open an issue to address that later on.
There was a problem hiding this comment.
|
Flaky tests detected in b8f157d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6430096160
|
luisherranz
approved these changes
Oct 6, 2023
Totally agree 🙂 It is not intuitive why those flags, and not others, are needed. By the way, it seems a test of the cover block is failing. I cannot see the relation with this pull request and it seems it's failing in other PRs like this one as well. I'll rerun the tests once that is solved. |
mikachan
pushed a commit
that referenced
this pull request
Oct 10, 2023
…54816) * Add `aria-label` only when is open * Remove unnecessary `label` property in edit * Escape translation * Move navigation context to `wp_json_encode` * Add `wp_json_encode` flags
|
I just cherry-picked this PR to the 6.4-beta3-2 branch to get it included in the next release: 80e0020 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
karmatosed
pushed a commit
that referenced
this pull request
Oct 10, 2023
* Site Editor Styles Screen: Fix dancing styles previews (#55183) * Pulling across changes from WordPress/wordpress-develop#5441 (#55181) Removed var * Add `aria-label` attribute to navigation block only when it is open (#54816) * Add `aria-label` only when is open * Remove unnecessary `label` property in edit * Escape translation * Move navigation context to `wp_json_encode` * Add `wp_json_encode` flags * Paste: fix MS Word list paste (#55127) * Paste: fix MS Word list paste * Match mso-list:Ignore * Fix inline paste * Fix scrollbars on pattern transforms (#55069) * Fix scrollbars on pattern transforms * Fix single pattern previews * Improve classname semantics * Remove modal title * Reset styles on window resize (#55077) Co-authored-by: Ricardo Artemio Morales <[email protected]> --------- Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: Mario Santos <[email protected]> Co-authored-by: Ella <[email protected]> Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Ricardo Artemio Morales <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fix the issue reported here.
This pull request adds the logic to only add the
aria-labelattribute to the navigation block when this is open.Why?
As reported in the mentioned issue, it is causing accessibility issues.
How?
I basically removed that attribute from the server side rendering and added some logic in JavaScript to load it conditionally only when the menu is open.
I also removed an unnecessary prop of the React component used in the editor.
Testing Instructions
Before this fix
divwith classwp-block-navigation__responsive-dialog. It has the attributearia-label="Menu", which is not correct.After this fix
divwith classwp-block-navigation__responsive-dialog. It has NOT the attributearia-label.aria-label="Menu"is added.