1253905 - HTMLMediaElement.oncuechange not receiving events

Status: RESOLVED DUPLICATE of bug 1548731

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

Description

[Mardeg]

Attachment: oncuechange testcase

Since bug 1033144 landed I noticed http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.webidl#42 still says:
//(Not implemented)attribute EventHandler oncuechange;

The attached testcase in Nightly builds fails to update the video.poster image oncuechange

Comment 1

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

Ruxton: would you be interested in handling this? We need to add an "oncuechange" to EventHandler.webidl.

Ralph: can you mentor again?

Comment 2

[Ralph Giles (:rillian)]

I am very happy to mentor.

Comment 3

[Mardeg]

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.

Comment 4

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

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?

Comment 5

[Mardeg]

Attachment: oncuechange testcase

Modified the testcase adding a checkbox to switch between broken attribute handler (this bug) and working event listener.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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 https://html.spec.whatwg.org/#event-handlers-on-elements,-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 http://hg.mozilla.org/mozilla-central/file/946ed22cad04/dom/base/nsGlobalWindow.h#l782 will produce a Get/SetOnCueChange on nsGlobalWindow.  Similarly, http://hg.mozilla.org/mozilla-central/file/dd1abe874252/dom/base/nsINode.h#l1960 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.

Comment 7

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

Attachment: MozReview Request: Bug 1253905 - Add WebIDL/event definitions for 'oncuechange' attribute. r?bz

Review commit: https://reviewboard.mozilla.org/r/39637/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39637/

Comment 8

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

Attachment: MozReview Request: Bug 1253905 - Dispatch cuechange to HTMLMediaElement as well as TrackElements and TextTracks. r?rillian

Review commit: https://reviewboard.mozilla.org/r/39639/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39639/

Comment 9

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

Thanks bz!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8730038 [details]
MozReview Request: Bug 1253905 - Add WebIDL/event definitions for 'oncuechange' attribute. r?bz

https://reviewboard.mozilla.org/r/39637/#review36385

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.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8730039 [details]
MozReview Request: Bug 1253905 - Dispatch cuechange to HTMLMediaElement as well as TrackElements and TextTracks. r?rillian

https://reviewboard.mozilla.org/r/39639/#review37961

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.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 13

[Eric Shepherd [:sheppy]]

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

Comment 14

[Alastor Wu [:alwu]]

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.