996331 - [webvtt] remove broken cuechange interface

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

When the list of active cues changes as a result of running the 'time marches on' algorithm, a TextTrack object should queue a 'cuechange' event.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#event-media-cuechange

Comment 1

[ramonjsantiago]

Super important for us. See this Chrome demo; http://www.youtube.com/watch?v=Pnb-r8uCpjE&feature=youtu.be

Comment 2

[Brad Dougherty]

This is very important for the Vimeo player (it's pretty annoying to work around because we have to do user agent checks). Is there any chance this will be fixed for 31 when text track support ships? If not, it would be nice if the "oncuechange" property didn't exist on TextTrack objects in JS so we can detect that way.

Comment 3

[Rick Eyre (:reyre)]

Hey Brad, thanks for letting us know. We've been waiting for bug 882718 to resolve this issue, but I may be able to get some functionality for this in before that lands.

Ralph, if I got a patch prepared for triggering these events ready do you think we could apply it to FF31?

Comment 4

[Ralph Giles (:rillian)]

We can ask, but it's 3 weeks to release, so it sounds risky to me. If the release manager says no we should pref off for 31 and try to ship the fix with 32.

Brad, is it better to have no TextTrack support at all, or to remove the oncuechange property? I'd be more comfortable with the later than a backported implementation in 31.

Comment 5

[Brad Dougherty]

I think we're ok with either option, although not having TextTrack support at all will require no extra work for us (not that big of a deal though). Removing the oncuechange property would at least allow us to work around it without having to do a user agent check.

Comment 6

[Ralph Giles (:rillian)]

Attachment: quick patch to disable oncuechange

Quick patch to remove the unimplemented oncuechange dom interface as suggested by vimeo.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8448359 [details] [diff] [review]
quick patch to disable oncuechange

Add smaug for dom review.

Comment 8

[Ralph Giles (:rillian)]

Green on try. https://tbpl.mozilla.org/?tree=Try&rev=3cb8ce9379b7

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8448359 [details] [diff] [review]
quick patch to disable oncuechange

So we had added oncuechange without any tests for it o_O

But this should be ok.


(I wonder what http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.webidl?rev=417acde736e7#42 is)

Comment 10

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c57f7df3876

Comment 11

[Ralph Giles (:rillian)]

(In reply to Olli Pettay [:smaug] from comment #9)

> But this should be ok.

Thanks.

> (I wonder what
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.
> webidl?rev=417acde736e7#42 is)

I saw that, and indeed copied from it. Is it the other half of the event implementation?

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c57f7df3876

Comment 13

[Brad Dougherty]

This isn't really fixed though.

Comment 14

[Ralph Giles (:rillian)]

(In reply to Brad Dougherty from comment #13)
> This isn't really fixed though.

Sorry, sloppy workflow on my part. I've opened bug 1033144 to continue tracking implementation.

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8448359 [details] [diff] [review]
quick patch to disable oncuechange

Approval Request Comment
[Feature/regressing bug #]: webvtt

[User impact if declined]:

If declined, authors depending on the cuechange event would have to agent sniff to work around.

[Describe test coverage new/current, TBPL]:

We have no test coverage TextTrack::cuechange either way. TBPL is green. Landed on m-c.

[Risks and why]:

Risk is low. The patch just removes a dummy property from the TextTrack object.

Vimeo depends on the the cuechange event for their subtitle support, but we don't yet implement it. They requested a way to detect that without doing version sniffing; disabling the dummy property gives them a way until we have a working implementation.

[String/UUID change made/needed]: None

Comment 16

[Ralph Giles (:rillian)]

Comment on attachment 8448359 [details] [diff] [review]
quick patch to disable oncuechange

Approval Request Comment
[Feature/regressing bug #]: webvtt

[User impact if declined]:

Authors would have to version sniff to work around.

[Describe test coverage new/current, TBPL]:

We have no test coverage for TextTrack::cuechange either way. TBPL is green.

[Risks and why]: 

Risk is low. The patch only removes a dummy dom property.

Vimeo uses the cuechange event in their player, but we don't yet implement it. They requested a way (other than agent sniffing) to detect that for work-around. Removing the dummy property gives them a way. If we can't do that, we should probably disable the entire webvtt feature in 31, which would be a shame.

[String/UUID change made/needed]:

No string change. A WebIDL change may imply a UUID change?

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8448359 [details] [diff] [review]
quick patch to disable oncuechange

Ralph, any particular reason why we don't have any tests here?
By the way, did you update the documentation?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/254653eab4a3

This bug as-filed is fixed. Setting the status flags as such for proper tracking.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/afdcf760d91b

Comment 20

[Ralph Giles (:rillian)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)

> Ralph, any particular reason why we don't have any tests here?

We knew we hadn't implemented it. Given the resources available for this feature, I thought it better to enable without first writing an xfail test.

There is some w3c work on this appearing as part of the web-platform-tests initiative. Once we land those our coverage should be a bit better.

> By the way, did you update the documentation?

I didn't find any docs to update, but I added a stub with implementation status at https://developer.mozilla.org/en-US/docs/Web/Reference/Events/cuechange

Comment 21

[Ralph Giles (:rillian)]

:adalucinet asked for verification steps. 

Open about:blank
Open devtools console.
Create a TextTrack object and verify it has no oncuechange attribute:

> var v = document.createElement('video');
undefined

> var t = v.addTextTrack('subtitles');
undefined

> t.oncuechange
undefined

> pprint(t);
"  addCue: function ()
  removeCue: function ()
  kind:
    get: function ()
    set: undefined
  label:
    get: function ()
    set: undefined
  language:
    get: function ()
    set: undefined
  id:
    get: function ()
    set: undefined
  inBandMetadataTrackDispatchType:
    get: function ()
    set: undefined
  mode:
    get: function ()
    set: function ()
  cues:
    get: function ()
    set: undefined
  activeCues:
    get: function ()
    set: undefined
  addEventListener: function ()
  removeEventListener: function ()
  dispatchEvent: function ()"

Comment 22

[Ralph Giles (:rillian)]

Attachment: verification page

I put together a quick test page with the above script, so simplify testing. It should be green on old builds, red on fixed builds.

Comment 23

[Ada [:adalucinet]]

Ralph, many thanks for STR!

Unfortunately, I don't see any green background color for "TextTrack object has no cuechange property." on builds before this fix. The background color is red, including on Firefox 31 beta 9 (Build ID: 20140710141843), latest Nightly (Build ID: 20140713030204) and Aurora (Build ID: 20140713004001) on Windows 7 x64, Mac OS X 10.9.4 and Ubuntu 13.04 x64. 

The only visible difference is when I call t.oncuechange on old builds it returns "null" and on latest builds is "undefined". Is this the expected result? Then why no green background color? 
Thanks in advance.

Comment 24

[Ralph Giles (:rillian)]

Attachment: verification page

Ummm, right you are. The test as written doesn't distinguish between 'property not present' and 'property present with a null or undefined' value.

Here's a new version using "if ('oncuechange' in track)" It's green for me on Nightly from July 1st, Chrome Dev and Safari, and red on current Nightly.

Comment 25

[Ada [:adalucinet]]

The background color is green on Nightly 2014-04-14 with the attachment from comment 24.
Verified as fixed with 31 RC (Build ID: 20140714151536), latest Aurora 32.0a2 (Build ID: 20140714004001) and Nightly 33.0a1 (Build ID: 20140714030201) builds on Windows 7 x64, Mac OS X 10.9.4 and Ubuntu 14.04 x32.