Last Comment Bug 745067 - Page Info Media preview fullscreen videos shouldn't have a margin
: Page Info Media preview fullscreen videos shouldn't have a margin
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 11 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: 545812
  Show dependency treegraph
 
Reported: 2012-04-12 19:52 PDT by Chris Pearce (:cpearce)
Modified: 2012-05-03 00:30 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot; show video fullscreen, but with transparent margin (255.58 KB, image/jpeg)
2012-04-12 19:52 PDT, Chris Pearce (:cpearce)
no flags Details
Testcase: Simpler STR (651 bytes, text/html)
2012-04-17 16:21 PDT, Chris Pearce (:cpearce)
no flags Details
Patch: use padding in the container rather than margin on the containee (1.73 KB, patch)
2012-04-17 19:03 PDT, Chris Pearce (:cpearce)
dao+bmo: review-
Details | Diff | Splinter Review
Patch: remove magins on fullscreen elements (691 bytes, patch)
2012-04-23 16:43 PDT, Chris Pearce (:cpearce)
bzbarsky: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2012-04-12 19:52:01 PDT
Created attachment 614663 [details]
Screenshot; show video fullscreen, but with transparent margin

STR:
1. Load http://pearce.org.nz/full-screen
2. Right click > View Page Info
3. In the "Media Preview" box showing the quake video, press the fullscreen button on the video control.
4. Video will go fullscreen, but there will still be a transparent margin around the media showing the non-fullscreen content beneath.

Expected result:
Video should fill entire screen, there should not be a transparent margin.

I've seen this before in a few cases, and I think it's something to do with the containing frames.
Comment 1 Chris Pearce (:cpearce) 2012-04-17 16:21:43 PDT
Created attachment 615935 [details]
Testcase: Simpler STR

A simpler testcase demonstrating the problem. Containing frames aren't the issue, it looks like we're not accounting for margin when we're doing the width calculation?
Comment 2 Chris Pearce (:cpearce) 2012-04-17 18:58:06 PDT
Scratch that, my understanding of the CSS box model was incorrect. The videos in the media preview have a margin defined, and this is actually correct CSS margin behaviour.

However it looks silly to have a margin on Page Info media preview videos which are made fullscreen. I have a patch...
Comment 3 Chris Pearce (:cpearce) 2012-04-17 19:03:21 PDT
Created attachment 615985 [details] [diff] [review]
Patch: use padding in the container rather than margin on the containee

Use padding in the PageInfo's media preview container, rather than giving the contained media a margin. This means the fullscreen styles will work correctly when media preview videos request fullscreen, and a margin won't be drawn around them.
Comment 4 Chris Pearce (:cpearce) 2012-04-22 19:13:02 PDT
Comment on attachment 615985 [details] [diff] [review]
Patch: use padding in the container rather than margin on the containee

Gavin, maybe you can review this instead?
Comment 5 Dão Gottwald [:dao] 2012-04-22 22:10:35 PDT
Comment on attachment 615985 [details] [diff] [review]
Patch: use padding in the container rather than margin on the containee

This is something that could happen to any video element on the web (i.e. as soon as the author adds a margin). Maybe this is expected behavior for full-screen API users on the web, but at least for the built-in full-screen button I think we should prevent this at a lower level.
Comment 6 Chris Pearce (:cpearce) 2012-04-23 16:43:21 PDT
Created attachment 617697 [details] [diff] [review]
Patch: remove magins on fullscreen elements

I guess we should remove margins on fullscreen elements, as without this we leave a way to expose the content underneath the fullscreen element.

Once we implement ::backdrop [1], we won't need to remove the margins to obscure the content underneath the fullscreen element.

[1] http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html#::backdrop-pseudo-element
Comment 7 Boris Zbarsky [:bz] 2012-04-23 19:39:33 PDT
Comment on attachment 617697 [details] [diff] [review]
Patch: remove magins on fullscreen elements

r=me
Comment 9 Ed Morley [:emorley] 2012-04-26 10:35:24 PDT
https://hg.mozilla.org/mozilla-central/rev/7321b7a0c775
Comment 10 Paul Silaghi, QA [:pauly] 2012-05-03 00:30:55 PDT
Verified fixed on FF 15 (2012-05-02):
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/15.0 Firefox/15.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/15.0 Firefox/15.0a1

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