Closed Bug 568052 Opened 10 years ago Closed 9 years ago

Adapt Modern theme for new Add-on Manager

Categories

(SeaMonkey :: Themes, defect, major)

defect
Not set
major

Tracking

(blocking-seamonkey2.1 final+)

RESOLVED FIXED
seamonkey2.1final
Tracking Status
blocking-seamonkey2.1 --- final+

People

(Reporter: InvisibleSmiley, Assigned: iann_bugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

The new Add-on Manager (which now opens in a new tab or window) requires porting the corresponding CSS style rules (extensions.css) to the Modern theme, maybe also new images etc.

The initial changeset that affected the Default theme was this:
http://hg.mozilla.org/mozilla-central/rev/e3b94f3e9baa
blocking-seamonkey2.1: --- → ?
see also bug 562978 which applies to all themes other than Default and might someday become a tracking bug.

See attachment #445647 [details] (Modern) and attachment #445650 [details] (Default) for comparative screenshots taken around midnight PDT in the night between 16 and 17 May (the date-times at bottom right of the screenshots are UTC+0200).
blocking-seamonkey2.1: ? → b1+
Assignee: nobody → bugspam.Callek
I think we don't need to block beta for this, though would be nice to have it fixed by then. Definitely need to block final though
blocking-seamonkey2.1: b1+ → final+
Duplicate of this bug: 638994
Attached patch almost 1:1 port (obsolete) — Splinter Review
Note: I'm *not* taking this, just trying to move things forward. From what I've heard, Ratty wanted to give this a whirl. HTH

The attached patch was created by
0. rm suite/themes/modern/mozapps/extensions/*
1. cp mozilla/toolkit/themes/winstripe/mozapps/extensions/* suite/themes/modern/mozapps/extensions/
2. rm suite/themes/modern/mozapps/extensions/*aero*
3. hg add suite/themes/modern/mozapps/extensions/*
4. adding suite/themes/modern/mozapps/extensions/* to suite/themes/modern/jar.mn
5. replacing the Toolkit throbber by the Modern throbber in update.css
6. removing the %ifdef WINSTRIPE_AERO block from extensions.css
7. hg qnew modern-aom.patch

I deliberately refrained from making more changes (like comparing to the old versions of the CSS files) so that whoever takes a stab at this won't be distracted.
Comment on attachment 525465 [details] [diff] [review]
almost 1:1 port

Ok, I have not yet tested this, but I want to spin the first rc this week if I can.

I will be using this as a base, and I'm not the best themer... so early review appreciated.
Attachment #525465 - Flags: review?(neil)
Comment on attachment 525465 [details] [diff] [review]
almost 1:1 port

Obviously this is generally much better than what we've currently got. Without looking at the patch itself (which had trailing whitespace on some jar.mn lines) it looks as if there are some minor nits that need to be addressed:
*All .addon-control styles should be removed, so that the buttons look normal.
*.header-button should look like button[type="menu"] (without the min-width)
*Various colours should match modern, such as
 *.button-link colours to match .text-link (might get changed back to a button)
 *[active="false"] colours to match disabled="true"
 *Background colours throughout
Attachment #525465 - Flags: review?(neil) → feedback+
[I've been sick the last few days and only recovering slowly. Don't count on me finishing this quickly, but feel free to request a patch of what I have now any time if that helps moving things forward.]

(In reply to comment #7)
> the patch itself (which had trailing whitespace on some jar.mn lines)

Fixed locally.

> *All .addon-control styles should be removed, so that the buttons look normal.

Fixed locally (kept the disabled rule).

> *.header-button should look like button[type="menu"] (without the min-width)

Huh? Comparing with the "Tabs" dropdown-button on the Sidebar, the rules from global/toolbarbutton.css seem to apply; I found none specifically for button[type="menu"]. So if I just remove all .header-button rules I get just the wheel + dropdown marker, transparent (i.e. not the darker grey background used elsewhere in Modern but the current background color of the about:addons page), so basically the default border and fancy gradients removed. 

Still I couldn't find anything relating to min-width (be it in the existing or new rules). Elaborate please?

> *Various colours should match modern, such as
>  *.button-link colours to match .text-link (might get changed back to a button)
>  *[active="false"] colours to match disabled="true"
>  *Background colours throughout

I'd love to leave those to someone with more Modern/themes experience. Getting this right could take me days, and it's not exactly important to me to be honest...
just a quick reminder that commander_keen marked bug 638994 as a "dupe" of this bug around early April.  hopefully you guys have read all my comments in bug 638994.  guess I have to wait for this bug to get resolved, which will also fix the problems I mentioned in bug 638994.
(In reply to comment #9)
> hopefully you guys have read all my comments in bug 638994

Don't (ever) count on that. Very obvious issues like this one often spawn many duplicates and no developer who's just watching the main bug will go through all comments of all bugs marked as duplicates.

That said, it seems the comments over there can be summarized as:
1. The buttons are all out of place.
2. The above is an issue esp. with screen sizes like 800x600.

If there's more to it, please explain *here*.

> guess I have to wait for this bug to get resolved, which will also fix
> the problems I mentioned in bug 638994.

Right. Basically, given the short time until SM 2.1 will be cut, I'll be bold and say that if you see no issues with the Classic/Default theme with your setup/configuration, you won't see any with Modern once this bug is fixed. If however you do see issues with Classic, speak up *now*.
Assignee: bugspam.Callek → iann_bugzilla
Status: NEW → ASSIGNED
Changes since last version:
* Fixed feedback comments (hopefully).
Attachment #525465 - Attachment is obsolete: true
Attachment #529707 - Flags: review?(neil)
Comment on attachment 529707 [details] [diff] [review]
Revised with backgrounds and colours patch v1.2

This:
* fixes the jar.mn trailing whitespace issues
* changes the bg color of the page to Modern's light gray
* changes the bg color of the buttons (including the wheel drop-down) to Modern's dark gray (with black rounded border)
* works with 800x600 (checked with Web Developer add-on): all action buttons stay visible; the main categories lose their labels but have tooltips; overflowing content triggers scrollbars.
* has good contrast for anything that is enabled and acceptable contrast for disabled items.
-> f=me :-)
Attachment #529707 - Flags: feedback+
(In reply to comment #8)
> (In reply to comment #7)
> > *All .addon-control styles should be removed, so that the buttons look normal.
> Fixed locally (kept the disabled rule).
[As I said, I hadn't read the patch, so I'd only bothered to look at enabled buttons, because I couldn't see the disabled ones ;-) ]

> > *.header-button should look like button[type="menu"] (without the min-width)
> Huh? Comparing with the "Tabs" dropdown-button on the Sidebar, the rules from
> global/toolbarbutton.css seem to apply; I found none specifically for
> button[type="menu"]. So if I just remove all .header-button rules I get just
> the wheel + dropdown marker, transparent (i.e. not the darker grey background
> used elsewhere in Modern but the current background color of the about:addons
> page), so basically the default border and fancy gradients removed. 
Right, but winstripe has it styled to look like a button (gnomestripe just leaves it with default styles, which looks like a button but only when it's hovered) which is why I suggested adding Modern button styles to it.
Comment on attachment 529707 [details] [diff] [review]
Revised with backgrounds and colours patch v1.2

git-apply complained about 5 lines with trailing whitespace that I must have overlooked last time because it only reports the first 5 which were in jar.mn

>+  skin/modern/mozapps/extensions/background-texture.png            (mozapps/extensions/background-texture.png)
Unused.

> #clientBox {
>-  background-color: #FFFFFF;
>-  color: #000000;
>+  background-color: -moz-Dialog;
>+  color: -moz-DialogText;
This can't be right. (Actually, I'm not sure the old background colour was right either.)

>+@media all and (-moz-windows-compositor) {
>+  #genericAbout {
>+    -moz-appearance: -moz-win-glass;
>+    background: transparent;
>+  }
>+}
Oops, unnecessary.

>diff --git a/suite/themes/modern/mozapps/extensions/alerticon-error.png b/suite/themes/modern/mozapps/extensions/alerticon-error.png
I wonder whether these files have seen pngcrush and optipng.
(See stefanh for details, or check the logs for our pngs for the bug.)

>diff --git a/suite/themes/modern/mozapps/extensions/blocklist.css b/suite/themes/modern/mozapps/extensions/blocklist.css
>--- a/suite/themes/modern/mozapps/extensions/blocklist.css
>+++ b/suite/themes/modern/mozapps/extensions/blocklist.css
>@@ -1,50 +1,14 @@
>-/* ***** BEGIN LICENSE BLOCK *****
This change looks wrong.

>-  border-bottom: 1px solid #A5ABC0;
>+  border-bottom: 1px solid #C0C0C0;
As does this.

>-  color: #000000;
>-  background-color: #FFFFFF;
>-  margin-top: 1em;
>-  margin-bottom: 1em;
>+  -moz-appearance: none;
>+  color: -moz-FieldText;
>+  background-color: -moz-Field;
>+  margin: 1em;
>   border: 1px solid;
>-  border-colors: #6E7378 #EEF0F3 #EEF0F3 #6E7378;
>+  -moz-border-top-colors: ActiveBorder;
>+  -moz-border-right-colors: ActiveBorder;
>+  -moz-border-bottom-colors: ActiveBorder;
>+  -moz-border-left-colors: ActiveBorder;
Again, the colour changes look wrong here.

>- * The Original Code is SeaMonkey modern code.
>+ * The Original Code is the Extension Manager UI.
>  *
>  * The Initial Developer of the Original Code is
>- * Kuden <spitfire.kuden@gmail.com>
>- * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
IMHO these changes are wrong.

>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>+ *   Blair McBride <bmcbride@mozilla.com>
...but this change is good.

>+#view-port-container {
>+  /* Needed to allow the radius to clip the inner content, see bug 595656 */
>+  /* Disabled because of bug 623615
>+  overflow: hidden;
>+  */
>+  background-color: #C7D0D9;
>+  border: 1px solid #494F5D;
>+  border-radius: 10px;
The border radius looks very ugly when combined with disabled or selected items (the smaller radius from the previous patch looks a little better, but it's still dodgy). I've got some ideas to improve this in a followup bug.

>+/* Plugins aren't yet disabled by safemode (bug 342333),
>+   so don't show that warning when viewing plugins. */
>+#addons-page[warning="safemode"] .view-pane[type="plugin"] .global-warning-container,
>+#addons-page[warning="safemode"] #detail-view[loading="true"] .global-warning-container {
>+  background-color: inherit;
>+  background-image: none;
>+}
Of course, we won't see the safe mode warning in Modern anyway ;-)

>+  -moz-appearance: none;
Don't need this at all.

>+#category-search > .category-icon {
>+  list-style-image: url("chrome://mozapps/skin/extensions/category-search.png");
These should be set directly on the item itself, and allowed to inherit.

>+@media all and (-moz-windows-default-theme) {
More unnecessary stuff.

>+.sorter[checkState="1"],
>+.sorter[checkState="2"] {
>+  background-color: rgba(194, 200, 206, 0.4);
Not sure what this does (don't know how to display it)

>+#detail-contrib-btn {
>+  -moz-appearance: none;
I can't decide whether this should just be a normal button but we don't need to worry about the -moz-appearance ;-)

>+.download-progress {
Again, didn't know how to check this.

>+  <filter id="greyscale">
>+    <feColorMatrix values="0.3333 0.3333 0.3333 0 0
>+                           0.3333 0.3333 0.3333 0 0
>+                           0.3333 0.3333 0.3333 0 0
>+                           0      0      0      1 0"/>
I don't like this sort of greyscale. Most graphics packages use desaturation instead. <feColorMatrix type="saturate" values="0"/>

>diff --git a/suite/themes/modern/mozapps/extensions/themeGeneric.png b/suite/themes/modern/mozapps/extensions/themeGeneric.png
This change is unnecessary (stefanh optimised the existing file).

>-  list-style-image: url("chrome://mozapps/skin/update/update.png");
>+  list-style-image: url("chrome://mozapps/skin/update/update.png"); 
Aha, I actually spotted one of those trailing spaces.

> .alertBox {
>-  background-color: #FFFFE7;
>-  color: #000000;
>-  border: 1px outset #FFFFE7;
>+  background-color: InfoBackground;
>+  color: InfoText;
>+  border: 1px outset InfoBackground;
Oops.
Attachment #529707 - Flags: review?(neil)
Changes since v1.2:
* Addressed comments - hopefully anything else can be in followups.
Attachment #529707 - Attachment is obsolete: true
Attachment #530446 - Flags: review?(neil)
Comment on attachment 530446 [details] [diff] [review]
Revised with backgrounds and colours patch v1.3

OK, so the reason I didn't spot the whitespace errors before was that for some reason I don't get warned for more than 5 whitespace errors in a patch that removes files. Here's the actual list of remaining whitespace errors:

(155) #extensionCreator, .contributor {
(156)   margin: 0px;
 157: }
       ^

1894:  background-image: -moz-linear-gradient(rgba(251, 252, 253, 0.70), rgba(246, 247, 248, 0.27) 49%,

2025:  background-image: -moz-linear-gradient(rgba(251, 252, 253, 0.95), rgba(246, 247, 248, 0.47) 49%,

Plus extensions.svg is in DOS format instead of UNIX line endings.
r=me with the line endings fixed.
Attachment #530446 - Flags: review?(neil) → review+
Please log new bugs for any follow-ups.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
(In reply to comment #10)
> (In reply to comment #9)
> > hopefully you guys have read all my comments in bug 638994
> 
> Don't (ever) count on that. Very obvious issues like this one often spawn
> many duplicates and no developer who's just watching the main bug will go
> through all comments of all bugs marked as duplicates.
> 
> That said, it seems the comments over there can be summarized as:
> 1. The buttons are all out of place.
> 2. The above is an issue esp. with screen sizes like 800x600.
> 
> If there's more to it, please explain *here*.
> 
> > guess I have to wait for this bug to get resolved, which will also fix
> > the problems I mentioned in bug 638994.
> 
> Right. Basically, given the short time until SM 2.1 will be cut, I'll be
> bold and say that if you see no issues with the Classic/Default theme with
> your setup/configuration, you won't see any with Modern once this bug is
> fixed. If however you do see issues with Classic, speak up *now*.

yup.  no serious issues with the SM 2.1 Classic theme as well as the Modern theme.  this bug as well as bug 568052 have been fixed in the May 7 SM2.1pre nightly build that I've just tried out on my WinXP machine (woo-hoo!)
You need to log in before you can comment on or make changes to this bug.