WebVTT / HTML5 track support too buggy to switch it on

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: info, Assigned: rillian)

Tracking

({dev-doc-complete})

28 Branch
mozilla29
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox28+ fixed, firefox29+ verified disabled, firefox30+ verified disabled, firefox31 affected, b2g-v1.3 disabled, b2g-v1.3T disabled, b2g-v1.4 disabled)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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?
(Reporter)

Comment 2

5 years ago
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

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
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

5 years ago
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.
(Reporter)

Comment 7

5 years ago
(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

5 years ago
(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.
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 ...
Status: UNCONFIRMED → NEW
status-firefox30: --- → ?
tracking-firefox29: --- → ?
tracking-firefox30: --- → ?
Ever confirmed: true
Flags: needinfo?(rick.eyre)
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.
tracking-firefox28: --- → ?
Flags: needinfo?(release-mgmt)

Comment 11

5 years ago
(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.
Flags: needinfo?(rick.eyre) → needinfo?(cpearce)
(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.
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.
Flags: needinfo?(release-mgmt)
Keywords: dev-doc-needed, relnote
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.
status-firefox28: --- → fixed
tracking-firefox29: ? → +
tracking-firefox30: ? → +
Thanks bz. I support the decision to disable WebVVT until a solution can be found.
Flags: needinfo?(cpearce)

Comment 17

5 years ago
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?
Flags: needinfo?(rick.eyre)

Comment 18

5 years ago
(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.
Flags: needinfo?(rick.eyre)
Updated the document: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/track
Keywords: dev-doc-needed, relnote → dev-doc-complete
OS: Mac OS X → All
Hardware: x86 → All
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?
status-firefox30: ? → ---
tracking-firefox28: ? → +

Comment 21

5 years ago
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.
OK. It would be nice to have the patch disabling it for beta 6 (next monday) or 7 (Thursday 10). Thanks

Comment 23

5 years ago
Sure. I can get it to you tomorrow.
Rick, any news on this subject? Thanks
Flags: needinfo?(rick.eyre)

Comment 25

5 years ago
(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.
Flags: needinfo?(rick.eyre)
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

5 years ago
Created attachment 8402713 [details] [diff] [review]
Disable WebVTT support on 29 r=bz,cpearce

Boris, I've modified test_interfaces.html, hence the review flag :-).
Attachment #8402713 - Flags: review?(cpearce)
Attachment #8402713 - Flags: review?(bzbarsky)

Comment 28

5 years ago
Created attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

I've modified test_interfaces.html on this one as well.
Attachment #8402715 - Flags: review?(cpearce)
Attachment #8402715 - Flags: review?(bzbarsky)

Comment 29

5 years ago
(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.
Of course, I wasn't suggesting to by pass the rules. :)

Comment 31

5 years ago
(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 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.
Attachment #8402713 - Flags: review?(cpearce) → review+
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.
Attachment #8402715 - Flags: review?(cpearce) → review+
Summary: HTML5 track support too buggy to switch it on → WebVTT / HTML5 track support too buggy to switch it on
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.
Attachment #8402713 - Flags: review?(bzbarsky) → review+
Comment on attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

r=me
Attachment #8402715 - Flags: review?(bzbarsky) → review+

Comment 36

5 years ago
(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.
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
Flags: needinfo?(rick.eyre)

Comment 38

5 years ago
(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.
Flags: needinfo?(rick.eyre)

Comment 39

5 years ago
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
Attachment #8402713 - Flags: approval-mozilla-beta?

Comment 40

5 years ago
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
Attachment #8402715 - Flags: approval-mozilla-aurora?
Attachment #8402713 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8402715 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks for the super fast reaction.

Comment 42

5 years ago
(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.
> Correct me if I'm wrong,

You're not wrong.  Thanks for explaining!

Updated

5 years ago
Attachment #8402713 - Flags: checkin?

Updated

5 years ago
Attachment #8402715 - Flags: checkin?
Comment on attachment 8402715 [details] [diff] [review]
Disable WebVTT support on 30 r=bz,cpearce

https://hg.mozilla.org/releases/mozilla-aurora/rev/64c58ec763c4
Attachment #8402715 - Flags: checkin? → checkin+
test_texttracklist.html needs pushPrefEnv for the disable to work.
Created attachment 8405535 [details] [diff] [review]
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
Attachment #8405535 - Flags: review?(cpearce)
Aurora patch backed out for test failure. See comment #45.
Passes content/media/test mochitests on my local Mac build.
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.
Attachment #8405535 - Flags: review?(cpearce) → review+
Having this for beta8 would be nice (gtb today at about 12 PDT)
Created attachment 8406231 [details] [diff] [review]
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
Attachment #8402715 - Attachment is obsolete: true
Attachment #8405535 - Attachment is obsolete: true
Attachment #8406231 - Flags: checkin+
status-b2g-v1.3: fixed → disabled
status-b2g-v1.3T: fixed → disabled
status-b2g-v1.4: --- → disabled
status-firefox29: --- → affected
status-firefox30: --- → disabled
Created attachment 8406294 [details] [diff] [review]
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
Attachment #8402713 - Attachment is obsolete: true
Attachment #8402713 - Flags: checkin?
Attachment #8406294 - Flags: checkin+
(Assignee)

Updated

5 years ago
status-firefox29: affected → disabled
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!
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Florin, can you please make sure this gets tested?
Flags: needinfo?(florin.mezei)
Keywords: verifyme
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
Flags: needinfo?(florin.mezei)
Keywords: verifyme
status-firefox29: disabled → verified disabled
status-firefox30: disabled → verified disabled
Assignee: nobody → giles
Target Milestone: --- → mozilla29
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
status-firefox31: --- → affected
Flags: needinfo?(giles)
Created attachment 8453932 [details] [diff] [review]
Disable WebVTT support in Firefox 31

Patch to disable the feature in firefox 31.

https://tbpl.mozilla.org/?tree=Try&rev=8400c977917d
Flags: needinfo?(giles)
Created attachment 8454013 [details] [diff] [review]
Disable WebVTT support in Firefox 31

Last patch didn't pass try. Disable unconditional feature test.
Attachment #8453932 - Attachment is obsolete: true
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
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.
(Reporter)

Comment 62

4 years ago
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.
OK. Let's ship it in 31 then \o/
Huzzah! :-)

Updated

4 years ago
Blocks: 1048120
You need to log in before you can comment on or make changes to this bug.