-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
fix: TextTrackMenuItem components should not disable text tracks of different kind(s). #5741
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,27 +93,37 @@ class TextTrackMenuItem extends MenuItem { | |
* @listens click | ||
*/ | ||
handleClick(event) { | ||
const kind = this.track.kind; | ||
let kinds = this.track.kinds; | ||
const referenceTrack = this.track; | ||
const tracks = this.player_.textTracks(); | ||
|
||
if (!kinds) { | ||
kinds = [kind]; | ||
} | ||
|
||
super.handleClick(event); | ||
|
||
if (!tracks) { | ||
return; | ||
} | ||
|
||
// Determine the relevant kind(s) of tracks for this component and filter | ||
// out empty kinds. | ||
const kinds = (referenceTrack.kinds || [referenceTrack.kind]).filter(Boolean); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
for (let i = 0; i < tracks.length; i++) { | ||
const track = tracks[i]; | ||
|
||
if (track === this.track && (kinds.indexOf(track.kind) > -1)) { | ||
// If the track from the text tracks list is not of the right kind, | ||
// skip it. We do not want to affect tracks of incompatible kind(s). | ||
if (kinds.indexOf(track.kind) === -1) { | ||
continue; | ||
} | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like it belongs in the setter for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
track.mode = 'showing'; | ||
} | ||
|
||
// If this text track is not the component's track and it is not | ||
// disabled, set it to disabled. | ||
} else if (track.mode !== 'disabled') { | ||
track.mode = 'disabled'; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* eslint-env qunit */ | ||
import TextTrackMenuItem from '../../../../src/js/control-bar/text-track-controls/text-track-menu-item.js'; | ||
import TestHelpers from '../../test-helpers.js'; | ||
|
||
QUnit.module('TextTrackMenuItem', { | ||
beforeEach(assert) { | ||
this.player = TestHelpers.makePlayer(); | ||
}, | ||
afterEach(assert) { | ||
this.player.dispose(); | ||
} | ||
}); | ||
|
||
QUnit.test('clicking should enable the selected track', function(assert) { | ||
assert.expect(2); | ||
|
||
const foo = this.player.addTextTrack('captions', 'foo', 'en'); | ||
|
||
const fooItem = new TextTrackMenuItem(this.player, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this isn't cleaned up/disposed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't they be disposed with the player in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, right. |
||
track: foo | ||
}); | ||
|
||
assert.strictEqual(foo.mode, 'disabled', 'track "foo" begins "disabled"'); | ||
|
||
fooItem.trigger('click'); | ||
|
||
assert.strictEqual(foo.mode, 'showing', 'clicking set track "foo" to "showing"'); | ||
|
||
fooItem.dispose(); | ||
}); | ||
|
||
QUnit.test('clicking should disable non-selected tracks of the same kind', function(assert) { | ||
assert.expect(9); | ||
|
||
const foo = this.player.addTextTrack('captions', 'foo', 'en'); | ||
const bar = this.player.addTextTrack('captions', 'bar', 'es'); | ||
const bop = this.player.addTextTrack('metadata', 'bop'); | ||
|
||
bop.mode = 'hidden'; | ||
|
||
const fooItem = new TextTrackMenuItem(this.player, { | ||
track: foo | ||
}); | ||
|
||
const barItem = new TextTrackMenuItem(this.player, { | ||
track: bar | ||
}); | ||
|
||
assert.strictEqual(foo.mode, 'disabled', 'captions track "foo" begins "disabled"'); | ||
assert.strictEqual(bar.mode, 'disabled', 'captions track "bar" begins "disabled"'); | ||
assert.strictEqual(bop.mode, 'hidden', 'metadata track "bop" is "hidden"'); | ||
|
||
barItem.trigger('click'); | ||
|
||
assert.strictEqual(foo.mode, 'disabled', 'captions track "foo" is still "disabled"'); | ||
assert.strictEqual(bar.mode, 'showing', 'captions track "bar" is now "showing"'); | ||
assert.strictEqual(bop.mode, 'hidden', 'metadata track "bop" is still "hidden"'); | ||
|
||
fooItem.trigger('click'); | ||
|
||
assert.strictEqual(foo.mode, 'showing', 'captions track "foo" is now "showing"'); | ||
assert.strictEqual(bar.mode, 'disabled', 'captions track "bar" is now "disabled"'); | ||
assert.strictEqual(bop.mode, 'hidden', 'metadata track "bop" is still "hidden"'); | ||
|
||
fooItem.dispose(); | ||
barItem.dispose(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be good to note that we also filter out any invalid kinds in a comment.