Last Comment Bug 770945 - "Simple events" on HTML5 media shouldn't be cancelable
: "Simple events" on HTML5 media shouldn't be cancelable
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Matthew Gregan [:kinetik]
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks: 782876
  Show dependency treegraph
 
Reported: 2012-07-04 11:14 PDT by Jeremy Banks
Modified: 2012-08-22 22:24 PDT (History)
5 users (show)
kinetik: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v0 (wrong patch) (1.65 KB, patch)
2012-08-13 20:28 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v0 (4.42 KB, patch)
2012-08-13 23:30 PDT, Matthew Gregan [:kinetik]
roc: review+
Details | Diff | Splinter Review
patch v0 partial revert (3.18 KB, patch)
2012-08-16 22:08 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review

Description Jeremy Banks 2012-07-04 11:14:41 PDT
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.)
Comment 1 Matthew Gregan [:kinetik] 2012-08-13 20:28:07 PDT
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.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-13 20:35:37 PDT
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?
Comment 3 Matthew Gregan [:kinetik] 2012-08-13 21:25:29 PDT
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.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-13 23:17:21 PDT
OK. I guess it probably doesn't make any difference to Web content.

You uploaded the wrong patch though :-)
Comment 5 Matthew Gregan [:kinetik] 2012-08-13 23:20:07 PDT
Comment on attachment 651620 [details] [diff] [review]
patch v0 (wrong patch)

Oops.
Comment 6 Matthew Gregan [:kinetik] 2012-08-13 23:30:53 PDT
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.
Comment 7 Matthew Gregan [:kinetik] 2012-08-14 20:51:15 PDT
WebKit bug (also covers Chrom{e,ium}): https://bugs.webkit.org/show_bug.cgi?id=94065
Opera bug: DSK-372112@bugs.opera.com
Comment 8 Matthew Gregan [:kinetik] 2012-08-15 00:36:56 PDT
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).
Comment 9 Matthew Gregan [:kinetik] 2012-08-16 20:38:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/806f04d54ab5
Comment 10 Matthew Gregan [:kinetik] 2012-08-16 22:08:35 PDT
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
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-08-17 06:04:39 PDT
(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.
Comment 12 Matthew Gregan [:kinetik] 2012-08-22 22:24:51 PDT
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

Note You need to log in before you can comment on or make changes to this bug.