Improve accessibility of the video track editor #66832
Merged
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
|
Size Change: +203 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f4a51b8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11894983917
|
|
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. |
|
@WordPress/gutenberg-core I'd appreciate a review, when you have a chance 🚀 |
ntsekouras
reviewed
Nov 15, 2024
| return ( | ||
| <> | ||
| { tracks.length === 0 && ( | ||
| <div className="block-library-video-tracks-editor__tracks-informative-message"> |
There was a problem hiding this comment.
Would using a VStack here simplify a bit the extra styles? Also block-library-video-tracks-editor__tracks-informative-message-description is not used anywhere.
There was a problem hiding this comment.
Also
block-library-video-tracks-editor__tracks-informative-message-descriptionis not used anywhere.
it is used here to reduce the bottom margin.
There was a problem hiding this comment.
Would using a VStack here simplify a bit the extra styles?
I'd agree it could be simplified but it's out of the scope of this PR. As you mentioned, the whole UI of the video track editor feels a little 'old' and may benefit from some redesign, in a separate issue.
ntsekouras
reviewed
Nov 15, 2024
| { __( | ||
| 'Tracks can be subtitles, captions, chapters, or descriptions. They help make your content more accessible to a wider range of users.' | ||
| ) } | ||
| </p> |
There was a problem hiding this comment.
In general these components might need a bit design refresh, but I think we should at least match trunk. That means we should add the separator that previously was added by the MenuGroup, no?
There was a problem hiding this comment.
That separator is part of the components-menu-group MenuGroup component. It is meant to separate menu groups.
Previously, in the initial state, there were two groups and one of them was used incorrectly. Now, there's only one group so there's nothing to separate. After uploading a track, there are two groups and the separator is still there. Screenshot:
ntsekouras
reviewed
Nov 15, 2024
| if ( ! isOpen ) { | ||
| // When the Popover opens make sure the initial view is | ||
| // always the track list rather than the edit track UI. | ||
| setTrackBeingEdited( null ); |
There was a problem hiding this comment.
I think it's better to do that in onClose Dropdown prop. And even also check if we need to update the state.
There was a problem hiding this comment.
That would not work. onClose isn't called when closing the Popover with the Escape key. It is only called when activating the Close or remove buttons. As such, there's the need to make sure the UI is in the initial state when it opens.
There was a problem hiding this comment.
Hm.. I also tested pressing Escape and it is called. Did you try adding this in block-library-video-tracks-editor Dropdown?
There was a problem hiding this comment.
OK I see the misunderstanding. It's the SingleTrackEditor onClose that is not called when pressing Escape.
Instead, when adding the onClose to the Dropdown it is called, as expected. That could work.
However, I'm not sure I understand the problem you're willing to fix. Can you please expand a bit? Is it only a stylistic preference?
To me, things seem logical:
- We need to make sure the UI is reset to the initial view (track list) when it opens.
- As such, the state is reset when the UI opens.
- I'm not sure there's the need to 'check if we need to update the state', as it should always be reset when it opens.
There was a problem hiding this comment.
Logically it's the same, but semantically it makes more sense to me to reset the state in onClose. TBH I don't think it's that important..
Regarding the check, since we use the same value(null), it won't trigger a re-render, so we can skip that. At first I thought it would re-render.
There was a problem hiding this comment.
Thanks for the clarification and additional review @ntsekouras
ntsekouras
reviewed
Nov 15, 2024
There was a problem hiding this comment.
Besides mostly the separator comment, I think this is an improvement over trunk.
It seems this components were implemented a while ago and could use a design refresh and possibly some refactoring. @WordPress/gutenberg-design can we add here some small quick wins or track them separately?
Some quick wins might be the popover padding, or having the kind select control full width, etc..
I'm also not keen of changing the contents of the same popover when editing a track. Should we consider opening a modal maybe? (not for this PR)
|
@ntsekouras thanks for your review. I think I answered all your points. |
afercia
force-pushed
the
fix/video-track-editor-accessibility
branch
from
32aa06e to
9eae0ba
Compare
|
Rebased. |
afercia
force-pushed
the
fix/video-track-editor-accessibility
branch
from
9eae0ba to
f4a51b8
Compare
|
@WordPress/gutenberg-core I'd appreciate a final review, when you have a chance. |
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 #66717
What?
Why?
How?
Testing Instructions
sample.txt
Testing Instructions for Keyboard
Screenshots or screencast
Before:
After: