"Simple events" on HTML5 media shouldn't be cancelable

RESOLVED FIXED in mozilla17

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jeremy Banks, Assigned: kinetik)

Tracking

unspecified
mozilla17
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_3) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.47 Safari/536.11

Steps to reproduce:

I used addEventListener to add listeners for the "play", "playing", "pause", "seeking", "seeked" and "timeupdate" events on a <video> element, twice.

The first time I just logged the event objects to the console. The second time I called their .preventDefault() method first.


Actual results:

The .cancelable property was set true on all of the events.

Calling .preventDefault() did not cancel any of the actions, though it did set the .defaultPrevented property to true.


Expected results:

In the current working draft of the specification for HTML5 media elements and events, all of the above are described as "simple events". Simple events are not supposed to be cancelable unless otherwise specified, and it was not otherwise specified for these events.

Therefore I would expect the .cancelable property to be false and .preventDefault() to have no effect, even on the value of .defaultPrevented.

Tested in Firefox Nightly. (Chrome, Safari and Opera's stable releases exhibit the same behaviour. Perhaps I'm confused, but the specification seem unambiguous.)
Component: Untriaged → General
Product: Firefox → Core
Component: General → Video/Audio
Assignee: nobody → kinetik
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 651620 [details] [diff] [review]
patch v0 (wrong patch)

Oops, we fixed bubbling in bug 608630 but missed cancellation.

Simple patch attached.  Mochitests are green locally; pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6a2f068a9fb4

This patch also changes the MozAudioAvailable events to not cancel *or* bubble, which seems like what we'd want.
Attachment #651620 - Flags: review?(roc)
If Chrome, Safari, Opera and us (what about IE?) all have already made these events cancelable, shouldn't we change the spec instead of changing our behavior?
There's no behaviour specified for cancelling media events, and I don't know what'd mean to be able to cancel most of them.

From a quick look, I can't see any code in WebKit/Chromium's media implementation that handles media events being cancelled.
OK. I guess it probably doesn't make any difference to Web content.

You uploaded the wrong patch though :-)
Comment on attachment 651620 [details] [diff] [review]
patch v0 (wrong patch)

Oops.
Attachment #651620 - Attachment description: patch v0 → patch v0 (wrong patch)
Attachment #651620 - Attachment is obsolete: true
Attachment #651620 - Flags: review?(roc)
Created attachment 651646 [details] [diff] [review]
patch v0

Correct patch attached this time.

And a little more history:

The spec used to say that simple events were not bubbling but were cancellable and was changed to the current wording in in April 2009: http://html5.org/tools/web-apps-tracker?from=2991&to=2992

Our implementation of nsHTMLMediaElement::DispatchEvent was committed July 2008 and initially creating bubbling and cancellable events--bubbling was fixed in November 2010.  WebKit's implementation dates from late 2007 and created cancellable non-bubbling events from inception.

So I think this is just a case of the spec changing and nobody noticing.  I've made a note to file bugs against the other browsers to fix their implementations.
Attachment #651646 - Flags: review?(roc)
Attachment #651646 - Flags: review?(roc) → review+
WebKit bug (also covers Chrom{e,ium}): https://bugs.webkit.org/show_bug.cgi?id=94065
Opera bug: DSK-372112@bugs.opera.com
Blocks: 782876
IE 9's events aren't cancellable and don't bubble.  Nor are IE 10 (pp2, I don't have Windows 8 to test later versions).
https://hg.mozilla.org/integration/mozilla-inbound/rev/806f04d54ab5
Flags: in-testsuite?
Target Milestone: --- → mozilla17
Created attachment 652677 [details] [diff] [review]
patch v0 partial revert

I realized that I should not have changed InitAudioAvailableEvent (since DOM events inits share common parameters at the start, and my change breaks that pattern), and instead should just pass the appropriate args to InitAudioAvailableEvent from nsHTMLMediaElement.

No need for a review, since this is just a partial revert of my previous patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b83bfbfa9b60
Attachment #652677 - Attachment description: patch v0 partial revent → patch v0 partial revert
(In reply to Matthew Gregan [:kinetik] from comment #10)
> Created attachment 652677 [details] [diff] [review]
> patch v0 partial revert
> 
> I realized that I should not have changed InitAudioAvailableEvent (since DOM
> events inits share common parameters at the start, and my change breaks that
> pattern), and instead should just pass the appropriate args to
> InitAudioAvailableEvent from nsHTMLMediaElement.
> 
> No need for a review, since this is just a partial revert of my previous
> patch.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b83bfbfa9b60

Indeed. I don't understand how this passed review in the first place.
This was merged to m-c but the bug was never updated:

http://hg.mozilla.org/mozilla-central/rev/806f04d54ab5
http://hg.mozilla.org/mozilla-central/rev/b83bfbfa9b60
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.