745067 - Page Info Media preview fullscreen videos shouldn't have a margin

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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 (Not reading bugmail)]]

Attachment: 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 (Not reading bugmail)]]

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 (Not reading bugmail)]]

Attachment: 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 (Not reading bugmail)]]

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]]

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 (Not reading bugmail)]]

Attachment: 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 [:bzbarsky]]

Comment on attachment 617697 [details] [diff] [review]
Patch: remove magins on fullscreen elements

r=me

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7321b7a0c775

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7321b7a0c775

Comment 10

[Paul Silaghi, QA [:pauly]]

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