Closed Bug 996331 Opened 6 years ago Closed 5 years ago

[webvtt] remove broken cuechange interface

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified

People

(Reporter: rillian, Assigned: rillian)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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
Blocks: webvtt
Blocks: 882718
Super important for us. See this Chrome demo; http://www.youtube.com/watch?v=Pnb-r8uCpjE&feature=youtu.be
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.
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?
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles) → needinfo?(brad)
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.
Flags: needinfo?(brad)
Quick patch to remove the unimplemented oncuechange dom interface as suggested by vimeo.
Attachment #8448359 - Flags: review?(cpearce)
Attachment #8448359 - Flags: review?(cpearce) → review+
Comment on attachment 8448359 [details] [diff] [review]
quick patch to disable oncuechange

Add smaug for dom review.
Attachment #8448359 - Flags: review?(bugs)
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)
Attachment #8448359 - Flags: review?(bugs) → review+
(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?
https://hg.mozilla.org/mozilla-central/rev/2c57f7df3876
Assignee: nobody → giles
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
This isn't really fixed though.
Summary: [webvtt] dispatch cuechange events → [webvtt] remove broken cuechange interface
(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 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
Attachment #8448359 - Flags: approval-mozilla-aurora?
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?
Attachment #8448359 - Flags: approval-mozilla-beta?
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?
Attachment #8448359 - Flags: approval-mozilla-beta?
Attachment #8448359 - Flags: approval-mozilla-beta+
Attachment #8448359 - Flags: approval-mozilla-aurora?
Attachment #8448359 - Flags: approval-mozilla-aurora+
Flags: needinfo?(giles)
https://hg.mozilla.org/releases/mozilla-aurora/rev/254653eab4a3

This bug as-filed is fixed. Setting the status flags as such for proper tracking.
(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
Flags: needinfo?(giles)
: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 ()"
Flags: needinfo?(alexandra.lucinet)
Attached file verification page (obsolete) —
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.
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.
Flags: needinfo?(alexandra.lucinet) → needinfo?(giles)
Attached file 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.
Attachment #8454065 - Attachment is obsolete: true
Flags: needinfo?(giles)
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.
You need to log in before you can comment on or make changes to this bug.