493022 - Add mozapps/ to the modern theme

Status: RESOLVED FIXED

(SeaMonkey :: Themes, enhancement)

Description

[Giacomo Magnini]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; it; rv:1.9.0.10) Gecko/2009042700 SUSE/3.0.10-1.1.1 Firefox/3.0.10
Build Identifier: 

Using the work done by Kuden, update the modern theme with the mozapps dir.

Reproducible: Always

Comment 1

[Giacomo Magnini]

Attachment: The whole package

First version of the patch:
* Added 2 icons to global/icons/because they are referenced by some css
* renamed downloadButtons.png -> buttons.png (and relative css files) to match *stripe
* left downloads in, even if it's not used yet
* added boilerplate to all css files, citing Kuden

Comment 2

[[e-mail]]

Comment on attachment 377449 [details] [diff] [review]
The whole package

>diff --git a/suite/themes/modern/global/icons/Error.png b/suite/themes/modern/global/icons/Error.png
I guess alert-error.gif is the wrong size for this.

>diff --git a/suite/themes/modern/global/icons/loading_16.png b/suite/themes/modern/global/icons/loading_16.png
chrome://communicator/skin/icons/loading.gif

Comment 3

[[e-mail]]

Comment on attachment 377449 [details] [diff] [review]
The whole package

>+  skin/modern/mozapps/downloads/buttons.png                        (mozapps/downloads/buttons.png)
>+  skin/modern/mozapps/downloads/downloadIcon.png                   (mozapps/downloads/downloadIcon.png)
>+  skin/modern/mozapps/downloads/downloads.css                      (mozapps/downloads/downloads.css)
>+  skin/modern/mozapps/downloads/unknownContentType.css             (mozapps/downloads/unknownContentType.css)
downloads will be hard to test, but not impossible...

>+  skin/modern/mozapps/passwordmgr/key.png                          (mozapps/passwordmgr/key.png)
We don't use the mozapps passwordmgr...

>+  skin/modern/mozapps/places/defaultFavicon.png                    (mozapps/places/defaultFavicon.png)
>+  skin/modern/mozapps/places/tagContainerIcon.png                  (mozapps/places/tagContainerIcon.png)
Or the mozapps places...

>+  skin/modern/mozapps/profile/profileicon.png                      (mozapps/profile/profileicon.png)
>+  skin/modern/mozapps/profile/profileSelection.css                 (mozapps/profile/profileSelection.css)
Or the mozapps profile selection...

Comment 4

[[e-mail]]

Comment on attachment 377449 [details] [diff] [review]
The whole package

I've been looking at the icons. I noticed a) update.png is a copy of the left half of extensionIcons.png b) you have two copies of buttons.png

The image editor that creates those images strangely uses white background for areas with 100% alpha transparency except for the outer border which is black...

Comment 5

[[e-mail]]

Comment on attachment 377449 [details] [diff] [review]
The whole package

As well as the comments previously mentioned, these changes seem to depend on some changes to global/richlistbox.css yet to be posted.

>+  border-top-width: 0;
>+  border-right-width: 0;
>+  border-bottom: 2px solid;
>+  border-left-width: 0;
>+  -moz-border-bottom-colors: #E0ECF6 #C7D0D9;
Instead of border-XXX-width: 0; I prefer border-XXX: none;
Assuming the richlistbox CSS changes are what I think they are, you won't need the bottom colours, but I can't tell without seeing them, not that it matters, since nobody will be seeing this file anyway.

>+richlistitem[type="download"][selected="true"] {
>+  background-image: url("chrome://mozapps/skin/extensions/itemEnabledFader.png");
Download manager referring to a file in extensions? Any shared images belong in a shared location, e.g. mozapps/icons

>+  border: 1px solid;
>+  -moz-border-top-colors: rgb(110, 115, 120);
>+  -moz-border-right-colors: rgb(238, 240, 243);
>+  -moz-border-bottom-colors: rgb(238, 240, 243);
>+  -moz-border-left-colors: rgb(110, 115, 120);
Single colour borders should simply be specified using
border-XXX: 1px solid #XXXXXX;
I saw the rgb() used somewhere else by mistake too.

>+#extensionsBox {
>+  margin: 10px 10px 0px 10px;
I thought this a little large: try 7px 5px 0px;

>+#resizerBox {
>+  margin-top: -12px;
>+  visibility: hidden;
Should be -15px to match the resizer (we also don't need either visibility.)

>+#commandBarBottom {
>+  margin: 10px 10px 5px 10px;
Try 7px 5px 5px;

>+  padding-top: 6px;
>+  padding-bottom: 6px;
>+  -moz-padding-start: 7px;
>+  -moz-padding-end: 7px;
padding: 6px 7px;

>+.updateBadge {
>+  -moz-margin-end: -2px;
>+  -moz-image-region: rect(0px 16px 16px 0px);
>+}
>+
>+.notifyBadge {
>+  -moz-margin-start: -2px;
Put this margin in the shared style rule instead.

>+  -moz-image-region: rect(0px 48px 16px 32px);
Why such a gap between the images?

>+/* Selected Add-on buttons
>+   See content/extensions.css to hide / display buttons */
I don't see the point of this margin tweaking. Just remove it please.

>+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
I might have mentioned that this is wrong. There are places where the wrong throbber is used too.

Comment 7

[Philip Chee]

Attachment: Patch v1.0

First the unfixed nits:

> I've been looking at the icons. I noticed a) update.png is a copy of the left
> half of extensionIcons.png
Not sure what to do here.

> The image editor that creates those images strangely uses white background for
> areas with 100% alpha transparency except for the outer border which is
> black...
Sorry. I don't know how to fix this.

> >+.updateBadge {
> >+  -moz-margin-end: -2px;
> >+  -moz-image-region: rect(0px 16px 16px 0px);
> >+}
> >+
> >+.notifyBadge {
> >+  -moz-margin-start: -2px;
> Put this margin in the shared style rule instead.
I don't understand why?

##################################################################

> >diff --git a/suite/themes/modern/global/icons/Error.png b/suite/themes/modern/global/icons/Error.png
> I guess alert-error.gif is the wrong size for this.
Also alert-error.gif doesn't have a transparent background.

> >diff --git a/suite/themes/modern/global/icons/loading_16.png b/suite/themes/modern/global/icons/loading_16.png
> chrome://communicator/skin/icons/loading.gif
Removed and Fixed all references to loading_16.png

> downloads will be hard to test, but not impossible...
Setting browser.download.manager.useToolkitUI to true seems to work.

> We don't use the mozapps passwordmgr...
Fixed.

> Or the mozapps places...
Fixed.

> Or the mozapps profile selection...
Fixed.

> I've been looking at the icons. I noticed a) update.png is a copy of the left
> half of extensionIcons.png
Not sure what to do here.

> b) you have two copies of buttons.png
Fixed. Moved to mozapps/icons/

> The image editor that creates those images strangely uses white background for
> areas with 100% alpha transparency except for the outer border which is
> black...
Sorry. I don't know how to fix this.

> (From update of attachment 377449 [details] [diff] [review])
> As well as the comments previously mentioned, these changes seem to depend on
> some changes to global/richlistbox.css yet to be posted.
Richlistbox.css updated in the modern->global bug.

> >+  border-top-width: 0;
> >+  border-right-width: 0;
> >+  border-bottom: 2px solid;
> >+  border-left-width: 0;
> >+  -moz-border-bottom-colors: #E0ECF6 #C7D0D9;
> Instead of border-XXX-width: 0; I prefer border-XXX: none;
Fixed.

> Assuming the richlistbox CSS changes are what I think they are, you won't need
> the bottom colours, but I can't tell without seeing them, not that it matters,
Fixed.

> since nobody will be seeing this file anyway.
??

> >+richlistitem[type="download"][selected="true"] {
> >+  background-image: url("chrome://mozapps/skin/extensions/itemEnabledFader.png");
> Download manager referring to a file in extensions? Any shared images belong in
> a shared location, e.g. mozapps/icons
Fixed.

> >+  border: 1px solid;
> >+  -moz-border-top-colors: rgb(110, 115, 120);
> >+  -moz-border-right-colors: rgb(238, 240, 243);
> >+  -moz-border-bottom-colors: rgb(238, 240, 243);
> >+  -moz-border-left-colors: rgb(110, 115, 120);
> Single colour borders should simply be specified using
> border-XXX: 1px solid #XXXXXX;
Fixed.

> I saw the rgb() used somewhere else by mistake too.
Fixed globally.

> >+#extensionsBox {
> >+  margin: 10px 10px 0px 10px;
> I thought this a little large: try 7px 5px 0px;
Fixed.

> >+#resizerBox {
> >+  margin-top: -12px;
> >+  visibility: hidden;
> Should be -15px to match the resizer (we also don't need either visibility.)
Fixed.

> >+#commandBarBottom {
> >+  margin: 10px 10px 5px 10px;
> Try 7px 5px 5px;
Fixed.

> >+  padding-top: 6px;
> >+  padding-bottom: 6px;
> >+  -moz-padding-start: 7px;
> >+  -moz-padding-end: 7px;
> padding: 6px 7px;
Fixed.

> >+.updateBadge {
> >+  -moz-margin-end: -2px;
> >+  -moz-image-region: rect(0px 16px 16px 0px);
> >+}
> >+
> >+.notifyBadge {
> >+  -moz-margin-start: -2px;
> Put this margin in the shared style rule instead.
Why?

> >+  -moz-image-region: rect(0px 48px 16px 32px);
> Why such a gap between the images?
It appears to have been copied from winstripe. The pinstripe version as four icons. Perhaps the original winstripe image also had four?

> >+/* Selected Add-on buttons
> >+   See content/extensions.css to hide / display buttons */
> I don't see the point of this margin tweaking. Just remove it please.
Fixed.

> >+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
> I might have mentioned that this is wrong. There are places where the wrong
> throbber is used too.
Fixed globally.

Comment 8

[[e-mail]]

(In reply to comment #7)
> > >+.updateBadge {
> > >+  -moz-margin-end: -2px;
> > >+  -moz-image-region: rect(0px 16px 16px 0px);
> > >+}
> > >+
> > >+.notifyBadge {
> > >+  -moz-margin-start: -2px;
> > Put this margin in the shared style rule instead.
> I don't understand why?
Why write the same line twice when you only have to do it once?

Comment 9

[[e-mail]]

(In reply to comment #7)
> > I've been looking at the icons. I noticed a) update.png is a copy of the left
> > half of extensionIcons.png
> Not sure what to do here.
Bah, I should never have looked at this. xpinstall/xpinstallItemGeneric.png and extensions/themeGeneric.png are 32x32 icons; extensions/extensionIcons.png has the same two icons at 16x16, and update.png has a copy too, and at least three of these have to be separate anyway...

Comment 10

[Philip Chee]

So what would you suggest? Making them all individual icons and putting them in mozapps/icons/?

Comment 11

[[e-mail]]

Attachment: Optimised icons

Notes:
notifyBadges.png split up into notifyBadge.png and updateBadge.png
ratings.png simplified, will need new image regions
buttons.png should be removed and suite download manager icons used instead
update.png and item*Fader.png were already optimised so not included here
plugin* are all copies so this zip only includes one at each size

Comment 12

[[e-mail]]

+.addonRating[rating] {
+  display: -moz-box;
+  width: 60px;
+  height: 12px;
+  list-style-image: url("chrome://mozapps/skin/extensions/ratings.png");
+}
+
+.addonRating[rating="0"] {
+  -moz-image-region: rect(0px 60px 12px 0px);
+}
+
+.addonRating[rating="1"] {
+  -moz-image-region: rect(0px 72px 12px 12px);
+}
+
+.addonRating[rating="2"] {
+  -moz-image-region: rect(0px 84px 12px 24px);
+}
+
+.addonRating[rating="3"] {
+  -moz-image-region: rect(0px 96px 12px 36px);
+}
+
+.addonRating[rating="4"] {
+  -moz-image-region: rect(0px 108px 12px 48px);
+}
+
+.addonRating[rating="5"] {
+  -moz-image-region: rect(0px 120px 12px 60px);
+}

Comment 13

[[e-mail]]

(In reply to comment #10)
> So what would you suggest? Making them all individual icons and putting them in
> mozapps/icons/?
Sadly we can't put them in mozapps/icons/ because their locations are hardcoded in various source files :-(

Comment 14

[[e-mail]]

Comment on attachment 395770 [details] [diff] [review]
Patch v1.0

>+  border-bottom: 2px solid;
Don't need this, it's already 2px solid.

>+  border: 1px solid !important;
Make this 1px dotted here since it's transparent anyway.

>+.mini-button:focus {
>+  border-style: dotted !important;
Thus avoiding having to change it here.

>+#clearListButton {
>+  margin-top: 5px;
>+  margin-bottom: 5px;
>+  -moz-margin-start: 5px;
>+}
Looks fine to me without these margins but you could try
margin: 2px 4px; or margin: 4px;

>+.small-indent {
>+  -moz-margin-start: 15px; 
>+  -moz-margin-end: 15px;
Use margin-left and margin-right when the values are the same.

>+#extensionDescription {
>+  color: #324055;
Strange choice of colour...

>+  list-style-image: url("chrome://communicator/skin/icons/loading.gif");
>+  -moz-image-region: auto;
I guess we won't need that now that my "new" badge image needs no region.

>+.throbber {
>+  list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif");
Oops

>+  margin-top: 1px;
>+  margin-bottom: 4px;
>+  -moz-margin-start: 5px;
>+  -moz-margin-end: 5px;
margin: 1px 5px 4px;

>+#moreDetails {
>+  margin: 1px 5px 4px 3px;
Oops, not RTL-safe ;-)

>+  margin: 1px 5px 4px 5px;
Can write 1px 5px 4px;

>+#downloadThrobber {
>+  margin-left: 5px;
-moz-margin-start?

>+#downloadStatusProgress {
>+  padding-right: 5px;
-moz-padding-start?

>+#verificationFailedIcon {
>+  margin-left: 5px;
margin-start?

>+  margin: 1px 5px 4px 5px;
1px 5px 4px;

>+  margin-left: 27px;
margin-start?

Comment 15

[Philip Chee]

Attachment: Patch v1.1

> Notes:
> notifyBadges.png split up into notifyBadge.png and updateBadge.png
Noted. CSS adjusted.

> ratings.png simplified, will need new image regions
Fixed.

> buttons.png should be removed and suite download manager icons used instead
Fixed.

> update.png and item*Fader.png were already optimised so not included here
> plugin* are all copies so this zip only includes one at each size
OK.

> +.addonRating[rating] {
> +.addonRating[rating="0"] {
> +.addonRating[rating="1"] {
> +.addonRating[rating="2"] {
> +.addonRating[rating="3"] {
> +.addonRating[rating="4"] {
> +.addonRating[rating="5"] {
Fixed.

> >+  border-bottom: 2px solid;
> Don't need this, it's already 2px solid.
Fixed.

> >+  border: 1px solid !important;
> Make this 1px dotted here since it's transparent anyway.
Fixed.

> >+.mini-button:focus {
> >+  border-style: dotted !important;
> Thus avoiding having to change it here.
Fixed.

> >+#clearListButton {
> >+  margin-top: 5px;
> >+  margin-bottom: 5px;
> >+  -moz-margin-start: 5px;
> >+}
> Looks fine to me without these margins but you could try
> margin: 2px 4px; or margin: 4px;
Fixed using 2px 4px.

> >+.small-indent {
> >+  -moz-margin-start: 15px; 
> >+  -moz-margin-end: 15px;
> Use margin-left and margin-right when the values are the same.
Fixed.

> >+#extensionDescription {
> >+  color: #324055;
> Strange choice of colour...
Any suggestions? They are all just funny hex numbers to me.

> >+  list-style-image: url("chrome://communicator/skin/icons/loading.gif");
> >+  -moz-image-region: auto;
> I guess we won't need that now that my "new" badge image needs no region.
Fixed.

> >+.throbber {
> >+  list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif");
> Oops
Fixed.

> >+  margin-top: 1px;
> >+  margin-bottom: 4px;
> >+  -moz-margin-start: 5px;
> >+  -moz-margin-end: 5px;
> margin: 1px 5px 4px;
Fixed.

> >+#moreDetails {
> >+  margin: 1px 5px 4px 3px;
> Oops, not RTL-safe ;-)
Fixed.

> >+  margin: 1px 5px 4px 5px;
> Can write 1px 5px 4px;
Fixed.

> >+#downloadThrobber {
> >+  margin-left: 5px;
> -moz-margin-start?
Fixed.

> >+#downloadStatusProgress {
> >+  padding-right: 5px;
> -moz-padding-start?
I used -moz-padding-end.

> >+#verificationFailedIcon {
> >+  margin-left: 5px;
> margin-start?
Fixed.

> >+  margin: 1px 5px 4px 5px;
> 1px 5px 4px;
Fixed.

> >+  margin-left: 27px;
> margin-start?
Fixed.

Comment 16

[[e-mail]]

about.css uses #22262F so I guess that's as good as any ;-)

Comment 17

[[e-mail]]

350: trailing whitespace.
  margin-top: 1px;
365: trailing whitespace.
  margin-left: 15px;
370: trailing whitespace.
  -moz-margin-start: 0;
1238: trailing whitespace.
  list-style-image: url("chrome://mozapps/skin/update/update.png");
1327: trailing whitespace.

Comment 18

[[e-mail]]

Comment on attachment 395888 [details]
Optimised icons

Whoops, I accidentally switched notifyBadge and updateBadge :-(

Comment 19

[[e-mail]]

Attachment: Fixed ratings.png

I got this back-to-front, so that the CSS I gave for a rating of 4 is really a rating of 1 with this corrected image etc.

Comment 20

[[e-mail]]

Comment on attachment 396065 [details] [diff] [review]
Patch v1.1

>+/* Not Implemented Yet.
>+.pause[disabled="true"] {
>+  -moz-image-region: rect();
>+}
Actually I think an opacity of 0.5 might work.

Comment 21

[Philip Chee]

Attachment: Patch v1.2 Fixed remaining problems [Checkin: Comment 22]

Carrying forward r+/sr+ from Neil

> about.css uses #22262F so I guess that's as good as any ;-)
Fixed.

> 350: trailing whitespace.
> 365: trailing whitespace.
> 370: trailing whitespace.
> 1238: trailing whitespace.
> 1327: trailing whitespace.
Fixed.

> Whoops, I accidentally switched notifyBadge and updateBadge :-(
Fixed.

> Created an attachment (id=396196) [details]
> Fixed ratings.png
> 
> I got this back-to-front, so that the CSS I gave for a rating of 4 is really a
> rating of 1 with this corrected image etc.
Fixed.

>>+/* Not Implemented Yet.
>>+.pause[disabled="true"] {
>>+  -moz-image-region: rect();
>>+}
> Actually I think an opacity of 0.5 might work.
Fixed.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 396206 [details] [diff] [review]
Patch v1.2 Fixed remaining problems
[Checkin: Comment 22]


http://hg.mozilla.org/comm-central/rev/7a15328eaa67

Comment 23

[Sven Grull]

Could it be that this checkin removed the icons from add-on manager in modern theme accidentally?

Comment 24

[Philip Chee]

> Could it be that this checkin removed the icons from add-on manager in modern
> theme accidentally?

No. It wasn't accidental.

Comment 25

[Sven Grull]

(In reply to comment #24)
> No. It wasn't accidental.

So for what reason were they removed? At the moment without the icons the add-on manager doesn't look very nice.