Closed Bug 992685 Opened 11 years ago Closed 10 years ago

resize event for video

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: crimsteam, Assigned: pehrsons)

References

Details

Attachments

(4 files, 7 obsolete files)

Part 2. Fire resize event when video size changes
2.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 1. Make onresize event handler global and forwarded (carries r=smaug)
3.87 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
Part 3. Test video element resize event (carries r=roc)
2.63 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
Part 4. Fix wpt expectancies wrt onresize being a global event handler
7.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
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: → 950304
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.
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)
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)
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)
Attachment #8527405 - Attachment is obsolete: true
Attachment #8528248 - Attachment is obsolete: true
Attachment #8531976 - Flags: review?(bugs)
Attachment #8531980 - Flags: review?(roc)
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+
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+
Also tests that 'durationchange' comes before 'resize' per comment 15, carrying r=roc.
Attachment #8531980 - Attachment is obsolete: true
Attachment #8533009 - Flags: review+
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
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
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
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)
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)
Attachment #8552245 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Depends on: 1126851
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: