resize event for video

RESOLVED FIXED in mozilla38

Status

()

Core
DOM: Events
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Arkadiusz Michalski (Spirit), Assigned: pehrsons)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

2.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.87 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
2.63 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
7.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Upgrade: Chrome 34 support this event for video element.
Assignee: nobody → pehrsons
Status: UNCONFIRMED → ASSIGNED
Depends on: 879717
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 31 Branch → unspecified
See Also: → bug 950304

Comment 2

3 years ago
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.
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.
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.
Flags: needinfo?(bugs)

Comment 5

3 years ago
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.
Flags: needinfo?(bugs)
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!
Flags: needinfo?(bugs)
So, I was just using the wrong macro =/

Patches and trypush on their way now.
Flags: needinfo?(bugs)
Created attachment 8531976 [details] [diff] [review]
Part 1. Make onresize event handler global and forwarded
Attachment #8527405 - Attachment is obsolete: true
Attachment #8528248 - Attachment is obsolete: true
Attachment #8531976 - Flags: review?(bugs)
Created attachment 8531979 [details] [diff] [review]
Part 2. Fire resize event when video size changes
Attachment #8531979 - Flags: review?(roc)
Created attachment 8531980 [details] [diff] [review]
Part 3. Test video element resize event
Attachment #8531980 - Flags: review?(roc)
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=92b137537999
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 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.
Attachment #8531976 - Flags: review?(bugs) → review+
Created attachment 8532323 [details] [diff] [review]
Part 1. Make onresize event handler global and forwarded (carries r=smaug)

Fix per comment 13.
Attachment #8531976 - Attachment is obsolete: true
Attachment #8532323 - Flags: review+
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'.
Attachment #8531980 - Flags: review?(roc) → review+
Attachment #8531979 - Flags: review?(roc) → review+
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.
Attachment #8531980 - Attachment is obsolete: true
Attachment #8533009 - Flags: review+
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
Attachment #8533009 - Attachment is obsolete: true
Attachment #8537181 - Flags: review+
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
Keywords: checkin-needed
Fixing bug 879717 first.
Keywords: checkin-needed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b65fb4dcf624
Keywords: checkin-needed
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!
Flags: needinfo?(pehrsons)
Keywords: checkin-needed
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.
Attachment #8537181 - Attachment is obsolete: true
Flags: needinfo?(pehrsons)
Attachment #8551654 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/142031795f33
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d8cf5b47ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aeda1b1d2e4
Keywords: checkin-needed
sorry Andreas, had to back this out for test failures like :
https://treeherder.mozilla.org/logviewer.html#?job_id=5651936&repo=mozilla-inbound
and https://treeherder.mozilla.org/logviewer.html#?job_id=5651933&repo=mozilla-inbound
Flags: needinfo?(pehrsons)
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
Flags: needinfo?(pehrsons)
Attachment #8552238 - Flags: review?(bugs)
Attachment #8552238 - Flags: review?(bugs)
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.
Attachment #8552238 - Attachment is obsolete: true
Attachment #8552245 - Flags: review?(bugs)
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb3bb4d38566

Updated

3 years ago
Attachment #8552245 - Flags: review?(bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c112fdf29ed1
https://hg.mozilla.org/integration/mozilla-inbound/rev/afa74310b970
https://hg.mozilla.org/integration/mozilla-inbound/rev/e714a95cf02c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf45d24e801a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c112fdf29ed1
https://hg.mozilla.org/mozilla-central/rev/afa74310b970
https://hg.mozilla.org/mozilla-central/rev/e714a95cf02c
https://hg.mozilla.org/mozilla-central/rev/cf45d24e801a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1126851
You need to log in before you can comment on or make changes to this bug.