VideoDocument should center video in tab

VERIFIED FIXED in mozilla9

Status

()

Core
Audio/Video
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Dolske, Assigned: jaws)

Tracking

({verified-aurora})

Trunk
mozilla9
verified-aurora
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team][qa!])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
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

9 years ago
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.
Wouldn't this slow down the movie terrifically?
(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.
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.

Updated

8 years ago
Blocks: 470629

Comment 6

8 years ago
Cant we give a Zoom control on status bar to take care of this
No longer blocks: 470629

Comment 7

8 years ago
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

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

Comment 10

8 years ago
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?
Summary: nsVideoDocument should scale video to fill tab → nsVideoDocument should center video in tab
Blocks: 502122
Created attachment 551583 [details] [diff] [review]
Patch for bug 472942

I have added some basic styles to center the video. I will attach a screenshot as well for ui-review.
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #551583 - Flags: review?(roc)
Created attachment 551585 [details]
Screenshot of centered video

Screenshot of centered video.
Attachment #551585 - Flags: feedback?(shorlander)
Attachment #551583 - Flags: review?(roc) → review+
Created attachment 551593 [details] [diff] [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.

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.
Attachment #551583 - Attachment is obsolete: true
Attachment #551593 - Flags: feedback?(roc)
(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!)
Attachment #551593 - Flags: feedback?(roc) → feedback+
Created attachment 551670 [details] [diff] [review]
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 :)
Attachment #551593 - Attachment is obsolete: true
Attachment #551670 - Flags: review?(roc)
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; } "));
Attachment #551670 - Flags: review?(roc) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 376997
Status: RESOLVED → REOPENED
Depends on: 376997
Resolution: DUPLICATE → ---
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.
Attachment #551585 - Flags: feedback?(shorlander) → feedback+
(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.
Pushed to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=48f0f4ecd342
Created attachment 556184 [details]
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.
Attachment #551585 - Attachment is obsolete: true
Attachment #556184 - Flags: feedback?(shorlander)
Created attachment 556185 [details] [diff] [review]
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.
Attachment #551670 - Attachment is obsolete: true
Attachment #556185 - Flags: review?(roc)
Attachment #556184 - Flags: feedback?(shorlander) → ui-review?(shorlander)
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>.
(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.
Created attachment 556229 [details] [diff] [review]
Patch for bug 472942 v4

Moved the <style> element to the <head>
Attachment #556185 - Attachment is obsolete: true
Attachment #556229 - Flags: review?(roc)
Attachment #556185 - Flags: review?(roc)
Attachment #556229 - Flags: review?(roc) → review+
No longer depends on: 376997
See Also: → bug 376997
Attachment #556184 - Flags: ui-review?(shorlander) → ui-review+
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.
Status: REOPENED → ASSIGNED
Depends on: 376997
Summary: nsVideoDocument should center video in tab → VideoDocument should center video in tab
https://hg.mozilla.org/integration/fx-team/rev/d1a258f48898
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/8e3e1c5f348d
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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.
(Reporter)

Updated

6 years ago
Depends on: 689106
(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.
Verified fixed on Firefox 9.0a2 using comment 30.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][qa!]

Comment 32

6 years ago
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.
(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.
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.
Depends on: 693958
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" ...?
(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.
You need to log in before you can comment on or make changes to this bug.