Fixup tracks some more #2494
Fixup tracks some more #2494
Conversation
…s us to not care about the 'tracks' option
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 91d71c0 (Please note that this is a fully automated comment.) |
| @@ -127,7 +127,7 @@ class Html5 extends Tech { | |||
|
|
|||
| // If the original tag is still there, clone and remove it. | |||
| if (el) { | |||
| const clone = el.cloneNode(false); | |||
| const clone = el.cloneNode(true); | |||
There was a problem hiding this comment.
Why is a deep clone necessary here (and not before)?
There was a problem hiding this comment.
Otherwise, we won't get tracks available on something like ios.
Previously, we'd grab all the tracks from the element and store them on the options object (which still happens) but then we need to pass it to the tech. However, if we pass through both that option and the textTracks object, if you swap between techs, you'll start getting duplicated tracks. So, instead, I can remove the need to pass through the tracks option to the tech and just clone the node with its sub nodes, since the tracks option should only have been used the first time the player was initialized.
There was a problem hiding this comment.
Hopefully, that makes sense.
|
lgtm |
| @@ -20,6 +20,7 @@ class TextTrackButton extends MenuButton { | |||
| constructor(player, options){ | |||
| super(player, options); | |||
|
|
|||
| this.update(); | |||
There was a problem hiding this comment.
this forces the buttons to update in case the video element has tracks from before we are able to register the handlers to update the buttons.
|
Got some travis errors |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: e585c5c (Please note that this is a fully automated comment.) |
|
lgtm |
No description provided.