fix: TextTrackMenuItem components should not disable text tracks of different kind(s). #5741
Conversation
| super.handleClick(event); | ||
|
|
||
| if (!tracks) { | ||
| return; | ||
| } | ||
|
|
||
| // Determine the relevant kind(s) of tracks for this component. | ||
| const kinds = (referenceTrack.kinds || [referenceTrack.kind]).filter(Boolean); |
There was a problem hiding this comment.
Could be good to note that we also filter out any invalid kinds in a comment.
|
|
||
| // If this text track is the component's track and it is not showing, | ||
| // set it to showing. | ||
| if (track === referenceTrack) { | ||
| if (track.mode !== 'showing') { |
There was a problem hiding this comment.
This seems like it belongs in the setter for mode in TextTrack if its not already there. Not that we have to change it in this pr.
There was a problem hiding this comment.
it's theoretically possible (and maybe eventually we will) to have multiple tracks enabled at the same time, while we disallow it via the UI, we shouldn't limit it if people want to enable multiple items directly via the API.
There was a problem hiding this comment.
it's possible I misread your comment. Do you mean verifying that if we set the mode to the same thing it'll be a no-op?
| super.handleClick(event); | ||
|
|
||
| if (!tracks) { | ||
| return; | ||
| } | ||
|
|
||
| // Determine the relevant kind(s) of tracks for this component. | ||
| const kinds = (referenceTrack.kinds || [referenceTrack.kind]).filter(Boolean); |
There was a problem hiding this comment.
After looking at the tests I was going to suggest that we add a tests for a track with multiple kinds. Then I looked at the HTML5 video spec and our track implementation and it seems like kinds isn't actually a thing. Only kind should ever exist. Perhaps we can keep it for now but add some code about deprecating this usage?
There was a problem hiding this comment.
kinds is a thing that was added for the combined subtitles/captions menu, it should be considered an implementation detail and not used outside of Video.js internals.
There was a problem hiding this comment.
Yeah, it might have been good to name it in a way that was more obviously an implementation detail because that confused me at first, too. Oh well.
|
also I chuckled at the TDD commit 👍 |
|
protip: push the test without the change first to see it fail 🙂 |
|
|
||
| QUnit.module('TextTrackMenuItem', { | ||
| beforeEach(assert) { | ||
| this.clock = sinon.useFakeTimers(); |
There was a problem hiding this comment.
Doesn't seem so. Copy/paste!
|
|
||
| const foo = this.player.addTextTrack('captions', 'foo', 'en'); | ||
|
|
||
| const fooItem = new TextTrackMenuItem(this.player, { |
There was a problem hiding this comment.
fooItem and barItem below aren't cleaned up either.
There was a problem hiding this comment.
Won't they be disposed with the player in the afterEach?
There was a problem hiding this comment.
this object is never added to a menu/the player, so it won't get disposed automatically. It's probably not a huge deal but best to clean up as much as we can to reduce any potential memory leak issues with tests.
|
It is a separate commit, but you can trust me that it failed. 😉 |
|
I can confirm that tests pass :) |
|
Also, tested it locally and works great. |
This fixes a regression created by #5741.
This fixes a regression created by videojs#5741.
Description
While investigating a non-public issue, I happened to notice that metadata tracks were being disabled when switching captions languages. Upon investigation, I found this code, which is the culprit. It will disable any track that's not the selected track.
Specific Changes proposed
Instead of disabling all tracks when switching, only disable tracks of the appropriate kind(s).
Requirements Checklist