Closed Bug 601022 Opened 14 years ago Closed 14 years ago

Add final visual style and graphics to add-ons manager

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: jboriss, Assigned: Unfocused)

References

Details

Attachments

(9 files, 6 obsolete files)

780.08 KB, image/png
Details
202.28 KB, image/png
Details
172.32 KB, image/png
Details
208.25 KB, image/png
Details
1.04 MB, image/png
Details
978.50 KB, image/png
Details
230.64 KB, patch
dao
: review+
mossop
: feedback+
Details | Diff | Splinter Review
922.56 KB, image/png
Details
453.69 KB, image/png
Details
This bug will track the addition of the final colors, textures, gradients, and files to the add-ons manager for Firefox 4.0.

Foreground color and graphics:

• See attachment: "Foreground color and graphics"
• All files: http://people.mozilla.com/~jboriss/bugs/addons_manager_styling/foreground_colors_and_graphics.zip

Background color and graphics:

• See attachment for OSX: "OSX Background Color and Graphics"
• See attachment for Windows 7: "Windows 7 Background Color and Graphics"
• All files: http://people.mozilla.com/~jboriss/bugs/addons_manager_styling/background_colors_and_graphics.zip
The images I marked above as attachments are too big and make Bugzilla's head explode, so I threw them online:

Foreground color and graphics:

• http://people.mozilla.com/~jboriss/bugs/addons_manager_styling/foreground_color_and_graphics.png

OSX background color and graphics:

• http://people.mozilla.com/~jboriss/bugs/addons_manager_styling/osx_background_color_and_graphics.png

• Windows 7 background color and graphics:

http://people.mozilla.com/~jboriss/bugs/addons_manager_styling/windows_7_background_color_and_graphics.png
OS: Mac OS X → Windows 7
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
blocking2.0: --- → ?
All these blockers are tied together, submit patches however is best.
Blocks: 590175, 590218, 590221
blocking2.0: ? → final+
Blocks: 563565
At present, in default theme color scheme used by Add-ons is not matching that of default theme. It looks odd(or not formal) when you see current Addon screen, when user use windows classic theme.

http://people.mozilla.com/~jboriss/bugs/addons_manager_styling/foreground_color_and_graphics.png
also not matching with windows classic theme.

Same issue is there for Tabcandy too
I have created Bug 601305

I also have another suggestion - bug 601306
RFE: Need an option to Hide disabled extension and plugin
Depends on: 601305
(In reply to comment #3)
> At present, in default theme color scheme used by Add-ons is not matching that
> of default theme.

That's because it's not finished yet, thus we have this bug ;)

> It looks odd(or not formal) when you see current Addon
> screen, when user use windows classic theme.
> 
> http://people.mozilla.com/~jboriss/bugs/addons_manager_styling/foreground_color_and_graphics.png
> also not matching with windows classic theme.

See bug 563565 for using system-colors when appropriate.

> I also have another suggestion - bug 601306
> RFE: Need an option to Hide disabled extension and plugin

I don't see how that's related to this bug.
Depends on: 603144
Attached patch WiP v1 (Windows-only) (obsolete) — Splinter Review
Windows only. This makes some changes to the structure of the XUL/XBL, so the OSX/Linux themes probably look a bit uglier with this applied.

Notes:
* Still need to make the aero-style light beams only apply when on Vista/7, when aero-glass (compositor) is enabled.
* The icon of a disabled addon is greyscale in the list view, but not in the detail view. For some reason, trying that just makes it not show at all. There's a couple of SVG bugs that could cause this (bug 577824, bug 590434), but I gave up spending time on it since its not a big thing (its mostly important for list view).
* For consistency, I moved the search-loading indicator to be in the search view. 
* The styling of the loading indicators for search and details will probably pick up the style used in bug 601143, rather than whats in this patch (although they're similar).
* Waiting on some graphics from Boriss - gear icon, and updated red stripes.
* Waiting on feedback from Boriss about what to do about integration with the page's browser tab. The patch currently has a gradient at the top, which fades out the texture/color/etc, to blend with the tab's background color.

Things that weren't possible:
* Inset shadow in the viewport and selected category item. I couldn't find any acceptable way to cut out the shadow where the category meets the view port - and any hacks I tried worked acceptably.
Attachment #482194 - Flags: review?(dtownsend)
Attachment #482194 - Attachment description: WiP v1 → WiP v1 (Windows-only)
Can we get a screenshot with the WIP applied?
Attached image WiP v1 list-view screenshot (Win7) (obsolete) —
(In reply to comment #6)
> Can we get a screenshot with the WIP applied?

Oh, yes - knew I forgot something!
Attachment #482399 - Attachment description: WiP v1 screenshot (Win7) → WiP v1 list-view screenshot (Win7)
Couple of other things to note:

* The patch purposefully goes against what bug 563565 proposes. Its overrides all system colors, so the interface should look (mostly) the same on any Windows theme. This guarantees that the interface will always be useable on any theme - which is quite a challenge when using system colors, since everything has custom styles (the best we can do is mix-and-match system colors and custom colors, which doesn't work so well in practice). Already discussed this with Boriss and Mossop - putting it here for future reference.

* If the window is small (800px wide, or less), I've made it so the categories list collapses to just icons. This makes the view-port much bigger on smaller screens, where the categories list was (needlessly) taking up half the screen real-estate. That solves one very common issue with using the EM on small screens. Going to need to add a tooltip though (which will be just the category name, so no string changes needed).
OS: Windows 7 → All
Hardware: x86 → All
Depends on: 562865
Do you know how it looks in Windows XP? It still is the most used operating system... The gradient that connects the tab with the page probably looks out of place then?

As a test in my CSS skills, I decided to recreate the mockups locally. I think reducing the size (font and icon size) of the categories looks a lot more mature. It also puts more focus where it should be, on the page contents.
Comment on attachment 482194 [details] [diff] [review]
WiP v1 (Windows-only)

This looks good, one correction that needs to be made however I'd really like it if we could get dao or Ryan to find the time to look over the styles, they have way more knowledge with CSS than me.

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js

>     var sortService = Cc["@mozilla.org/xul/xul-sort-service;1"].
>                       getService(Ci.nsIXULSortService);
>     sortService.sort(this._listBox, aSortBy, hints);
> 
>+    var item = this._listBox.querySelector("richlistitem[remote='true'][first]");
>+    if (item)
>+      item.remoteItems.remoteAttribute("first");
>+    item = this._listBox.querySelector("richlistitem[remote='true'][last]");
>+    if (item)
>+      item.remoteItems.remoteAttribute("last");

These look broken to me.

>+    var items = this._listBox.querySelectorAll("richlistitem[remote='true']");
>+    if (items.length > 0) {
>+      items[0].setAttribute("first", true);
>+      items[items.length - 1].setAttribute("last", true);
>+    }

Is there really not a nicer way to do this? :(

>diff --git a/toolkit/themes/winstripe/mozapps/extensions/extensions.css b/toolkit/themes/winstripe/mozapps/extensions/extensions.css

>-#category-search > .category-icon {
>+#category-search .category-icon {

Any reason for dropping all the >'s? It makes the lookups a bit quicker I believe.
Attachment #482194 - Flags: review?(dtownsend) → review-
(In reply to comment #11)
> >-#category-search > .category-icon {
> >+#category-search .category-icon {
> 
> Any reason for dropping all the >'s? It makes the lookups a bit quicker I
> believe.

Opps - that was left-over from some ill-fated experiments.
(In reply to comment #10)
> Do you know how it looks in Windows XP? It still is the most used operating
> system... The gradient that connects the tab with the page probably looks out
> of place then?

Sadly, I don't have a XP install to test on. Will see what I can do.

> I think
> reducing the size (font and icon size) of the categories looks a lot more
> mature. It also puts more focus where it should be, on the page contents.

Boriss: thoughts?
(In reply to comment #13)
> (In reply to comment #10)
> > Do you know how it looks in Windows XP? It still is the most used operating
> > system... The gradient that connects the tab with the page probably looks out
> > of place then?
> 
> Sadly, I don't have a XP install to test on. Will see what I can do.

Lets start a tryserver build so everyone with an XP machine/vm can test it. If it's not ready for yet, we should wait until remaining questions are solved. I could also ask for feedback in our nightly tester list.
Depends on: 579636
Depends on: 604869
(In reply to comment #13)
> (In reply to comment #10)
> > Do you know how it looks in Windows XP? It still is the most used operating
> > system... The gradient that connects the tab with the page probably looks out
> > of place then?
> 
> Sadly, I don't have a XP install to test on. Will see what I can do.

FYI, you might also be interested in bug 543910 then.
(In reply to comment #11)
> >+    var item = this._listBox.querySelector("richlistitem[remote='true'][first]");
> >+    if (item)
> >+      item.remoteItems.remoteAttribute("first");
> >+    item = this._listBox.querySelector("richlistitem[remote='true'][last]");
> >+    if (item)
> >+      item.remoteItems.remoteAttribute("last");
> 
> These look broken to me.

How so?


> >+    var items = this._listBox.querySelectorAll("richlistitem[remote='true']");
> >+    if (items.length > 0) {
> >+      items[0].setAttribute("first", true);
> >+      items[items.length - 1].setAttribute("last", true);
> >+    }
> 
> Is there really not a nicer way to do this? :(

Sadly, no :( Unless we split out local and remote results into their own richlistboxes (which I originally tried, but discovered half the search view would need rewritten). That code is solving the same issue we had with the rows in the details view (bug 592708).
(In reply to comment #15)
> FYI, you might also be interested in bug 543910 then.

I saw that - not sure if its needed for this or not. I'm now changing the background based on whether its running with aero (-moz-windows-compositor), the classic theme (-moz-windows-classic), and a catch-all for anything else - it seems to work really well for the various theming options in Windows 7. I'm hoping to get Windows XP running in a VM this week to play around with it.
Attached patch WiP v2 (Windows & Linux) (obsolete) — Splinter Review
Various tweaks, changes, and additions to the Windows theme.

Linux theme now has no hard-coded colors, other than colors that overlay ontop of system colors (eg, notification gradients). Also added icons to buttons, which only show up if GTK is configured to show icons for buttons. Tested with various themes - light and dark.
Attachment #483960 - Flags: feedback?(dtownsend)
Attachment #482194 - Attachment is obsolete: true
It's more obvious here that there's not a seemless transition between the tab and the content, when the navigation bar is hidden.
In the Windows theme, is the button theme exclusive to the add-ons manager? It's just that I've seen the being used in other mockups.
(In reply to comment #16)
> (In reply to comment #11)
> > >+    var item = this._listBox.querySelector("richlistitem[remote='true'][first]");
> > >+    if (item)
> > >+      item.remoteItems.remoteAttribute("first");
> > >+    item = this._listBox.querySelector("richlistitem[remote='true'][last]");
> > >+    if (item)
> > >+      item.remoteItems.remoteAttribute("last");
> > 
> > These look broken to me.
> 
> How so?

I don't now what remoteItems or remoteAttribute are, they don't exist anywhere in our codebase. I suspect you just want item.removeAttribute in both cases.
(In reply to comment #23)
> > How so?
> 
> I don't now what remoteItems or remoteAttribute are, they don't exist anywhere
> in our codebase. I suspect you just want item.removeAttribute in both cases.

Oh, wow, I didn't see those typos :\ Kinda surprised that didn't blow up more obviously.
(In reply to comment #22)
> In the Windows theme, is the button theme exclusive to the add-ons manager?
> It's just that I've seen the being used in other mockups.

The buttons in the header are almost exactly the same as Firefox's toolbar buttons (minus the drop shadow). The other buttons are also very similar, with some additional tweaks - eg, the corners aren't as rounded, there's additional padding on either side of the button text.
Attached patch WiP v3 (Windows and Linux) (obsolete) — Splinter Review
Attachment #483960 - Attachment is obsolete: true
Attachment #484540 - Flags: feedback?(dtownsend)
Attachment #483960 - Flags: feedback?(dtownsend)
Attachment #484540 - Flags: review?(dao)
Comment on attachment 484540 [details] [diff] [review]
WiP v3 (Windows and Linux)

Looks fine from my point of view, leaving the nitty gritty theme reviewing to dao though
Attachment #484540 - Flags: feedback?(dtownsend) → feedback+
Blocks: 601143
Blocks: 604869
No longer depends on: 604869
Blocks: 603144
No longer depends on: 603144
Comment on attachment 484540 [details] [diff] [review]
WiP v3 (Windows and Linux)

global nit: replace 0xp and 0em with 0

Links in selected list items are invisible over here (Linux). Should add color: inherit; there.

>               <xul:vbox anonid="relnotes-container" class="relnotes-container">
>                 <xul:label class="relnotes-header" value="&addon.releaseNotes.label;"/>
>                 <xul:label anonid="relnotes-loading" value="&addon.loadingReleaseNotes.label;"/>
>                 <xul:label anonid="relnotes-error" hidden="true"
>                            value="&addon.errorLoadingReleaseNotes.label;"/>
>-                <xul:vbox anonid="relnotes"/>
>+            <xul:vbox anonid="relnotes" class="relnotes"/>
>               </xul:vbox>

indentation is off

>--- a/toolkit/mozapps/extensions/content/extensions.xul
>+++ b/toolkit/mozapps/extensions/content/extensions.xul
>@@ -56,17 +56,17 @@
> 
>   <xhtml:link rel="shortcut icon"
>               href="chrome://mozapps/skin/extensions/extensionGeneric-16.png"/>
> 
>   <script type="application/javascript"
>           src="chrome://mozapps/content/extensions/extensions.js"/>
>   <script type="application/javascript"
>           src="chrome://global/content/contentAreaUtils.js"/>
>-
>+  

bogus whitespace change

>+.loading > image {
>+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
>+}

Can you reduce the selector to just ".loading"? The image node should automatically pick up the list-style-image, I think.

>+/* Maximize the size of the viewport when the window is small */
>+@media all and (max-width: 800px) {
>+  .category-name {
>+    display: none;
>+  }
>+}

Looks like the categories should have tooltips for that case.

>+.addon-view[active="false"]:not([selected]) .name-container,
>+.addon-view[active="false"]:not([selected]) .creator,
>+.addon-view[active="false"]:not([selected]) .description,
>+.addon-view[active="false"]:not([selected]) .date-updated {
>+  color: GrayText;
> }

This looks like it should just be:
.addon-view[active="false"]:not([selected]) {
  color: GrayText;
}

>-.warning, .pending, .error, .info {
>+.warning, .pending, .error {
>   -moz-margin-start: 48px;
> }

> .download-progress .pause, .download-progress .cancel {

>+.download-progress .pause:hover, .download-progress .cancel:hover {

[etc.]

nit: line break after commas in selectors

>--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css

> #addons-page {
>-  background-color: #c5ccd7;
>   -moz-appearance: none;
>-  margin: 20px;
>+  background-color: #CCD9EA;
>+  background-image: /* Fade-out texture at the top */
>+                    -moz-linear-gradient(top, rgb(231, 237, 346), rgba(231, 237, 346, 0) 156px),
>+                    /* Texture */
>+                    url("chrome://mozapps/skin/extensions/background-texture.png");
>+  background-repeat: repeat;
>+  padding: 18px;
>+  color: #000;
>+}

>+@media all and (-moz-windows-classic) {
>+  #addons-page {
>+    background-color: -moz-dialog;
>+    background-image: /* Fade-out texture at the top, and blend with browser tab */
>+                      -moz-linear-gradient(rgba(255, 255, 255, 0.5), rgba(255, 255, 255, 0) 30%),
>+                      /* Texture */
>+                      url("chrome://mozapps/skin/extensions/background-texture.png");
>+  }
> }

Why is classic special-cased here? If it's because the colors might not fit, then the logic should be reversed, i.e. the bottom part should be used unconditionally and the top part should be used depending on -moz-windows-default-theme.

>+@media all and (-moz-windows-compositor) {

Are you sure you want to exclude aero basic here?

> #back-btn:-moz-locale-dir(ltr),
> #forward-btn:-moz-locale-dir(rtl) {
>   -moz-image-region: rect(0, 18px, 18px, 0);
>+  -moz-border-radius-topright: 0px;
>+  -moz-border-radius-bottomright: 0px;

Use the non-prefixed border-radius properties (applies elsewhere too).

>+  border
> }

typo

>+.addon-view[active="false"] .name-container,
>+.addon-view[active="false"] .creator,
>+.addon-view[active="false"] .description,
>+.addon-view[active="false"] .date-updated {
>+  color: #888A8B;
>+}

Same suggestion as for gnomestripe:
.addon-view[active="false"] {
  color: #888A8B;
}

>+.header-button {
>+  -moz-appearance: none;
>+  margin: 0px;
>+  background: rgba(151,152,153,.05)
>+              -moz-linear-gradient(rgba(251, 252, 253, 0.95), rgba(246, 247, 248, 0.47) 49%, 
>+                                   rgba(231, 232, 233, 0.45) 51%, rgba(225, 226, 229, 0.3));
>+  background-clip: padding-box;
>+  border-radius: 4.5px;
>+  border: 1px solid;
>+  border-color: rgba(0, 0, 0, 0.12) rgba(0, 0, 0, 0.19) rgba(0, 0, 0, 0.38);
>+  box-shadow: 0 0 0 1px rgba(255, 255, 255, 0.3) inset,
>+              0 0 0 2px rgba(255, 255, 255, 0.1) inset;
>+}
>+
>+.header-button[disabled="true"] {
>+  -moz-border-top-colors: rgba(0, 0, 0, 0.12) !important;
>+  -moz-border-left-colors: rgba(0, 0, 0, 0.19) !important;
>+  -moz-border-right-colors: rgba(0, 0, 0, 0.19) !important;
>+  -moz-border-bottom-colors: rgba(0, 0, 0, 0.38) !important;
>+  opacity: 0.8;
>+}

Why are you switching to -moz-border-*-colors all of a sudden?
(In reply to comment #28)
> >+.loading > image {
> >+  list-style-image: url("chrome://global/skin/icons/loading_16.png");
> >+}
> 
> Can you reduce the selector to just ".loading"? The image node should
> automatically pick up the list-style-image, I think.

Yes, that worked.

> >+/* Maximize the size of the viewport when the window is small */
> >+@media all and (max-width: 800px) {
> >+  .category-name {
> >+    display: none;
> >+  }
> >+}
> 
> Looks like the categories should have tooltips for that case.

I had originally been planning on doing that in a separate bug, but I've done it here instead.


> >+@media all and (-moz-windows-classic) {
> >+  #addons-page {
> >+    background-color: -moz-dialog;
> >+    background-image: /* Fade-out texture at the top, and blend with browser tab */
> >+                      -moz-linear-gradient(rgba(255, 255, 255, 0.5), rgba(255, 255, 255, 0) 30%),
> >+                      /* Texture */
> >+                      url("chrome://mozapps/skin/extensions/background-texture.png");
> >+  }
> > }
> 
> Why is classic special-cased here? If it's because the colors might not fit,
> then the logic should be reversed, i.e. the bottom part should be used
> unconditionally and the top part should be used depending on
> -moz-windows-default-theme.

Ok, done.


> >+@media all and (-moz-windows-compositor) {
> 
> Are you sure you want to exclude aero basic here?

Yes - the additional gradients in the backgrond should only apply when Aero glass is enabled (I had already asked Boriss about that).


> >+.header-button[disabled="true"] {
> >+  -moz-border-top-colors: rgba(0, 0, 0, 0.12) !important;
> >+  -moz-border-left-colors: rgba(0, 0, 0, 0.19) !important;
> >+  -moz-border-right-colors: rgba(0, 0, 0, 0.19) !important;
> >+  -moz-border-bottom-colors: rgba(0, 0, 0, 0.38) !important;
> >+  opacity: 0.8;
> >+}
> 
> Why are you switching to -moz-border-*-colors all of a sudden?

Because I need to override the style set in button.css - which in this case uses -moz-border-*-colors with the !important flag. Using just border-color doesn't seem to override that.
Comment on attachment 484540 [details] [diff] [review]
WiP v3 (Windows and Linux)

I'd like to see a new patch.
Attachment #484540 - Flags: review?(dao) → review-
Attached patch WiP v4 (Windows and Linux) (obsolete) — Splinter Review
* Review comments addressed
* Fixes for Windows XP
* EULA dialog updated
* About dialog updated
* Re-did the alert boxes so they're all uniform and generally fit in better
* Numerous small tweaks and fixes everywhere

I might yet tweak the download progress widget. It doesn't fit in with the rest of the theme as nicely as I want.

Although the OSX theme isn't done in this patch, I've made sure that it isn't broken by the various XUL changes. Assuming there are no major things left to fix in the Windows theme, I'll start adapting it to OSX. If the tree unfreezes before the OSX theme is finished, it might be nice to land Windows/Linux ahead of OSX.
Attachment #484540 - Attachment is obsolete: true
Attachment #487325 - Flags: review?(dao)
Attachment #487325 - Attachment is patch: true
Attachment #487325 - Attachment mime type: application/octet-stream → text/plain
Attachment #487325 - Flags: review?(dtownsend)
Comment on attachment 487325 [details] [diff] [review]
WiP v4 (Windows and Linux)

>diff --git a/toolkit/mozapps/extensions/content/about.js b/toolkit/mozapps/extensions/content/about.js

> function init() {
>   var addon = window.arguments[0];
>   var extensionsStrings = document.getElementById("extensionsStrings");
> 
>+  var dialog = document.getElementById("genericAbout");
>+  dialog.setAttribute("addontype", addon.type);

Just use document.documentElement.setAttribute(...

>diff --git a/toolkit/mozapps/extensions/content/eula.js b/toolkit/mozapps/extensions/content/eula.js

> function Startup() {
>   var bundle = document.getElementById("extensionsStrings");
>-  var label = document.createTextNode(bundle.getFormattedString("eulaHeader", [window.arguments[0].name]));
>+  var addon = window.arguments[0].addon;
>+
>+  document.getElementById("eula-dialog").setAttribute("addontype", addon.type);

Again just document.documentElement seems fine.

>diff --git a/toolkit/mozapps/extensions/content/extensions.xul b/toolkit/mozapps/extensions/content/extensions.xul

>     <!-- category list -->
>     <richlistbox id="categories" persist="last-selected">
>       <richlistitem id="category-search" value="addons://search/"
>                     class="category"
>-                    name="&view.search.label;" disabled="true"/>
>+                    name="&view.search.label;"
>+                    tooltiptext="&view.search.label;" disabled="true"/>

Is there much point in having the same text as label and tooltip?

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_searching.js b/toolkit/mozapps/extensions/test/browser/browser_searching.js

>+  // Check the "first" and "last" attributes are set correctly
>+  for (let i = 0; i < actualResults.length; i++) {
>+    if (i == 0) {
>+      is(actualResults[0].item.hasAttribute("first"), true,
>+         "First item should have 'first' attribute set");
>+      is(actualResults[0].item.hasAttribute("last"), false,
>+         "First item should not have 'last' attribute set");
>+    } else if (i == (actualResults.length - 1)) {
>+      is(actualResults[actualResults.length - 1].item.hasAttribute("first"), false,
>+         "Last item should not have 'first' attribute set");
>+      is(actualResults[actualResults.length - 1].item.hasAttribute("last"), true,
>+         "Last item should have 'last' attribute set");
>+    } else {
>+      is(actualResults[i].item.hasAttribute("first"), false,
>+         "First item should not have 'first' attribute set");
>+      is(actualResults[i].item.hasAttribute("last"), false,
>+         "First item should not have 'last' attribute set");

s/First item/item i/

>diff --git a/toolkit/themes/winstripe/mozapps/extensions/eula.css b/toolkit/themes/winstripe/mozapps/extensions/eula.css
>--- a/toolkit/themes/winstripe/mozapps/extensions/eula.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/eula.css
>@@ -1,17 +1,54 @@
>+/*
>+ @media all and (-moz-windows-compositor) {
>+  dialog {
>+    -moz-appearance: -moz-win-borderless-glass;
>+    background: transparent;
>+    padding: 0;
>+  }
>+  
>+  #content {
>+    border: 1px solid rgb(40%, 40%, 40%);
>+    background: -moz-Dialog;
>+  }
>+}
>+*/

Why is this commented out? Either uncomment it or remove it.
Attachment #487325 - Flags: review?(dtownsend) → review+
(In reply to comment #32)
> >diff --git a/toolkit/mozapps/extensions/content/extensions.xul b/toolkit/mozapps/extensions/content/extensions.xul
> 
> >     <!-- category list -->
> >     <richlistbox id="categories" persist="last-selected">
> >       <richlistitem id="category-search" value="addons://search/"
> >                     class="category"
> >-                    name="&view.search.label;" disabled="true"/>
> >+                    name="&view.search.label;"
> >+                    tooltiptext="&view.search.label;" disabled="true"/>
> 
> Is there much point in having the same text as label and tooltip?

I added the tooltips because I have the category names collapsing when the window is less than 800px wide (makes a *huge* difference in the size of the view-port at those small resolutions). Right now I have the tooltips there regardless of whether the category name is shown or not - with a hack in the category binding I could make it so its only there when the name is hidden, but I didn't see any real benefit.


> Why is this commented out? Either uncomment it or remove it.

Oops - that was meant to have been removed.
Once Bug 581193 is fixed, the default action for the button in add-on manager may be set as "Check for Updates" with the other options appearing in a dropdown menu.
Blocks: 579636
No longer depends on: 579636
Blocks: 562865, 601305
No longer depends on: 562865, 601305
Blocks: 586066
Blocks: 448559
Comment on attachment 487325 [details] [diff] [review]
WiP v4 (Windows and Linux)

>--- a/toolkit/themes/winstripe/mozapps/extensions/about.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/about.css

>+@media all and (-moz-windows-compositor) {
>+  #genericAbout {
>+    -moz-appearance: -moz-win-borderless-glass;
>+    background: transparent;
>+  }
>+
>+  #clientBox {
>+    border: 1px solid rgb(40%, 40%, 40%);
>+  }
>+}

Just use -moz-win-glass and skip the custom border?

>--- a/toolkit/themes/winstripe/mozapps/extensions/eula.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/eula.css
>@@ -1,17 +1,54 @@
>+/*
>+ @media all and (-moz-windows-compositor) {
>+  dialog {
>+    -moz-appearance: -moz-win-borderless-glass;
>+    background: transparent;
>+    padding: 0;
>+  }
>+  
>+  #content {
>+    border: 1px solid rgb(40%, 40%, 40%);
>+    background: -moz-Dialog;
>+  }
>+}
>+*/

Why is this commented out?
Comment on attachment 487325 [details] [diff] [review]
WiP v4 (Windows and Linux)

>--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css

> #addons-page {
>-  background-color: #c5ccd7;
>   -moz-appearance: none;
>-  margin: 20px;
>+  padding: 18px;
>+  color: #000;
>+  background-repeat: repeat;
>+  background-color: -moz-dialog;
>+  background-image: /* Fade-out texture at the top, and blend with browser tab */
>+                    -moz-linear-gradient(rgba(255, 255, 255, 0.5), rgba(255, 255, 255, 0) 30%),
>+                    /* Texture */
>+                    url("chrome://mozapps/skin/extensions/background-texture.png");
>+}
>+

Looks like you should be using color: -moz-dialogText here.

>+@media all and (-moz-windows-theme: aero) {
>+  #addons-page {
>+    background-color: #CCD9EA;
>+    background-image: /* Fade-out texture at the top */
>+                      -moz-linear-gradient(top, rgb(231, 237, 246), rgba(231, 237, 246, 0) 156px),
>+                      /* Texture */
>+                      url("chrome://mozapps/skin/extensions/background-texture.png");
>+  }
>+}
>+
>+@media all and (-moz-windows-compositor) {
>+  #addons-page {
>+    border: 1px solid rgb(40%, 40%, 40%);
>+    border-top: none;
>+    /* Blame shorlander for this monstrosity. */
>+    background-image: /* Fade-out texture and light beams at the top */
>+                      -moz-linear-gradient(top, rgb(231, 237, 246) 3px, rgba(231, 237, 246, 0) 156px),
>+                      /* Side gradients */
>+                      -moz-linear-gradient(left,
>+                                           rgba(255, 255, 255, 0.2), rgba(255, 255, 255, 0) 40%,
>+                                           rgba(255, 255, 255, 0) 60%, rgba(255, 255, 255, 0.2)),
>+                      /* Aero-style light beams */
>+                      -moz-linear-gradient(left 32deg,
>+                                           /* First light beam */
>+                                           rgba(255, 255, 255, 0) 19.5%, rgba(255, 255, 255, 0.1) 20%,
>+                                           rgba(255, 255, 255, 0.1) 21.5%, rgba(255, 255, 255, 0.2) 22%,
>+                                           rgba(255, 255, 255, 0.2) 25.5%, rgba(255, 255, 255, 0.1) 26%,
>+                                           rgba(255, 255, 255, 0.1) 27.5%, rgba(255, 255, 255, 0) 28%,
>+                                           /* Second light beam */
>+                                           rgba(255, 255, 255, 0) 49.5%, rgba(255, 255, 255, 0.1) 50%,
>+                                           rgba(255, 255, 255, 0.1) 52.5%, rgba(255, 255, 255 ,0.2) 53%,
>+                                           rgba(255, 255, 255, 0.2) 54.5%, rgba(255, 255, 255, 0.1) 55%,
>+                                           rgba(255, 255, 255, 0.1) 57.5%, rgba(255, 255, 255, 0) 58%,
>+                                           /* Third light beam */
>+                                           rgba(255, 255, 255, 0) 87%, rgba(255, 255, 255, 0.2) 90%),
>+                      /* Texture */
>+                      url("chrome://mozapps/skin/extensions/background-texture.png");
>+  }
> }

Move to extensions-aero.css (and replace -moz-windows-theme: aero with -moz-windows-default-theme).

nit: removing the spaces within rgba() will make this slightly more readable as a whole, I think.

> .name-container {
>   font-size: 150%;
>-  margin-bottom: 0px;
>+  font-weight: bold;
>+  color: #3F3F3F;
>+  margin-bottom: 0;
>+  text-shadow: 1px 1px 1px #FFF;  

trailing spaces

>+.addon-control:active {
>+  background-color: transparent;
>+  border-color: rgba(0, 0, 0, 0.65) rgba(0, 0, 0, 0.55) rgba(0, 0, 0, 0.5);
>+  box-shadow: 0 0 6.5px rgba(0, 0, 0, 0.4) inset,
>+              0 0 2px rgba(0, 0, 0, 0.4) inset,
>+              0 1px 0 rgba(255, 255, 255, 0.4);
>+}

Use :active:hover instead of :active? This may apply elsewhere too.
(In reply to comment #36)
> Move to extensions-aero.css (and replace -moz-windows-theme: aero with
> -moz-windows-default-theme).

Any specific reason why? I'd like to avoid it if possible - there's very little that's Aero-specific, so it wouldn't reduce the complexity of extensions.css by much. I asked Dave, and he agrees it'd be nice to avoid - but we both may be missing something.
It's a waste to have the code loaded, parsed etc. for non-aero Windows.

You may keep all code in one file and use preprocessing -- see http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/popup.css and http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/popup-aero.css
Blocks: 607150
Attached patch v5 (Windows, Linux, OSX) (obsolete) — Splinter Review
Changes in this patch:
* OSX theme
* Split out aero-specific stuff
* Buttons in the header are now toolbarbuttons (needed for the utilities button to look good on OSX, but I think it generally makes sense for them to be toolbarbuttons)
* Changed the all-results link in the search view to get its label/href set as an attribute (rather than a property), as I noticed some XBL-async-binding weirdness.
* New download progressbar (Boriss is going to attach mockups somewhere)
* Changes to gnomestripe to make certain things much more readable, and generally adapt better to different system themes
* Small tweaks here and there
Attachment #487325 - Attachment is obsolete: true
Attachment #489749 - Flags: review?(dao)
Attachment #487325 - Flags: review?(dao)
Attachment #489749 - Flags: feedback?(dtownsend)
Depends on: 611241
Note: In the latest patch, the icons of disabled addons in view view may not show. This seems to be due to the greyscale SVG filter (which used to work perfectly) - filed bug 611241.


(In reply to comment #35)
> Just use -moz-win-glass and skip the custom border?

I tried that at first, but the top glass border was drawn about 22px below the top of the client box. I don't know enough about -moz-win-glass to tell if its a bug or something weird about that dialog box - and using -moz-win-borderless-glass worked ok, so I didn't spend time investigating.
(In reply to comment #42)
> > Just use -moz-win-glass and skip the custom border?
> 
> I tried that at first, but the top glass border was drawn about 22px below the
> top of the client box. I don't know enough about -moz-win-glass to tell if its
> a bug or something weird about that dialog box - and using
> -moz-win-borderless-glass worked ok, so I didn't spend time investigating.

It's a bug that needs fixing (bug 606061). Please don't work around it.
Comment on attachment 489749 [details] [diff] [review]
v5 (Windows, Linux, OSX)

>diff --git a/toolkit/mozapps/extensions/content/extensions.xml b/toolkit/mozapps/extensions/content/extensions.xml

>         <xul:button anonid="undo-btn" class="button-link"
>                     label="&addon.undoAction.label;"
>                     tooltipText="&addon.undoAction.tooltip;"
>                     oncommand="document.getBindingParent(this).undo();"/>
>         <xul:spacer flex="5000"/> <!-- Necessary to allow the message to wrap -->
>       </xul:hbox>
> 
>       <xul:hbox>
>-        <xul:vbox align="center" pack="center" class="icon-container">
>+        <xul:vbox align="center" pack="start" class="icon-container">
>           <xul:image anonid="icon" class="icon"/>
>         </xul:vbox>
>         <xul:vbox flex="1">
>           <xul:hbox align="start">
>             <xul:vbox flex="1">
>               <xul:hbox class="name-container" align="end">

I didn't do this because the UI mockups called for the icon to be centered in the regular list view. I don't think it looks right this way, maybe add some padding to the top so it is roughly centered in the normal list but stays put in the expanded item.
Attachment #489749 - Flags: feedback?(dtownsend) → feedback+
Attached patch Patch v6Splinter Review
Note: This is on top of the patch in bug 602895, which hasn't landed yet. It adds a glow effect to the global warning icon when the warning message is collapsed (as described in bug 602895 comment 3), and changes the threshold of when the warning message will collapse (because this patch changes the amount of space the warning message has at smaller window sizes).

Also addresses comment 43 and comment 44.
Attachment #489749 - Attachment is obsolete: true
Attachment #490764 - Flags: review?(dao)
Attachment #490764 - Flags: feedback?(dtownsend)
Attachment #489749 - Flags: review?(dao)
Forgot to mention: I purposefully did not add a glow effect to the icon in gnomestripe, since I don't think that could be made to look good on all themes.
Can we get screenshots, at least for Win? There were only WIP v1 screenshots for it.
Attachment #490764 - Flags: feedback?(dtownsend) → feedback+
(In reply to comment #47)
> Can we get screenshots, at least for Win? There were only WIP v1 screenshots
> for it.

Nothing major in the look has changed - mostly just a lot of smaller tweaks, and cleaning up the code. I think the big noticeable difference in this screenshot is the download progress.
Attachment #482399 - Attachment is obsolete: true
And here's one of the message boxes, in the search view. I re-sized the window, to show how the categories list on the left collapses to give more room to the main content.
Blair, I miss the tabshadow as given by the mockups by Jennifer. What happened with those?
(In reply to comment #50)
> Blair, I miss the tabshadow as given by the mockups by Jennifer. What happened
> with those?

Do you mean the shadow inside the viewport and the selected category? Sadly, that was one of the things in the mockups that wasn't realistic to do. Same with extending the background color of the list items into the selected category, and extending the background of about:addons into its browser tab (though we figured out a compromise for that).
(In reply to comment #51)
> Do you mean the shadow inside the viewport and the selected category? Sadly,
> that was one of the things in the mockups that wasn't realistic to do. Same

Yes, exactly that part. Looks like I missed comment 5. Does it mean it is not possible at all? Otherwise we should file a new bug to get it implemented at a later stage. 

> with extending the background color of the list items into the selected
> category, and extending the background of about:addons into its browser tab
> (though we figured out a compromise for that).

It's because of the hbox and that both parts are positioned beneath each other, right? Is there a way to overlay items in XUL? Means putting the category list on top of the list, extend the list to the left, and add a padding at the left?
Comment on attachment 490764 [details] [diff] [review]
Patch v6

>--- a/toolkit/themes/pinstripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/pinstripe/mozapps/extensions/extensions.css
> 
> .download-progress .progress {
>-  background-color: transparent;
>   /* Force the progress meter to use the default binding rather than the OSX
>      animated binding */
>   -moz-binding: url("chrome://global/content/bindings/progressmeter.xml#progressmeter") !important;

This isn't needed anymore. http://hg.mozilla.org/mozilla-central/rev/a0351ac4eb45

>--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css

> #addons-page {
    [...]
>+%ifdef WINSTRIPE_AERO
>+  color: #000;
>+  background-color: #CCD9EA;
>+  background-image: /* Fade-out texture at the top */
>+                    -moz-linear-gradient(top, rgb(231,237,246), rgba(231,237,246,0) 156px),
>+                    /* Texture */
>+                    url("chrome://mozapps/skin/extensions/background-texture.png");
>+%else
>+  color: -moz-dialogText;
>+  background-color: -moz-dialog;
>+  background-image: /* Fade-out texture at the top, and blend with browser tab */
>+                    -moz-linear-gradient(rgba(255,255,255,0.5), rgba(255,255,255,0) 30%),
>+                    /* Texture */
>+                    url("chrome://mozapps/skin/extensions/background-texture.png");
>+%endif
> }
> 
>+%ifdef WINSTRIPE_AERO
>+@media all and (-moz-windows-compositor) {
>+  #addons-page {
      [...]
>+  }
>+}
>+%endif

Rather than Classic or third-party themes on Win 7, I suspect you want to target aero basic with the first %ifdef WINSTRIPE_AERO section. Here's this executed correcty:

> #addons-page {
    [...]
>   color: -moz-dialogText;
>   background-color: -moz-dialog;
>   background-image: /* Fade-out texture at the top, and blend with browser tab */
>                     -moz-linear-gradient(rgba(255,255,255,0.5), rgba(255,255,255,0) 30%),
>                     /* Texture */
>                     url("chrome://mozapps/skin/extensions/background-texture.png");
> }
> 
> %ifdef WINSTRIPE_AERO
> @media all and (-moz-windows-default-theme) {
>   #addons-page {
>     color: #000;
>     background-color: #CCD9EA;
>     background-image: /* Fade-out texture at the top */
>                       -moz-linear-gradient(top, rgb(231,237,246), rgba(231,237,246,0) 156px),
>                       /* Texture */
>                       url("chrome://mozapps/skin/extensions/background-texture.png");
>   }
> }
> @media all and (-moz-windows-compositor) {
>   #addons-page {
      [...]
>   }
> }
> %endif


Otherwise, lots of hardcoded colors in winstripe. To ensure legibility with various OS themes, I hope you hardcoded all the background and foreground colors consistently rather than mixing them with native ones.
Ideally, you'd use native colors throughout like in gnomestripe and wrap all the hardcoded stuff in a @media all and (-moz-windows-default-theme) {  } section.
Attachment #490764 - Flags: review?(dao) → review+
(In reply to comment #53)
> I hope you hardcoded all the background and foreground
> colors consistently rather than mixing them with native ones.

And by that I don't mean you explicitly using a platform color but merely omitting e.g. a text color such that winstripe/global/ styling would kick in with a color which might be black in your case but could be anything else in the wild.
Landed with changes to address Dão's comments. I tested the patch with Windows classic and high contrast themes to make sure the text was still legible.

http://hg.mozilla.org/mozilla-central/rev/367398a8cbfe
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: uiwanted
Target Milestone: --- → mozilla2.0b8
The background ( http://tinyurl.com/2upbong ) is quiet different than the screenshot ( https://bugzilla.mozilla.org/attachment.cgi?id=491016 ). Is this a bug, or just you made the screenshot with other patches applied?
(In reply to comment #56)
> The background ( http://tinyurl.com/2upbong ) is quiet different than the
> screenshot ( https://bugzilla.mozilla.org/attachment.cgi?id=491016 ). Is this a
> bug, or just you made the screenshot with other patches applied?

It should be following your windows color preferences, it looks ok for me. If not please file a new bug.
Marking as verified fixed with builds on all platforms like Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101125 Firefox/4.0b8pre ID:20101125030318
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Flags: in-litmus-
I see the same issue. :WonderCsabo have you filled a bug?(In reply to comment #56)
> The background ( http://tinyurl.com/2upbong ) is quiet different than the
> screenshot ( https://bugzilla.mozilla.org/attachment.cgi?id=491016 ). Is this a
> bug, or just you made the screenshot with other patches applied?

I see the same issue. :WonderCsabo have you filled a bug?
I didn't file one, because I still dunno this is a bug or not.

It's obvious that the screenshot made with the "hide main browser chrome" patch, maybe that effects that.

Waiting for Blair to give feedback on this.
No longer blocks: 614811
Depends on: 614811
Csaba, lets file a bug! Even when it's unclear what the situation is. I just want to make sure that it doesn't get lost. Thanks.
Ok, filed bug Bug 614824. I hope the decorations will make, it looks much better, and reminds me of shorlander's beautiful mockups of in-content UI.
Marking as verified fixed with build: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101125 Firefox/4.0b8pre ID: 20101125030318
Please ignore the last comment, it was intended for bug 614152.
No longer blocks: 604869
Depends on: 614838
No longer blocks: 614824
Depends on: 614824
Depends on: 614865
Blocks: 604869
From Bug 588764 comment #60:
> The new Addon manager magically creates a good looking border around the
> content, why?
(In reply to comment #65)
> From Bug 588764 comment #60:
> > The new Addon manager magically creates a good looking border around the
> > content, why?

See bug 614838.
Blocks: 591852
Blocks: 623199
No longer blocks: 623199
Blocks: 655588
Depends on: 1305764
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: