Last Comment Bug 992685 - resize event for video
: resize event for video
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: All All
-- normal with 3 votes (vote)
: mozilla38
Assigned To: Andreas Pehrson [:pehrsons] (Telenor)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 879717 1126851
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-06 15:30 PDT by Arkadiusz Michalski (Spirit)
Modified: 2015-02-03 19:42 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fire resize event when video size changes (5.51 KB, patch)
2014-11-23 18:29 PST, Andreas Pehrson [:pehrsons] (Telenor)
no flags Details | Diff | Splinter Review
Make resize event global and break mochitest layout/base/tests/test_bug114649.html (3.72 KB, patch)
2014-11-25 01:37 PST, Andreas Pehrson [:pehrsons] (Telenor)
no flags Details | Diff | Splinter Review
Part 1. Make onresize event handler global and forwarded (3.90 KB, patch)
2014-12-04 02:00 PST, Andreas Pehrson [:pehrsons] (Telenor)
bugs: review+
Details | Diff | Splinter Review
Part 2. Fire resize event when video size changes (2.05 KB, patch)
2014-12-04 02:01 PST, Andreas Pehrson [:pehrsons] (Telenor)
roc: review+
Details | Diff | Splinter Review
Part 3. Test video element resize event (2.25 KB, patch)
2014-12-04 02:02 PST, Andreas Pehrson [:pehrsons] (Telenor)
roc: review+
Details | Diff | Splinter Review
Part 1. Make onresize event handler global and forwarded (carries r=smaug) (3.87 KB, patch)
2014-12-04 17:26 PST, Andreas Pehrson [:pehrsons] (Telenor)
pehrson: review+
Details | Diff | Splinter Review
Part 3. Test video element resize event (carries r=roc) (2.62 KB, patch)
2014-12-07 20:13 PST, Andreas Pehrson [:pehrsons] (Telenor)
pehrson: review+
Details | Diff | Splinter Review
Part 3. Test video element resize event (carries r=roc) (2.67 KB, patch)
2014-12-16 05:37 PST, Andreas Pehrson [:pehrsons] (Telenor)
pehrson: review+
Details | Diff | Splinter Review
Part 3. Test video element resize event (carries r=roc) (2.63 KB, patch)
2015-01-19 23:46 PST, Andreas Pehrson [:pehrsons] (Telenor)
pehrson: review+
Details | Diff | Splinter Review
Part 4. Fix wpt expectancies wrt onresize being a global event handler (5.90 KB, patch)
2015-01-20 21:48 PST, Andreas Pehrson [:pehrsons] (Telenor)
no flags Details | Diff | Splinter Review
Part 4. Fix wpt expectancies wrt onresize being a global event handler (7.26 KB, patch)
2015-01-20 22:07 PST, Andreas Pehrson [:pehrsons] (Telenor)
bugs: review+
Details | Diff | Splinter Review

Description User image Arkadiusz Michalski (Spirit) 2014-04-06 15:30:51 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

HTML5 defines two case when this event should be firing for video element:

- when video has been fetched (http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#getting-media-metadata) << step 5

- whenever the intrinsic width or intrinsic height of the video changes (http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#dimUpdate), for example, because the selected video track was changed.

At now this event is fired only for window (when resize). I see someon working on the implementation of videoTracks (https://bugzilla.mozilla.org/show_bug.cgi?id=744896) so maybe resize event should be implemented too.
Comment 1 User image Arkadiusz Michalski (Spirit) 2014-04-16 07:35:42 PDT
Upgrade: Chrome 34 support this event for video element.
Comment 2 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2014-11-20 03:43:47 PST
Since resize does not bubble, this should be somewhat safe (scripts listening for the resize on Window don't usually deal with this event). And given the Chrome has this too, should be ok to implement.
Comment 3 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-11-20 17:59:04 PST
Great! I'll have a patch ready fairly soon I think. Though there are quite some changes to when loadedmetadata is fired in bug 879717 so we'll have to wait for that one before landing anyhow.
Comment 4 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-11-23 18:29:15 PST
Created attachment 8527405 [details] [diff] [review]
Fire resize event when video size changes

My event-fu is a bit.. limited. This seemed like a reasonable patch but it causes some errors. See https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fd20035806bd (M-4/5 and M-oth).

I'm guessing some sort of conflict between WINDOW_EVENT and EVENT, but don't know enough to resolve it. smaug, any assisting you can provide would be very appreciated.
Comment 5 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2014-11-24 03:55:34 PST
Hmm, why EventNameType_HTMLBodyOrFramesetOnly. 

But I don't really know about those test failures. Are we handling resize event somewhere?
Or are those tests expecting certain stuff to be in the event loop so that they fail when something 
is added there (DispatchAsyncEvent seems to be some MediaElement related Runnable).
You might want to try to dispatch some random namely event. Does such break the tests too?

I assume you can reproduce at least some of those failures locally.
Comment 6 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-11-25 01:37:02 PST
Created attachment 8528248 [details] [diff] [review]
Make resize event global and break mochitest layout/base/tests/test_bug114649.html

The test failure is only related to making the resize event global and changing the macro. This is a patch for 100% repro rate. Just apply it on top of m-c.

I don't know enough about events to dig into this without spending too much time on it I'm afraid. If you have an idea, or if you know who might, I'd appreciate it!
Comment 7 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-04 01:55:19 PST
So, I was just using the wrong macro =/

Patches and trypush on their way now.
Comment 8 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-04 02:00:25 PST
Created attachment 8531976 [details] [diff] [review]
Part 1. Make onresize event handler global and forwarded
Comment 9 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-04 02:01:33 PST
Created attachment 8531979 [details] [diff] [review]
Part 2. Fire resize event when video size changes
Comment 10 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-04 02:02:05 PST
Created attachment 8531980 [details] [diff] [review]
Part 3. Test video element resize event
Comment 11 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-04 02:04:30 PST
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92b137537999
Comment 12 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-04 08:36:16 PST
I think the failures there are due to a patch for another bug that was actually untested. Here's a new try without it. Also just rebased on m-c.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2a662ccf03fc
Comment 13 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2014-12-04 11:35:16 PST
Comment on attachment 8531976 [details] [diff] [review]
Part 1. Make onresize event handler global and forwarded

> FORWARDED_EVENT(load,
>                 NS_LOAD,
>                 EventNameType_All,
>                 eBasicEventClass)
>+FORWARDED_EVENT(resize,
>+                NS_RESIZE_EVENT,
>+                (EventNameType_HTMLXUL | EventNameType_SVGSVG),
I think we should use EventNameType_All here, since GlobalEventHandlers anyway adds the onfoo property to all the
svg elements.
Comment 14 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-04 17:26:58 PST
Created attachment 8532323 [details] [diff] [review]
Part 1. Make onresize event handler global and forwarded (carries r=smaug)

Fix per comment 13.
Comment 15 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2014-12-07 18:09:27 PST
Comment on attachment 8531980 [details] [diff] [review]
Part 3. Test video element resize event

Review of attachment 8531980 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that. Thanks!!!

::: dom/media/test/test_video_dimensions.html
@@ +23,5 @@
> +    var v = ev.target;
> +    ok(!v.resize, v.testName + " should only fire resize once for same size");
> +    v.resize = true;
> +    is(v.videoWidth, test.width, v.testName + " width should be set on resize");
> +    is(v.videoHeight, test.height, v.testName + " height should be set on resize");

Please also test that 'resize' is fired after 'durationchange'.
Comment 16 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-07 20:13:01 PST
Created attachment 8533009 [details] [diff] [review]
Part 3. Test video element resize event (carries r=roc)

Also tests that 'durationchange' comes before 'resize' per comment 15, carrying r=roc.
Comment 17 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-16 05:37:20 PST
Created attachment 8537181 [details] [diff] [review]
Part 3. Test video element resize event (carries r=roc)

Trivial change to fix the failures on B2G here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d8ee1e060d56

We were checking that "durationchange" was only fired once but it may be fired multiple times if the media element's duration changes - i.e., if it's playing a stream and finishes.

Also adds some logging for easier debugging. Carries forward r=roc.


https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=94f7b7681fac
Comment 18 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-19 06:22:17 PST
Tests are not all done yet but they seem to be doing well. Please check in when they have finished.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34bd384d307a
Comment 19 User image Andreas Pehrson [:pehrsons] (Telenor) 2014-12-19 12:15:46 PST
Fixing bug 879717 first.
Comment 20 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-01-19 01:53:17 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b65fb4dcf624
Comment 21 User image Carsten Book [:Tomcat] 2015-01-19 23:40:16 PST
Hi,

part 3 failed to apply: 

adding 992685 to series file
renamed 992685 -> 0003-Bug-992685-Part-3.-Test-video-element-resize-event.-.patch
applying 0003-Bug-992685-Part-3.-Test-video-element-resize-event.-.patch
patching file dom/media/test/test_video_dimensions.html
Hunk #1 FAILED at 13
1 out of 1 hunks FAILED -- saving rejects to file dom/media/test/test_video_dimensions.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

could you take a look thanks!
Comment 22 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-01-19 23:46:50 PST
Created attachment 8551654 [details] [diff] [review]
Part 3. Test video element resize event (carries r=roc)

Sure thing. This was just rebased, carrying r=roc.
Comment 25 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-01-20 21:48:26 PST
Created attachment 8552238 [details] [diff] [review]
Part 4. Fix wpt expectancies wrt onresize being a global event handler

Based on the fact that the other similar global event handlers (onblur, onfocus, onload, etc. See https://html.spec.whatwg.org/multipage/webappapis.html#handler-onresize) don't have these exceptions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=621314f13b4e
Comment 26 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-01-20 22:07:05 PST
Created attachment 8552245 [details] [diff] [review]
Part 4. Fix wpt expectancies wrt onresize being a global event handler

Sorry, that was a bit fast. Now also fixing the two UNEXPECTED-PASS media-source tests.
Comment 27 User image Andreas Pehrson [:pehrsons] (Telenor) 2015-01-20 22:09:27 PST
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb3bb4d38566

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