981280 - WebVTT / HTML5 track support too buggy to switch it on

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alexander Farkas]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140306171728

Steps to reproduce:

open http://afarkas.github.io/webshim/demos/demos/mediaelement/track-demo.html 
and simply watch demo


Actual results:

- There is no carousel of cues from the texttrack[kind="metadata"] showing.
- starting with 75.2 the text (JSON) for the track[kind="metadata"].mode = 'hidden' is shown to the user, but activeCues.length is 0
- setting track to mode 'disabled' or track elements kind to 'metadata' should hide the text rendering
- there are no cuechange, enter/exit events dispatched.


Expected results:

Most of the issues are already known. It's just weired to me, that track implementation seems to be activated/enabled with FF 28 beta. Simply because it's not good enough to ship. 

At least, if the mode is changed to hidden/disabled or kind is changed to metadata FF should not show the textTracks at all.

Comment 1

[Ralph Giles (:rillian)]

Yeah, displaying hidden metadata cues is a bug.

I'm less clear what's going on with the carousel. I take it that's how the search results are supposed to be displayed? Is "could not find webshims-feature (aborted): json-storage" important?

Comment 2

[Alexander Farkas]

No, json-storage isn't important. 

The main carousel creation bug is quite simple and already a known issue (https://bugzilla.mozilla.org/show_bug.cgi?id=913016). Simply dispatch a load event manually: $('track').trigger('load');

If you do this, you will get at least two other bugs.

1. There is no cuechange event dispatched on the textTrack object
2. textTrack.activeCues has always a length of 0 with no cues

Comment 3

[Rick Eyre (:reyre)]

Yeah, most of these are known issues:

TextTrack's with kind 'hidden' showing: https://bugzilla.mozilla.org/show_bug.cgi?id=977299
oncuechange event: https://bugzilla.mozilla.org/show_bug.cgi?id=952130
TextTrack's with kind 'metadata' showing: https://bugzilla.mozilla.org/show_bug.cgi?id=977302

I don't know why textTrack.activeCues has a length of 0 though, that might be a bug.

Comment 4

[Ralph Giles (:rillian)]

activeCues doesn't work in FF28 (beta) probably because bug 921484 didn't land until FF29.

It's unfortunate that the demo doesn't work in 28. We enabled it at that point because I thought passive display support was useful even if all the events weren't hooked up yet.

Comment 5

[Alexander Farkas]

Yeah, I do understand this. But it isn't really about the missing events for the demo. The current implementation has 2 other problems. Both problems together make this feature not only partiall, it make this feature so buggy, that it might break existing sites...

1. even if there is no default attribute. At least the last track will be shown (even it is a metadata track)
2. Setting the textTrack.mode to disabled/hidden, the kind to 'metadata' and/or simply removing the track element (+ invoke the load method on the mediaelement) won't remove the textTracks visibility

Untill you don't fix at least one of those two bugs, it might break existing sites. FF is in this case the last browser which will add support for this feature. So it's really not the time to enable a very buggy feature.

Comment 6

[Rick Eyre (:reyre)]

If we don't enable it now that would mean waiting to FF31+ to enable, if we go through the regular release channels, I believe. 

(In reply to Alexander Farkas from comment #5)

> 2. Setting the textTrack.mode to disabled/hidden, the kind to 'metadata' and/or simply removing the
> track element (+ invoke the load method on the mediaelement) won't remove the textTracks visibility

Yeah, the only way to stop showing it would be to remove the relevant TextTrack from the MediaElement.

(In reply to Ralph Giles (:rillian) from comment #4)
> activeCues doesn't work in FF28 (beta) probably because bug 921484 didn't
> land until FF29.
> 
> It's unfortunate that the demo doesn't work in 28. We enabled it at that
> point because I thought passive display support was useful even if all the
> events weren't hooked up yet.

Ah, yes, that makes sense.

Comment 7

[Alexander Farkas]

(In reply to Rick Eyre (:reyre) from comment #6)
> If we don't enable it now that would mean waiting to FF31+ to enable, if we
> go through the regular release channels, I believe. 
> 
> (In reply to Alexander Farkas from comment #5)
> 
> > 2. Setting the textTrack.mode to disabled/hidden, the kind to 'metadata' and/or simply removing the
> > track element (+ invoke the load method on the mediaelement) won't remove the textTracks visibility
> 
> Yeah, the only way to stop showing it would be to remove the relevant
> TextTrack from the MediaElement.
> 

How? What is the easiest way/less obtrusiv way to do so? From my testing cloning the mediaelement then removing the track element and then replacing the original mediaelement is possible, but quiet obtrusiv.

Comment 8

[Rick Eyre (:reyre)]

(In reply to Alexander Farkas from comment #7)
> How? What is the easiest way/less obtrusiv way to do so? From my testing
> cloning the mediaelement then removing the track element and then replacing
> the original mediaelement is possible, but quiet obtrusiv.

My mistake Alexander. You can't remove TextTracks from the MediaElement directly. This seems strange to me, perhaps we should file a bug to make that happen.

I've tested whether or not removing the Track element in Nightly removes the TextTrack from the MediaElement and it does. So the capability to do that is in the pipeline, just not in FF29, unfortunately.

I think your solution may be the only one for now.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Rick, could you file a new bug about removing TextTracks from the MediaElement as you mention in commment 8?   And, do you want to track this bug differently? From the comments, I'm still not sure what the next step should be or which tracking flags to raise. 

I'll mark it NEW for now since there does seem to be a known issue and unresolved. 

Anthony, I'm cc-ing you in to this bug for advice about how to untangle doing the QA for this bundle of issues. Maybe there is a good meta bug already ...

Comment 10

[u279076]

Liz, thanks for bringing this to my attention. I don't think there's anything for QA to do here until we have a solution to test. I'm flagging Release Management on this bug to decide how to proceed for Firefox 28, though I'm skeptical anything can be done at this point. Best case scenario might be to ship WebVTT as is in Firefox 28 and uplift the solution in comment 8 to Firefox 29.

Comment 11

[Rick Eyre (:reyre)]

(In reply to Liz Henry :lizzard from comment #9)
> Rick, could you file a new bug about removing TextTracks from the
> MediaElement as you mention in commment 8?   And, do you want to track this
> bug differently? From the comments, I'm still not sure what the next step
> should be or which tracking flags to raise. 
I'm not sure whether or not the spec needs that method I discussed in comment 8. Alexander's need for it seems to be not because the spec needs it, but because our implementation of WebVTT isn't finished yet. So he needs to do workarounds.

I'm not quite sure what would be the best way to track this. Chris what do you think we should do about this bug?

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> Best case scenario might be to ship WebVTT as is in Firefox 28 and uplift the solution in
> comment 8 to Firefox 29.
Implementing that change would be exposing a feature that isn't yet in the spec so we might want to wait until the spec agrees before changing Firefox.

Comment 12

[u279076]

(In reply to Rick Eyre (:reyre) from comment #11)
> Implementing that change would be exposing a feature that isn't yet in the
> spec so we might want to wait until the spec agrees before changing Firefox.

Good point.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

We're currently taking security fixes in preparation for a final FF28 build today - can we disable this by simply backing out the enabling patch in bug 887978?  That would be the only option here - if that's not a clean return to known-good state we should release note this as being 'experimental' support and let it be what it is, with followup fixes in 29.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ralph, Chris, I'm sorry you aren't around to jump in here...

I talked to Rick about the impact of the issues described above.  The biggest problem seems to be that they will in fact bite people who are using webvtt _and_ that working around them is not trivial.  In particular, the common webvtt polyfills would not work right with the workarounds.

Given that, we agreed to disable webvtt on beta 28.  https://hg.mozilla.org/releases/mozilla-beta/rev/a9e2dddb09b7

We need a plan for 29 and later, of course, so leaving this bug open.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/a9e2dddb09b7
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a9e2dddb09b7

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks bz. I support the decision to disable WebVVT until a solution can be found.

Comment 17

[Ada [:adalucinet]]

WebVTT is properly disabled on Firefox 28 RC (Build ID: 20140314220517): media.webvtt.enabled is set to false by default on:
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:28.0) Gecko/20100101 Firefox/28.0

I noticed that the demo from Comment 0 is still displaying a subtitle, so I saved it on my local drive (along with the vtt files) and concluded that the texttrack[kind="metadata"] is shown only if I enabled WebVTT (which is expected), while the other track is not shown at all. 

On the other hand, if I navigate to http://afarkas.github.io/webshim/demos/demos/mediaelement/track-demo.html, the track is displayed. I couldn't figure out why. Any thoughts on this?

Comment 18

[Rick Eyre (:reyre)]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #17)
The subtitles that are showing are from the polyfill for WebVTT and the Track element that are running on that page. So the subtitles there are being generated by JavaScript on the page and not from Firefox.

From looking at it, it looks like it's been disabled correctly.

Comment 19

[Kohei Yoshino [:kohei]]

Updated the document: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/track

Comment 20

[Sylvestre Ledru [:Sylvestre]]

What is the status of this bug for 29? Looks like this bug is still open but I don't think WebVTT has been disabled?

Comment 21

[Rick Eyre (:reyre)]

We've been in the process of determining what to do for 29/30. It looks like we're going to disable it in them as well since the issues we disabled WebVTT for in 28 have only just been fixed in 31. I'll have a patch to disable them in 29/30 ready soon if that is the decision we make.

Check out bug 985977 for where that discussion is taking place.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

OK. It would be nice to have the patch disabling it for beta 6 (next monday) or 7 (Thursday 10). Thanks

Comment 23

[Rick Eyre (:reyre)]

Sure. I can get it to you tomorrow.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Rick, any news on this subject? Thanks

Comment 25

[Rick Eyre (:reyre)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Rick, any news on this subject? Thanks

Yep, I'll have a patch to disable it today. Sorry I didn't get it in last Friday like I promised. I had some technical difficulties with my MacBook charger.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

No worries. Do you think we could have the patch in a couple hour? I am going to launch the build of beta6 in a 3 or 4 hours and it would be nice to have it too. Thanks

Comment 27

[Rick Eyre (:reyre)]

Attachment: Disable WebVTT support on 29 r=bz,cpearce

Boris, I've modified test_interfaces.html, hence the review flag :-).

Comment 28

[Rick Eyre (:reyre)]

Attachment: Disable WebVTT support on 30 r=bz,cpearce

I've modified test_interfaces.html on this one as well.

Comment 29

[Rick Eyre (:reyre)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> No worries. Do you think we could have the patch in a couple hour? I am
> going to launch the build of beta6 in a 3 or 4 hours and it would be nice to
> have it too. Thanks

I'd like to wait on review from Boris and Chris first, just to make sure everything is good before we do anything. Hopefully, we make it for the beta build.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Of course, I wasn't suggesting to by pass the rules. :)

Comment 31

[Rick Eyre (:reyre)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #30)
> Of course, I wasn't suggesting to by pass the rules. :)

Didn't mean to imply that you wanted to, if that's how I sounded :).

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8402713 [details] [diff] [review]
Disable WebVTT support on 29 r=bz,cpearce

Review of attachment 8402713 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce for disabling. bz needs to r+ the other stuff.

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

Review of attachment 8402715 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce for disabling. bz needs to r+ the other stuff.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8402713 [details] [diff] [review]
Disable WebVTT support on 29 r=bz,cpearce

r=me, though we should consider making those test_interfaces entries just conditional on the pref.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

r=me

Comment 36

[Rick Eyre (:reyre)]

(In reply to Boris Zbarsky [:bz] from comment #34)

> r=me, though we should consider making those test_interfaces entries just
> conditional on the pref.

Correct me if I'm wrong, but it seemed like test_interfaces.html wasn't able to deal with interfaces with multiple prefs on it like VTTRegion and VTTRegionList so I opted for removing them from the list entirely.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Can we get the uplift request asap? I would like to see the beta patch in beta7 (go to build in a few hours). Thanks

Comment 38

[Rick Eyre (:reyre)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #37)
> Can we get the uplift request asap? I would like to see the beta patch in
> beta7 (go to build in a few hours). Thanks

Sure, I'll ask for approval now.

Comment 39

[Rick Eyre (:reyre)]

Comment on attachment 8402713 [details] [diff] [review]
Disable WebVTT support on 29 r=bz,cpearce

[Approval Request Comment]
Bug caused by (feature/regressing bug #): WebVTT / Track
User impact if declined: Buggy support for the WebVTT / Track features that break existing polyfills.
Testing completed (on m-c, etc.): No.
Risk to taking this patch (and alternatives if risky): Pages with Track support on them will not function correctly and the workarounds for web devs to fix this will be very hard if at all possible.
String or IDL/UUID changes made by this patch: N/A

Comment 40

[Rick Eyre (:reyre)]

Comment on attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

[Approval Request Comment]
Bug caused by (feature/regressing bug #): WebVTT / Track
User impact if declined: Buggy support for the WebVTT / Track features that break existing polyfills.
Testing completed (on m-c, etc.): No.
Risk to taking this patch (and alternatives if risky): Pages with Track support on them will not function correctly and the workarounds for web devs to fix this will be very hard if at all possible.
String or IDL/UUID changes made by this patch: N/A

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Thanks for the super fast reaction.

Comment 42

[Rick Eyre (:reyre)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #41)
> Thanks for the super fast reaction.

No worries! Thanks for your help getting this in as well.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> Correct me if I'm wrong,

You're not wrong.  Thanks for explaining!

Comment 44

[Ralph Giles (:rillian)]

Comment on attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

https://hg.mozilla.org/releases/mozilla-aurora/rev/64c58ec763c4

Comment 45

[Ralph Giles (:rillian)]

test_texttracklist.html needs pushPrefEnv for the disable to work.

Comment 46

[Ralph Giles (:rillian)]

Attachment: Add pushPrefEnv to test_texttracklist.html

Testing a patch to fix the test breakage. Pushed to try on top of the aurora patch as https://tbpl.mozilla.org/?tree=Try&rev=5cf625dfa352

Comment 47

[Ralph Giles (:rillian)]

Aurora patch backed out for test failure. See comment #45.

Comment 48

[Ralph Giles (:rillian)]

Comment on attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

https://hg.mozilla.org/releases/mozilla-aurora/rev/4518ab9c2587

Comment 49

[Ralph Giles (:rillian)]

Passes content/media/test mochitests on my local Mac build.

Comment 50

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8405535 [details] [diff] [review]
Add pushPrefEnv to test_texttracklist.html

Review of attachment 8405535 [details] [diff] [review]:
-----------------------------------------------------------------

Please indent the code inside the new function you added, so it's obvious it's part of the function.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Having this for beta8 would be nice (gtb today at about 12 PDT)

Comment 52

[Ralph Giles (:rillian)]

Attachment: Disable WebVTT support in Firefox 30

Fix test to pass after the pref flip, incorporating review comment. Carrying forward r=bz,cpearce a=sledru

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad066acbefaa

Comment 53

[Ralph Giles (:rillian)]

Attachment: Disable WebVTT support in Firefox 29

Seems to be passing tests on aurora; landing on beta. Folded in the test fix. Carrying forward r=cpearce,bz a=sledru

https://hg.mozilla.org/releases/mozilla-beta/rev/3a3224245147

Comment 54

[Ralph Giles (:rillian)]

Patches for 29 and 30 seem to have stuck. I've opened bugs 996331 and 996333 for the missing events. Otherwise the issues from the description seem to be resolved with the code current in Firefox 31 Nightly. Therefore treating this bug as resolved. Thanks for testing!

Comment 55

[u279076]

Florin, can you please make sure this gets tested?

Comment 56

[Cornel Ionce [:noni] [Hubs QA]]

Tested on Windows 7 64bit, Ubuntu 13.04 and Mac OS X 10.9.

WebVTT is properly disabled on Fx 29 and 30.
-- media.webvtt.enabled is set to false by default on Firefox 29 beta 8 (build ID: 20140414143035) and on latest Aurora (build ID: 20140415004003). 


On latest Nightly (build ID: 20140414030203) the media.webvtt.enabled pref is set to true by default.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Because of bugs 975447 & bug 992664, we are considering to disable WebVTT in 31.
Ralph, we haven't decided if we are going to do it or not but could you prepare a patch in case we disable it? Thanks

Comment 58

[Ralph Giles (:rillian)]

Attachment: Disable WebVTT support in Firefox 31

Patch to disable the feature in firefox 31.

https://tbpl.mozilla.org/?tree=Try&rev=8400c977917d

Comment 59

[Ralph Giles (:rillian)]

Attachment: Disable WebVTT support in Firefox 31

Last patch didn't pass try. Disable unconditional feature test.

Comment 60

[Ralph Giles (:rillian)]

This patch passed try.

As far as the feature goes, I don't feel strongly about it, but Rick and I both prefer releasing it as is.

- There's no ui for enabling tracks, so this is only available under the control of page authors.
- It's not widely used, and most usage I've seen has been by library authors, who can work around the rough edges.
- The feature is generally useful for accessibility even without full event support.
- We have a two-line patch to disable the feature. We can apply that in a point release if it causes problems.

Whatever your final decision for Firefox 31, we'll continue to improve the implementation as time permits.

https://tbpl.mozilla.org/?tree=Try&rev=5623b529e4eb

Comment 61

[u279076]

I spoke with Ralph about this at lunch today and I also don't have any reservations about releasing this as it is. My primary concern was stability and it seems those issues have been addressed. So I say let's just release it and keep an eye on feedback. We can always pref it off if we get explosively negative feedback.

Comment 62

[Alexander Farkas]

As a developer using track with audio and video, I don't see a big problem with bug 992664 (Although nasty). -> I would like to see this feature enabled.

Comment 63

[Sylvestre Ledru [:Sylvestre]]

OK. Let's ship it in 31 then \o/

Comment 64

[Ralph Giles (:rillian)]

Huzzah! :-)