Closed
Bug 472942
Opened 16 years ago
Closed 13 years ago
VideoDocument should center video in tab
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
VERIFIED
FIXED
mozilla9
People
(Reporter: Dolske, Assigned: jaws)
References
Details
(Keywords: verified-aurora, Whiteboard: [fixed-in-fx-team][qa!])
Attachments
(2 files, 5 obsolete files)
(deleted),
image/png
|
shorlander
:
ui-review+
|
Details |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•16 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•16 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.
Comment 3•16 years ago
|
||
Wouldn't this slow down the movie terrifically?
Comment 4•16 years ago
|
||
(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•16 years ago
|
||
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.
Cant we give a Zoom control on status bar to take care of this
No longer blocks: TrackAVUI
Comment 7•15 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•15 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)
Comment 9•15 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)
Yeah, I agree.
Reporter | ||
Comment 10•15 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
Assignee | ||
Comment 11•13 years ago
|
||
I have added some basic styles to center the video. I will attach a screenshot as well for ui-review.
Assignee | ||
Comment 12•13 years ago
|
||
Screenshot of centered video.
Attachment #551585 -
Flags: feedback?(shorlander)
Attachment #551583 -
Flags: review?(roc) → review+
Assignee | ||
Comment 13•13 years ago
|
||
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+
Assignee | ||
Comment 15•13 years ago
|
||
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+
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Updated•13 years ago
|
Comment 18•13 years ago
|
||
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+
Assignee | ||
Comment 19•13 years ago
|
||
(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.
Assignee | ||
Comment 20•13 years ago
|
||
Assignee | ||
Comment 21•13 years ago
|
||
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)
Assignee | ||
Comment 22•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
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>.
Assignee | ||
Comment 24•13 years ago
|
||
(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.
Assignee | ||
Comment 25•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #556184 -
Flags: ui-review?(shorlander) → ui-review+
Assignee | ||
Comment 26•13 years ago
|
||
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
Assignee | ||
Comment 27•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 28•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment 29•13 years ago
|
||
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.
Assignee | ||
Comment 30•13 years ago
|
||
(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•13 years ago
|
||
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•13 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.
Assignee | ||
Comment 33•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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" ...?
Assignee | ||
Comment 36•13 years ago
|
||
(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.
Description
•