Skip to content

Video: Add option to set a track as default #70227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

yogeshbhutkar
Copy link
Contributor

@yogeshbhutkar yogeshbhutkar commented May 27, 2025

What?

Closes #37537

This PR adds an option to mark a particular track as the default track for the video block.

Why?

Setting a track as default ensures it's the one automatically selected for playback, providing a consistent user experience.

How?

This is done by setting the default property of the selected track to true.

Testing Instructions

  1. Create a new post.
  2. Insert a video block.
  3. Attach multiple tracks to the video block from the block toolbar.
  4. Set a particular track as the default.
  5. Confirm on the front-end that the default track (if set) gets displayed out of the box.
  6. Also, confirm that at most, only a single track can be default.

Testing Instructions for Keyboard

Same.

Screencast

PR-70227.720p.mov

@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review May 27, 2025 11:53
Copy link

github-actions bot commented May 27, 2025

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <yogeshbhutkar@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: Zodiac1978 <zodiac1978@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Block] Video Affects the Video Block labels May 28, 2025
@Mamaduka
Copy link
Member

Excellent work, @yogeshbhutkar!

Maybe we should reduce the spacing for nested VStack and use 4 instead of 8 there as well. What do you think?

Screenshots

Before
CleanShot 2025-05-28 at 10 18 53

After
CleanShot 2025-05-28 at 10 19 02

@yogeshbhutkar yogeshbhutkar force-pushed the enhance-37537/add-default-tracks branch from 1783e19 to bd45771 Compare May 28, 2025 06:40
@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented May 28, 2025

Thanks for the review, @Mamaduka.

I have a few more points regarding the Tracks UI that I'd like to discuss.

The Apply button in the Edit Tracks section feels redundant. Since each control already uses an onChange handler, changes are applied immediately, making the button unnecessary and potentially misleading.

Ref.

onChange={ ( newLabel ) =>
onChange( {
...track,
label: newLabel,
} )
}

In addition to reapplying existing values, the Apply button also sets default values when no input is provided. (I'd consider it to be a surprising behavior)

Ref.

if ( label === '' ) {
changes.label = __( 'English' );
hasChanges = true;
}
if ( srcLang === '' ) {
changes.srcLang = 'en';
hasChanges = true;
}

I think instead of language scoping the label, it's best to keep it as Track, as this is what Chrome displays for Tracks with empty labels. And instead of doing this on button press, I'd suggest adding these defaults while destructuring the data. (No surprises, the user knows the default values beforehand)

I believe it's best to discard the Apply button for these reasons. I also propose introducing a Back button to go to the previous component where all the tracks are listed.

Ref.
Screenshot

Currently, it's possible to save tracks with empty labels simply by leaving the label blank and clicking outside the popover. In Firefox, these tracks are displayed without any label, which could be confusing. We might want to consider either preventing tracks without labels from being rendered on the front-end or showing a warning if a label is missing.

Ref.
bug

P.S. I plan to address these points as follow-ups, depending on their feasibility.

@Mamaduka
Copy link
Member

The Apply button in the Edit Tracks section feels redundant. Since each control already uses an onChange handler, changes are applied immediately, making the button unnecessary and potentially misleading.

Having an 'Apply' button is generally better for accessibility and UX. It's better to keep it.

Currently, it's possible to save tracks with empty labels simply by leaving the label blank and clicking outside the popover.

This is a standard behavior for some popover forms to prevent content loss. We could refactor the track editor and keep the editing state internal, and only call onChange when the "Apply" button is pressed.

It would be helpful to understand the rationale behind introducing the default values logic. As far as I can tell, the <track> element has no required attributes.

Let's merge this, and feel free to follow up on some of these points in a new PR.

@Mamaduka Mamaduka merged commit 4a296ae into WordPress:trunk May 28, 2025
60 checks passed
@github-actions github-actions bot added this to the Gutenberg 21.0 milestone May 28, 2025
chriszarate pushed a commit to chriszarate/gutenberg that referenced this pull request Jul 1, 2025
Co-authored-by: yogeshbhutkar <yogeshbhutkar@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: Zodiac1978 <zodiac1978@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Video Affects the Video Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Video Text Tracks is not able to set default
2 participants