Fix labeling of the navigation links in the list view. #59370
Conversation
afercia
added
[Type] Bug
|
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. |
|
Size Change: -46 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Also, @andrewserong. |
|
Thanks @alexstine for your review. To recap the main change in this PR, the logic to determine the list view items accessible name changes from this:
to this:
This is to take into account navigation links because the links text is stored in their |
|
Noting that the labeling will need further improvements for the 'locked' information, the 'sticky' information and 'anchor' information that are visually added to the items in the list view, see #59409 |
|
I decided to change approach. As such I decided to remove the aria-label. The accessible name will now be the visible text. This has a few advantages:
|
afercia
changed the title
|
@afercia Yeah, I've been wanting to do that for a while now but never got around to refactoring all of that out of the aria-label. Thanks, having another pass on this soon. |
|
Latest commit improves things a bit by providing the 'Untitled' fallback only for block labels (e.g. navigation links with a link text made of only spaces) and by adding a test. A separate issue is whether the editor should ever allow to render empty links on the front end, which is tracked in a separate issue at #59560 |
|
Flaky tests detected in b0ff43e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8185862794
|
|
It is also worth noting that right now on trunk it is possible to add a Heading block and leave it empty or with only spaces.
The current code on trunk prioritizes a block |
|
I'm probably missing something, as the failing tests show. I'll go again through my changes to see what I'm doing wrong. |
afercia
force-pushed
the
fix/site-editor-list-view-navigation-links-labeling
branch
from
cf3e968 to
24776bf
Compare
|
To clarify: I'd appreciate more thoughts on this though Cc @getdave Open to consideer a different fallback if that's more desirable. |
|
Thanks @Mamaduka I will rebase. It seems #59827 doesn't take into account the case when a heading contains only spaces though so I hink a
In this case, also the |
|
Good point. I can follow up on that! Update: Though historically, the RichText only checked emptiness based on the string length. See: https://github.com/WordPress/gutenberg/blob/trunk/packages/rich-text/src/is-empty.js Do we still want to account for spaces? |
|
Ideally we'd wrap this bug up in a single PR @Mamaduka as it will be easier to manage from a release perspective. Is that possible? |
getdave
added
the
Backport to WP 6.7 Beta/RC
|
@getdave, do you mean to push fixes on this branch? |
| // Users can enter and save a label made of only spaces e.g. for | ||
| // navigation links. In that case we provide a fallback label. | ||
| // We do want to trim leading and trailing spaces anyways. | ||
| const trimmedLabel = label.trim(); | ||
| return trimmedLabel === '' ? __( 'Untitled' ) : trimmedLabel; |
There was a problem hiding this comment.
I think this should be handled in getBlockLabel, which should return a fallback (block title) when only spaces are provided.
Then, we don't need to handle this case by case.
I just saw this comment. Changes in this hook will affect every block in the editor. While the argument makes sense for Heading and Navigation links, it might not work for other blocks like Template Part.
Sorry for adding confusion. I'm not personally intending to be pushing fixes to this branch. I meant, if this is targeting 6.5 it'd probably be easier for all fixes to be in a single PR. But if the followups are minor then maybe we should ship this one "as is" and then decide on whether it would be more appropriate to wait on the followups until a post-6.5 patch release? |
|
@getdave, unfortunately, I've not been following PR closely enough to propose the milestone. But I can see that PR introduces a new string that can't be merged after hard string free — RC1. |
getdave
removed
the
Backport to WP 6.7 Beta/RC
afercia
force-pushed
the
fix/site-editor-list-view-navigation-links-labeling
branch
from
24776bf to
13b0fe7
Compare
|
In the latest two commits:
Overall, I think a decision should be made first: whether to allow users to save empty headings or empty links. This should be evaluated for other blocks as well but it's very impportant for headings and links. Update: |
I think this will need a careful evaluation on what it's best for users. It really depends on the kind of content (the block type). It doesn't only impact the admin, it impacts also the front end. For example:
@getdave @Mamaduka thanks for your feednack, when you have a chance I'd appreciate a final review. |
| @@ -405,7 +396,7 @@ function ListViewBlock( { | |||
| clientIds={ dropdownClientIds } | |||
| block={ block } | |||
| icon={ moreVertical } | |||
| label={ settingsAriaLabel } | |||
There was a problem hiding this comment.
I had previously inquired about simplifying the "Options for %s" label, but was informed the block name was necessary by @alexstine. Is that no longer the case?
Either way though, this should be renamed "Options", like it's called in the block toolbar (as they're the same).
@afercia @Mamaduka Was there a reason to change "Options" to "Actions"?
There was a problem hiding this comment.
@richtabor I guess it was a battle I was going to lose eventually. It was really a stopgap to make NVDA behave as sometimes navigating the rows on the right side would not tell you which block you were opening an options menu for.
* Fix labeling of the navigation links in the site editor list view. * Adjust tests. * Simplify labeling by removing aria-label and using text content. * Improve logic and add test. * Adjust test. * Adjust block renaming test. * Adjust list view test. * Revert changes to useBlockDisplayTitle. * Make heading with only spaces be considered empty. Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Fixes #59369
What?
In the Site Editor > Design > Navigation > Any navigation menu, the labeling of the list view doesn't provide any informatiom about what the item is. The only information provided is the link type.
Why?
To be able to understand what they are going to edit, users need to know what the link actually is, ie. the link text.
How?
Testing Instructions
Block 3 of 5, Level 1..Observe in the editor navigation panel list view the empty link has now a label 'Untitled'.Block 3 of 5, Level 1. This block is locked.Testing Instructions for Keyboard
Screenshots or screencast