Add video tracks functionality #25861
Conversation
jorgefilipecosta
added
[Focus] Accessibility (a11y)
jorgefilipecosta
requested review from
ajitbohra,
Soean and
talldan
as code owners
|
Size Change: +2.06 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
7bb160d
to
6eb6675
Compare
|
Thank you, @jorgefilipecosta! This is looking really really good. I have some feedback on the design:
Do you have any context on why we only support the Again, thank you for working on this 🎉 |
|
Hi @enriquesanchez, thank you for the review. 👍
I did not add a Save because a save would not be reflecting what is happening. When the user changes something, e.g.: the label the attribute is already saved, there would be nothing to save. The changes are immediate and are saved on each keypress. That's what happens on all the other cases on the block editor, e.g., inspector changes; if we change a field, the change is applied right away. The user, of course, can use undo to undo the change. What the ok is doing is just going back to the tracklist. It is not saving anything. That's also the reason why we don't have a cancel because the cancel would also just go back to the tracklist. So Ok and Cancel are the same thing given how things are implemented. But if we think in this case, we have a special situation and should not follow the pattern of the immediate block attribute changes, I can implement a local state on the component. User changes are saved to the local state. On the save operation, the changes are saved to the attributes, and on the cancel, they are discarded, and the local state is reset.
The track element specification says only vtt (Web Video Text Tracks) format for the track element as we can check at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/track. |
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
6eb6675
to
c7194f8
Compare
Done 👍 |
Thank you for that detailed clarification, @jorgefilipecosta. I don't think this should be a special case and perhaps the design needs a bit of adjusting so we don't mislead users. The presence of a Save/Done/OK button does create the illusion of there being a confirmation step when in reality there isn't one. That said, because this is not a simple menu popover, I think there should still be a clear way to close it. In light of this, I think that your initial implementation is right; we just need to change the button to use the default style and change the label to 'Close' so it's more clear what is happening:
How does that feel to you? I also just noticed that in the PR the file name is missing in the 'File:' field up top:
|
|
Can we add a little inline description on what "tracks" are? It's can be quite inscrutable as is. |
|
Hi @mtias, Would you like that this text is expended or would want to show this message in other states e.g: there are tracks already added or the user is editing a track? |
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
c7194f8
to
e77bdef
Compare
|
Hi @enriquesanchez,
It was fixed. Your feedback was applied and the UI updates were implemented. |
Ah, thanks. I think it can make sense to keep it always displayed, but let's see how it feels |
|
@jorgefilipecosta Thanks for the updates 🙌 I'm still not seeing the file name displayed. I made sure to update the branch and tried in both Firefox and Safari. Maybe I'm missing something?
I also tested with VoiceOver and I think we can add a small improvement to make it more accessible. The 'Edit' button for each text track should say what text track is that you'll be editing. I think we can leave the 'Edit' label as is and just add an I also noticed that you can leave these fields empty, which results in this:
I don't think we should allow for this to happen and should make these fields required. Alternatively, we can at least display some defaults. For example, if no label is provided we can use the file name as the track's label. For reference the Classic editor defaults to What do you think? |
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
e77bdef
to
4aec637
Compare
I followed this suggestion by @mtias and added the text to the tracklist too:
Suggestion applied.
I followed the same approach the classic editor does. At the start the inputs are empty but if the user does not fill them they get the default values of "English" and "en" exactly what the classic editor does.
I'm not being able to reproduce the issue:
Right after I upload a vtt file for like 1 second the field is empty but after the file is uploaded the name appears, and each time I go to edit. |
|
@mtias Would like for us to reconsider showing the description of what text tracks are all the time. I think it's enough to display it when there are no text tracks/ the first time the menu is opened. Authors will encounter the description every time they add a new video, for example. I don't think it's necesssary to show it after tracks have been added. By then the user should already be familiar with the feature, and not having it helps us keep the menu more condensed in the case multiple tracks are added. |
|
Thank you for working on these updates, @jorgefilipecosta. This is in much better shape now. 🙌 I noticed a small issue when navigating with keyboard. When opening the menu, if you use the down arrow the menu closes and focus is lost and moved up to the title of the document. I expect that users might try to use up/down arrow keys when they open this menu for the first time because up/down is used to navigate other menus in the block editor. We should make sure that focus is retained inside the popover. |
| } | ||
|
|
||
| export default function TracksEditor( { tracks = [], onChange } ) { | ||
| const mediaUpload = useSelect( ( select ) => { |
There was a problem hiding this comment.
Just noting that this can be undefined, in which case we should probably hide the button entirely right?
There was a problem hiding this comment.
Or maybe allow using external URLs (can be for later).
There was a problem hiding this comment.
Yah not rendering anything seems like a good option for now. I updated the code.
| <TracksEditor | ||
| tracks={ tracks } | ||
| onChange={ ( newTracks ) => { | ||
| setAttributes( { tracks: newTracks } ); |
There was a problem hiding this comment.
Just want to ask whether the shape of the "track" object set here is well defined and that we're not dumping the whole object from the API or something like that.
There was a problem hiding this comment.
Yes, the shape is very specific an array of objects that may have {src, label, srcLang, kind } properties all being strings. We never dump anything from an API there we are intentional in storing the src explicitly (the other attributes came from user inputs).
|
@enriquesanchez sure, don't have a strong opinion on it outside of the fact I think we can generally use contextual guidance built in into the UI better. |
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
4aec637
to
a3869d4
Compare
|
Hi @enriquesanchez,
It was fixed. I also reverted and made the help text only appear before the first track is added. |
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
a3869d4
to
658ac82
Compare
|
Much better, thank you @jorgefilipecosta 👏 I'm still finding that the popover closes and my focus moves up to the document title when I use up/down arrows in the edit screen.
Can you reproduce this? |
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
658ac82
to
22f4e3b
Compare
Nice catch @enriquesanchez the issue was fixed. |
|
👏 This is working really well, @jorgefilipecosta. Thank you! I'm making some minor CSS tweaks on alignment and spacing of labels and fields and once I have that ready we should be good to go. |
|
Hey peeps, is this ready to merge? |
| @@ -102,3 +87,18 @@ | |||
| .block-library-video-tracks-editor__add-tracks-container { | |||
| padding: $grid-unit-15; | |||
| } | |||
|
|
|||
| .block-library-video-tracks-editor__single-track-editor .components-base-control { | |||
There was a problem hiding this comment.
these look like editor specific styles so they should move to editor.scss right?
There was a problem hiding this comment.
Actually the whole changes on this file should probably move there.
There was a problem hiding this comment.
I'm not sure how these changes will affect the whole of the editor. My intention was to specifically target the video tracks popover.
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
35ff68e
to
d6aba31
Compare
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
d6aba31
to
ebdac99
Compare
jorgefilipecosta
force-pushed
the
add/video-tracks-functionality
branch
from
ebdac99
to
7691c41
Compare
| return tracks.map( ( track ) => { | ||
| return <track key={ track.src } { ...track } />; | ||
| } ); |
There was a problem hiding this comment.
Maybe we should use a different variable name in the loop that can't be confused with the <track /> tag name easily?
| return tracks.map( ( track ) => { | |
| return <track key={ track.src } { ...track } />; | |
| } ); | |
| return tracks.map( ( t ) => { | |
| return <track key={ t.src } { ...t } />; | |
| } ); |
Implements tracks adding and edition functionality on the video block.
Fixes: #7673
This PR follows the design proposed by @enriquesanchez with some adaptations because of the way our components work.
How has this been tested?
I added a video block, with a video file.
I used the tracks option on the toolbar.
I selected the upload option.
I verified a system window to select a file to upload opened.
I verified this window only allow to pick .vtt (all tracks on the tracks element must be in the vtt format).
I uploaded the file.
I saved the post and verified the track worked as expected.
I added another video block and used the tracks option.
I selected the "Open Media Library Option" and verified the media library only showed vtt files.
I verified I could edit the tracks.
I verified I could remove the tracks.
Screenshots