472942 - VideoDocument should center video in tab

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Justin Dolske [:Dolske]]

Alex suggested that we should scale a directly-loaded video to fill the tab. I had been thinking the same thing, the current display is a bit lackluster because you usually end up with a tiny video in the corner of a huge white page.

A kludge to do this as a bookmarklet:

  javascript:void(document.body.firstChild.style.width='100%')

Although with certain window sizes you'll get scrollbars.

Comment 1

[Asa Dotzler [:asa]]

we don't fill the tab with an image when you load that up standalone. I think we should do the same thing for video as we do for image -- show it at native resolution.

Comment 2

[Asa Dotzler [:asa]]

and I'll just add, it's only lackluster because it's a tiny video. Even in this basic streaming setup, I could be pushing twice the resolution. There will no doubt be lots of people offering HD (and better?) content to the browser in the near future.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Wouldn't this slow down the movie terrifically?

Comment 4

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #3)
> Wouldn't this slow down the movie terrifically?

I see this only on one of my computers. It can't seem to handle videos larger than 200 px and even these small ones run with irregular speed. The larger videos like this one: https://bugzilla.mozilla.org/attachment.cgi?id=354005 play in slow motion, the sound is unrecognizably deformed.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Bit of a mis-communication, I was referring to the specific case of air.mozilla.com.  In terms of the general case of navigating directly to a video, perhaps center the video similar to quicktime?  It's strange, but for some reason upper left alignment feels right for images, and centered feels right for Video, but I can't actually back that up with any specific or rational reasons.

Comment 6

[Biju]

Cant we give a Zoom control on status bar to take care of this

Comment 7

[cajbir (:cajbir)]

Now that we have full screen support is this functionality really necessary? The user can right click on the video and go fullscreen.

Comment 8

[Asa Dotzler [:asa]]

I'll restate my view that we should treat video like we do images (I'm for centering both on the page when loaded without a document)

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

>I'll restate my view that we should treat video like we do images (I'm for
>centering both on the page when loaded without a document)

Yeah, I agree.

Comment 10

[Justin Dolske [:Dolske]]

Boriss and I talked about this a while ago, and iirc some variation of centering seemed best (and not filling the whole tab, though maybe we could change the scale somewhat). I did some experimentation with adding a boxshadow (since otherwise the controls can seem to float at a random point in the page), but wasn't really sold on it.

Maybe Boriss can play some more with the idea and see what looks good?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 472942

I have added some basic styles to center the video. I will attach a screenshot as well for ui-review.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of centered video

Screenshot of centered video.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP for bug 472942

I don't think we want these styles to appear if the video is the src attribute of an iframe. I'm going to continue working on this to see how I can remove these styles in this case.

I have also removed the focus rect on the video since it adds visual noise but not much else since the user is viewing the video directly already.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jared Wein [:jwein] from comment #13)
> Created attachment 551593 [details] [diff] [review] [diff] [details] [review]
> WIP for bug 472942
> 
> I don't think we want these styles to appear if the video is the src
> attribute of an iframe. I'm going to continue working on this to see how I
> can remove these styles in this case.

This version does avoid adding the styles when the video is the src of an IFRAME. (Good catch!)

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 472942 v2

The previous patch didn't actually remove the focus rectangle as I had thought it did. This patch correctly hides the focus rect.

roc: Nothing logic-wise changed here, just some of the CSS rules. I'm asking for review based upon the belief that it is better to over-review than under-review :)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 551670 [details] [diff] [review]
Patch for bug 472942 v2

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

::: content/html/document/src/VideoDocument.cpp
@@ +144,5 @@
> + 
> +    styleContent->SetTextContent(
> +      NS_LITERAL_STRING("body { background: #444; } ") +
> +      NS_LITERAL_STRING("video { display: block; margin: 1.5em auto 0; box-shadow: 0 0 1em #000; } ") +
> +      NS_LITERAL_STRING("video:focus { outline-width: 0; } "));

Better to write
  styleContent->SetTextContent(NS_LITERAL_STRING(
    "body { background: #444; } "
    "video { display: block; margin: 1.5em auto 0; box-shadow: 0 0 1em #000; } "
    "video:focus { outline-width: 0; } "));

Comment 18

[Stephen Horlander [:shorlander]]

Comment on attachment 551585 [details]
Screenshot of centered video

This looks good. We need to make sure match colors with bug 376997. Also the shadow works for video but won't work for images due to the potential for alpha transparency.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> Comment on attachment 551670 [details] [diff] [review]
> Patch for bug 472942 v2
> 
> Review of attachment 551670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/document/src/VideoDocument.cpp
> @@ +144,5 @@
> > + 
> > +    styleContent->SetTextContent(
> > +      NS_LITERAL_STRING("body { background: #444; } ") +
> > +      NS_LITERAL_STRING("video { display: block; margin: 1.5em auto 0; box-shadow: 0 0 1em #000; } ") +
> > +      NS_LITERAL_STRING("video:focus { outline-width: 0; } "));
> 
> Better to write
>   styleContent->SetTextContent(NS_LITERAL_STRING(
>     "body { background: #444; } "
>     "video { display: block; margin: 1.5em auto 0; box-shadow: 0 0 1em #000;
> } "
>     "video:focus { outline-width: 0; } "));

Based on the guide at https://developer.mozilla.org/En/Mozilla_internal_string_guide#String_Concatenation it doesn't seem like we can switch to using a single NS_LITERAL_STRING macro for the concatenation.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=48f0f4ecd342

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of centered video v2

Based on feedback from limi and faaborg, I have centered the video vertically and added some noise to the background.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 472942 v3

Slight changes to the CSS styles, however I don't think we can change to using one NS_LITERAL_STRING macro due to narrow vs. wide string compile-time differences on some platforms.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 556185 [details] [diff] [review]
Patch for bug 472942 v3

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

::: content/html/document/src/VideoDocument.cpp
@@ +144,5 @@
> +    styleContent->SetTextContent(
> +      NS_LITERAL_STRING("body { background: url(chrome://global/skin/icons/tabprompts-bgtexture.png) #333; height: 100%; width: 100%; margin: 0; padding: 0; } ") +
> +      NS_LITERAL_STRING("video { position: absolute; top: 0; right: 0; bottom: 0; left: 0; margin: auto; box-shadow: 0 0 15px #000; } ") +
> +      NS_LITERAL_STRING("video:focus { outline-width: 0; } "));
> +    body->AppendChildTo(styleContent, PR_FALSE);

This element has to go into the <head>, not the <body>.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Comment on attachment 556185 [details] [diff] [review]
> Patch for bug 472942 v3
> 
> Review of attachment 556185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/document/src/VideoDocument.cpp
> @@ +144,5 @@
> > +    styleContent->SetTextContent(
> > +      NS_LITERAL_STRING("body { background: url(chrome://global/skin/icons/tabprompts-bgtexture.png) #333; height: 100%; width: 100%; margin: 0; padding: 0; } ") +
> > +      NS_LITERAL_STRING("video { position: absolute; top: 0; right: 0; bottom: 0; left: 0; margin: auto; box-shadow: 0 0 15px #000; } ") +
> > +      NS_LITERAL_STRING("video:focus { outline-width: 0; } "));
> > +    body->AppendChildTo(styleContent, PR_FALSE);
> 
> This element has to go into the <head>, not the <body>.

Good catch. I'll switch it to adding the styles to the <head> and reupload.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 472942 v4

Moved the <style> element to the <head>

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

We will hold off on landing this change until bug 376997 lands, at which point the CSS changes can be applied to the CSS file introduced by bug 376997.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/d1a258f48898

Comment 28

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/8e3e1c5f348d

Comment 29

[u279076]

What is the valid testcase for this bug? I'm assuming:

1) Visit getpersonas.com
2) Click PLAY
3) Right click the video and select "View Video"

Result:
Video opens in a new tab and plays centered in content.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #29)
> What is the valid testcase for this bug? I'm assuming:
> 
> 1) Visit getpersonas.com
> 2) Click PLAY
> 3) Right click the video and select "View Video"
> 
> Result:
> Video opens in a new tab and plays centered in content.

Test case for this bug would be to visit this page:
http://videos-cdn.mozilla.net/serv/air_mozilla/07272011_brownbag.ogg

and see that the video is centered horizontally and vertically in the page along with a drop shadow and textured background.

Comment 31

[u279076]

Verified fixed on Firefox 9.0a2 using comment 30.

Comment 32

[M8R-gc72dc]

And the other way around, reducing big videos to fit (like it's done for images), is there a bug for that? I couldn't find one.

Right now the experience for large videos is quite miserable. You can't even reach the controls without scrolling.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to M8R-gc72dc from comment #32)
> And the other way around, reducing big videos to fit (like it's done for
> images), is there a bug for that? I couldn't find one.
> 
> Right now the experience for large videos is quite miserable. You can't even
> reach the controls without scrolling.

That functionality is part of bug 376997.

Comment 34

[Paul Adenot (:padenot)]

This functionality is explicitly bug 537718, but I still have to rebase and address dolske comments in it, as well as maybe modifying the code to be correct with this patch.

Comment 35

[Michal Čaplygin [:myf]]

Just for record, proposal of centering images in "image only page" has been made in bug 252054 (which is RESOLVED WONTFIX at the moment).

I'd like to add another proposal: would it be possible to add certain class to the body element of this page? The view-source page behaves this way: it has body id="viewsource".

There is quite intense need of some error-prone way to address image only pages in the userstyles community, since the change of structure of such pages which added the HEAD element obsoleted all body:only-child techniques. See http://userstyles.org/styles/browse/global/center%20images to get a picture.

What about body id="viewimage" | id="viewvideo" ...?

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Michal Čaplygin [:myf] from comment #35)
> Just for record, proposal of centering images in "image only page" has been
> made in bug 252054 (which is RESOLVED WONTFIX at the moment).
> 
> I'd like to add another proposal: would it be possible to add certain class
> to the body element of this page? The view-source page behaves this way: it
> has body id="viewsource".
> 
> There is quite intense need of some error-prone way to address image only
> pages in the userstyles community, since the change of structure of such
> pages which added the HEAD element obsoleted all body:only-child techniques.
> See http://userstyles.org/styles/browse/global/center%20images to get a
> picture.
> 
> What about body id="viewimage" | id="viewvideo" ...?

Michal: Thank you for your input. Can you please file a new bug for this? Please note that bug 713487 has a goal of moving these new stylesheets to toolkit/themes.