Adding vtt.js's exports to window breaks addCue in Chrome

[justinanastos]

Description

In tech.js#addWebVttScript_, all of the exports from vtt.js are blindly added to window. This will break the ability to call Chrome's native TextTrack.prototype.addCue() because Chrome expects an instance of it's native VTTCue and not the vtt.js version.

This will effect all use cases of calling textTrack.addCue(). I found the issue while trying to restore text track functionality to videojs-contrib-dash in videojs/videojs-contrib-dash#135. The details of the error can be seen specifically in this comment videojs/videojs-contrib-dash#135 (comment).

Reduced Test Case

http://jsbin.com/tomegu/7/edit?html,js,output

The console will show the error thrown.

Results

Expected

I expect TextTrack.addCue(new VTTCue( ... )) to add cues.

Actual

Chrome will throw an error, shown below.

Error output

Uncaught TypeError: Failed to execute 'addCue' on 'TextTrack': parameter 1 is not of type 'TextTrackCue'.

Additional Information

versions

videojs

I haven't checked any versions besides v5.16, but the same code exists in master so I assume all of the version.

browsers

Chrome

OSes

Only tested on MacOS 10.12.3 (latest as of this writing)

plugins

None

Resolution

I propose that only values that do not exist on window are defined when iterating through vtt.js's exports.

This reduced test case shows a cue being added as expected when using the native VTTCue in Chrome.

http://jsbin.com/yelizon/5/edit?html,js,output

[gkatsev]

Yeah, that sounds like an issue. Historically, we haven't needed to handle this because either native tracks or emulated tracks would be used. The current proposed fix, unfortunately, will break emulated caption support, at least with the current version of vttjs that we're using. If it doesn't, let me know I'm wrong!
We already have an issue for updating vttjs (#3819) and we recently updated how vttjs is included. However, it seems like we need to spend some more time on this, potentially making vttjs be much more contained within tracks.

[justinanastos]

@gkatsev I'm minimally versed with emulated captions so please forgive the naive question: why would this break it? Is the shimmed VTTCue from vtt.js adding some functionality that doesn't exist natively anywhere?

---

Edit: Added anywhere to the end.

[gkatsev]

I believe that there's some extra stuff that's currently happening in there that isn't available natively for the features we support with emulated tracks. I do remember that it wasn't sufficient to use native cues at the time I implemented it. It's possible things have changed and we probably should verify. Maybe we would only want to do this for older browsers or something. Though, I think making vttjs more contained with our emulated tracks functionality sounds like a good move regardless.

[justinanastos]

Thanks @gkatsev .

[gkatsev]

Thanks for looking into this @justinanastos.

[gkatsev]

I took a look today, I think the way everything is set up can be tweaked to fix the issue and also do it as a minor (so we can fix it in 5.x as well). I'll be working on this some more tomorrow.

[justinanastos]

Awesome, any way I can help @gkatsev ?

[gkatsev]

I'll ping you when I have something ready to test, thanks @justinanastos.

[gkatsev]

@justinanastos hey, I have a PR open against our vttjs fork with updates (videojs/vtt.js#7), I also published it as a beta tag on npm [email protected]. If you can give it a shot, that would be awesome.

[gkatsev]

Just realized that we needed an extra piece here: #4115. Please try out that branch, it's currently including the pre-release of vttjs in the PR.