Closed Bug 713487 Opened 8 years ago Closed 8 years ago

Move TopLevelImageDocument.css and TopLevelVideoDocument.css to toolkit/themes


(Toolkit :: Themes, defect)

Not set





(Reporter: Dolske, Assigned: jaws)




(1 file, 2 obsolete files)

Bug 376997 and 700856 (or thereabouts) added stylesheets for standalone images and videos. These (top-level) pages are essentially a front-end feature now, and the specific styles ought to live in the app's theme.

[The last rule in TopLevelImageDocument.css probably needs to stay in layout, or maybe it should just return (?) to being hard coded. Otherwise, the contents of these files are guided purely by visual design, not functionality.]

Should be simple enough to move these around, though it depends on how pedantic we want to be about about cross-module references. E.G., is it acceptable for and ImageDocument/VideoDocument to blindly reference a CSS file in Toolkit, even if some Gecko-based apps don't include it?
Having a way to be able to style the toplevel*document pages from a theme, whould be perfect. And a reference to a theme file that not all provide is ok, as missing .css files are handled correctly (I remember the days of Mozilla M10 where it did crash on a missing .css file tho).
Attached patch Patch for bug (obsolete) — Splinter Review
I tried to keep as much structural styles within layout/style since not all Gecko apps are required to use Toolkit.
Assignee: nobody → jwein
Attachment #613080 - Flags: review?(roc)
Attachment #613080 - Flags: review?(dolske)
Comment on attachment 613080 [details] [diff] [review]
Patch for bug

>--- a/layout/style/TopLevelImageDocument.css
>+++ b/layout/style/TopLevelImageDocument.css
>@@ -34,17 +34,16 @@
> /*
>   This CSS stylesheet defines the rules to be applied to ImageDocuments that
>   are top level (e.g. not iframes).
> */
> @media not print {
>   body {
>-    background-color: #222;
>     margin: 0;
>   }
>   img {
>     color: #eee;

You forgot to move this last line.
Attachment #613080 - Flags: review?(dolske) → review-
Attached patch Patch for bug v2 (obsolete) — Splinter Review
Moved the font color to the theme files.

I didn't move it to the reftest CSS files since those reftests will fail anyways if the image fails to decode.
Attachment #613080 - Attachment is obsolete: true
Attachment #613134 - Flags: review?(dao)
Comment on attachment 613134 [details] [diff] [review]
Patch for bug v2

>+body {
>+  background: url(...) #333;

make this "background: #333 url(...);"

All of the gnomestripe additions are redundant. You're overriding the winstripe files which have exactly the same content.
Attachment #613134 - Flags: review?(dao) → review-
Attached patch Patch for bug v3Splinter Review
Put background-color before background-image and removed the changes to gnomestripe.
Attachment #613134 - Attachment is obsolete: true
Attachment #613670 - Flags: review?(dao)
Comment on attachment 613670 [details] [diff] [review]
Patch for bug v3

>--- a/toolkit/themes/winstripe/global/
>+++ b/toolkit/themes/winstripe/global/

>+        skin/classic/aero/global/TopLevelImageDocument.css               (TopLevelImageDocument.css)
>+        skin/classic/aero/global/TopLevelVideoDocument.css               (TopLevelVideoDocument.css)

remove (TopLevelImageDocument.css) and (TopLevelVideoDocument.css)
Attachment #613670 - Flags: review?(dao) → review+
Flags: in-testsuite-
Flags: in-litmus-
Target Milestone: --- → mozilla14
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.