Closed Bug 1253905 Opened 8 years ago Closed 5 years ago

HTMLMediaElement.oncuechange not receiving events


(Core :: Audio/Video: Playback, defect, P3)




Tracking Status
firefox47 --- affected


(Reporter: mardeg, Unassigned, Mentored)


(Blocks 1 open bug)


(Keywords: dev-doc-needed)


(3 files, 1 obsolete file)

Attached file oncuechange testcase (obsolete) —
Since bug 1033144 landed I noticed still says:
//(Not implemented)attribute EventHandler oncuechange;

The attached testcase in Nightly builds fails to update the video.poster image oncuechange
Summary: oncuechange video attribute not firing events → oncuechange track attribute not firing events
Ruxton: would you be interested in handling this? We need to add an "oncuechange" to EventHandler.webidl.

Ralph: can you mentor again?
Flags: needinfo?(ruxton)
Flags: needinfo?(giles)
Priority: -- → P2
I am very happy to mentor.
Flags: needinfo?(giles)
Blocks: webvtt
Mentor: giles
oncuechange was just added to Firefox 47 release notes. What are the chances of, once this is patched on Nightly (48), being able to uplift it to 47 so we're not in the situation of announcing a half-broken feature?

That Ruxton has yet to answer about working on this is worrying.
I had a look at this. If uncomment "oncuechange" in EventHandlers.webidl, but I get compile errors in the bindings like so:

 3:08.00 In file included from /Users/CPearce/src/mozilla/central/obj-x86_64-apple-darwin15.3.0/dom/bindings/UnifiedBindings24.cpp:98:
 3:08.00 /Users/CPearce/src/mozilla/central/obj-x86_64-apple-darwin15.3.0/dom/bindings/WindowBinding.cpp:7213:44: error: no member named 'GetOncuechange' in 'nsGlobalWindow'
 3:08.00   RefPtr<EventHandlerNonNull> result(self->GetOncuechange());
 3:08.00                                      ~~~~  ^

However, I noted that the spec doesn't have an oncuechange defined for the HTMLTrackElement, only on the TextTrack object. The spec does say that a "cuechange" event can be fired at an HTMLTrackElement:

"For each text track in affected tracks, in the list order, queue a task to fire a simple event named cuechange at the TextTrack object, and, if the text track has a corresponding track element, to then fire a simple event named cuechange at the track element as well."

So we are supposed to fire "cuechange" at HTMLTrackElement, and we're not.

bz: should the above quote from the spec also mean we should be running oncuechange event handlers on HTMLTrackElements? Chrome is FWIW. And how do I fix the compile error, or otherwise ensure that oncuechange handlers set on HTMLTrackElements run? I don't understand other on$event handlers that are defined in EventHandlers.webidl such as oncanplaythrough work, but adding oncuechange doesn't?
Flags: needinfo?(ruxton) → needinfo?(bzbarsky)
Attached file oncuechange testcase
Modified the testcase adding a checkbox to switch between broken attribute handler (this bug) and working event listener.
Attachment #8727147 - Attachment is obsolete: true
Attachment #8729787 - Attachment mime type: text/plain → text/html
> However, I noted that the spec doesn't have an oncuechange defined for the HTMLTrackElement

If we're talking about the HTML spec, it defines an oncuechange in,-document-objects,-and-window-objects ("The following are the event handlers (and their corresponding event handler event types) that must be supported by all HTML elements...").  So there should be an oncuechange on all elements.  In terms of IDL, this is because the GlobalEventHandlers interface has an "oncuechange" attribute and "HTMLElement implements GlobalEventHandlers;".

So there should be an oncuechage on HTMLElement.prototype.

> bz: should the above quote from the spec also mean we should be running oncuechange event handlers on HTMLTrackElements?

The quote means the event should be fired.  The IDL for GlobalEventHandlers and prose I describe above means that setting oncuechange on any HTML element should add an event listener for the "cuechange" event.

> And how do I fix the compile error

You add an entry for cuechange to dom/events/EventNameList.h.  Note that there is already a commented-out placeholder there.  Then inclusion of that header in the code at will produce a Get/SetOnCueChange on nsGlobalWindow.  Similarly, will produce the getter and setter on nsINode, so it will exist (on the C++ side) on all elements and on document; in IDL it will only be exposed on the right kinds of elements depending on what the IDL says.

What I'm not 100% sure about is what the four arguments to the EVENT() macro should be here.  The first one should be cuechange, and the third one should be EventNameType_HTML (so we support things like <track oncuechange="..."> on HTML elements only).    What I'm not sure about are the second (event message) and fourth (event struct type).  Probably eBasicEventClass for that last, and add a new event message?  I think those go in widget/EventMessageList.h.
Flags: needinfo?(bzbarsky)
Summary: oncuechange track attribute not firing events → HTMLMediaElement.oncuechange not receiving events
Comment on attachment 8730038 [details]
MozReview Request: Bug 1253905 - Add WebIDL/event definitions for 'oncuechange' attribute. r?bz

r=me, but please add tests.  Specifically, test that <span oncuechange="something()"> with a manual event named "cuechange" then dispatched to it triggers the function, as does creating an element and setting its .cuechange.
Attachment #8730038 - Flags: review?(bzbarsky) → review+
Attachment #8730039 - Flags: review?(giles)
Comment on attachment 8730039 [details]
MozReview Request: Bug 1253905 - Dispatch cuechange to HTMLMediaElement as well as TrackElements and TextTracks. r?rillian

Thanks for taking this on, Chris. One question:

::: dom/html/HTMLTrackElement.cpp:341
(Diff revision 1)
> +HTMLTrackElement::DispatchCueChange()
> +{
> +  DispatchTrackRunnable(NS_LITERAL_STRING("cuechange"));
> +  if (mMediaParent) {
> +    mMediaParent->DispatchAsyncEvent(NS_LITERAL_STRING("cuechange"));
> +  }

I'm confused by this. According the the links from the bug discussion, we should support `oncuechange` on all HTML Elements, and `cuechange` should be queued by both `TextTrack` objects and their parent HTMLTrackElement, if any.

This seems to additionally queue a *third* `cuechange` even the track element's parent HTMLMediaElement. I still don't understand where this is specified.
Mass change P2 -> P3
Priority: P2 → P3

I assume this should be fixed now that bug 1548731 has been fixed?

Flags: needinfo?(drno)

Yes, after fixing bug 1548731, we can close this one. In addition, this bug is trying to add cuechange event on HTMLMediaElement, which is wrong. The spec said that we should only dispatch cuechange on HTMLTrackElement and TextTrack.

Closed: 5 years ago
Flags: needinfo?(drno)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.