Improve accessibility and consistency of the 'Last modified' Revisions button. #66606
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
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: -340 B (-0.02%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 318ea6f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12008420958
|
|
A note on the following components used for this UI:
They are all wrappers to other components like HStack and Text. It appears to me they are a bit overkill and not really necessary. It looks like they're only used here and the more generic component could be used instead. Also, SidebarNavigationScreenDetailsPanelLabel and SidebarNavigationScreenDetailsPanelValue are basically duplicate. The code could be greatly simplified by entirely removing these components. |
afercia
force-pushed
the
fix/styles-navigation-revisions-button
branch
from
eafd7f1 to
ea4d940
Compare
|
Rebased. |
ramonjd
reviewed
Nov 19, 2024
There was a problem hiding this comment.
Thanks for the PR. I don't have much to add, other than I agree the component could be simplified.
Keen to learn what @jameskoster thinks.
| { createInterpolateElement( | ||
| sprintf( | ||
| /* translators: %s: is the relative time when the post was last modified. */ | ||
| __( '<time>%s</time>' ), | ||
| __( 'Last modified: <time>%s</time>' ), |
There was a problem hiding this comment.
Do you think it makes sense to use the simpler format used by posts and templates?
sprintf(
// translators: %s: Human-readable time difference, e.g. "2 days ago".
__( 'Last edited %s.' ),
humanTimeDiff( modified )
);There was a problem hiding this comment.
Actually, on second thought. How valuable is this information?
If it's still accessible, I think just having the revisions button with a count might appear less cluttered:
// translators: %s: number of revisions.
sprintf( __( '(%s) Revisions' ), revisionsCount )
What do you reckon?
|
I wouldn't be opposed to remove the 'Last modified' info if there's agreement on that. |
afercia
force-pushed
the
fix/styles-navigation-revisions-button
branch
from
ea4d940 to
38712b8
Compare
|
I see random tests failures. Restarted the related jobs. |
I'm going to try that soon, thanks. |
t-hamano
reviewed
Nov 20, 2024
| @@ -17,7 +17,6 @@ import { backup } from '@wordpress/icons'; | |||
| import { | |||
| SidebarNavigationScreenDetailsPanelRow, | |||
| SidebarNavigationScreenDetailsPanelLabel, | |||
| SidebarNavigationScreenDetailsPanelValue, | |||
There was a problem hiding this comment.
Since the SidebarNavigationScreenDetailsPanelValue component isn't being used anywhere anymore, we should be able to delete the component itself.
| } | ||
|
|
||
| .edit-site-sidebar-navigation-details-screen-panel__label time, | ||
| .edit-site-sidebar-navigation-details-screen-panel__value.edit-site-sidebar-navigation-details-screen-panel__value { |
There was a problem hiding this comment.
We can remove the .edit-site-sidebar-navigation-details-screen-panel__value CSS class.
| margin: $grid-unit-20 0 0; | ||
| border-top: 1px solid $gray-800; | ||
|
|
||
| .components-item-group { |
There was a problem hiding this comment.
Suggested change
| .components-item-group { | |
| .edit-site-sidebar-navigation-screen-details-footer { |
Let's avoid using .components-prefixed CSS classes whenever possible.
|
This is going to be updated soon. However, in the meantime I think I spotted an issue. This revisions count: appears to be 0 even when there are saved revisions. It seems to me that Dumping
So that, for example:
I'm not sure I understand the logic behind it or maybe something changed in the meantime. Now that we're trying to show the number of revisions I think the count from the parent component should be used instead, which appears to be correct. |
|
In the last commit I'm trying to simplify a lot, remove unused components and CSS, and add the revisions count. Screenshot:
I'll update the testing instructions. |
Thanks for flagging that. It does seems odd. I'll look into it 🙇🏻 |
Looks like it was a bug. Good catch! I have a fix for it here: |
|
Last commit simplifies the revisions count retrieval following the approach from #67180 |
afercia
force-pushed
the
fix/styles-navigation-revisions-button
branch
from
2950c1d to
318ea6f
Compare
ramonjd
approved these changes
Nov 25, 2024
There was a problem hiding this comment.
This is a good change in my opinion. The last modified information was superfluous, and the reduced content appears much neater.
I tested in Firefox, Safari and Chrome. No regressions in the link to the revision panel.
Here's what I'm seeing:
| No revisions | One revision | > 1 revisions |
|---|---|---|
 |
 |
 |
Here's what's in trunk right now:
|
Thanks @ramonjd for your review.
It's worth reminding that besides the visual changes, this is mainly an accessibility fix to avoid a mismatch between visible text and accessible name. For example: 
This kind of mismatch is not OK. I would appreciate any new design to take into consideration that the visual labeling of interactive controls is not a place to communicate values or states. The visual labeling must communicate what the control does. Attempting to remediate with an aria-label (in this case it was 'Revisions') doesn't make things better. Instead, it introduces a barrier for users and a WCAG violation. |
|
This PR seems to have caused a visual regression in the alignment of menu items. In the screenshot below the top example is 6.7, the bottom is trunk. 
|
|
Thanks @jameskoster, looks like changing this selector after core review is the culprit of the alignment regression. Will submit a quick PR to fix it. |
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.
Fixes #66526
Alternative to #66535
What?
The 'Last modified' button in the Site editor navigation panel > Styles is not accessible and arguably usable as the visibel text mismatches the accessible name and the button's action isn't clear.
Why?
A complete mismatch between an interactive control visible text and its accessible name is a WCAG violation.
On top of that, buttons must clearly communicate the udnerlying action for all users.
For more details, see the issue #66526
How?
Separates the 'last modified' information from the button.
Makes clearer the button is a navigation button and that it brings to 'Revisions'.
Testing Instructions
Old instructions
New instructions
wp_global_styles?) or by switching to a theme where you haven't saved Styles changes yet.Testing Instructions for Keyboard
Screenshots or screencast