Last Comment Bug 678761 - Add-on selection UI footer won't look good on non-aero themes
: Add-on selection UI footer won't look good on non-aero themes
Status: VERIFIED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All Windows XP
: -- normal (vote)
: mozilla9
Assigned To: Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
:
Mentors:
Depends on:
Blocks: 596343
  Show dependency treegraph
 
Reported: 2011-08-13 14:26 PDT by Dave Townsend [:mossop]
Modified: 2011-11-03 06:22 PDT (History)
6 users (show)
bmcbride: in‑testsuite-
bmcbride: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch v1 (1.14 KB, patch)
2011-08-15 19:37 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details | Diff | Splinter Review
[checked-in] Patch v2 (3.55 KB, patch)
2011-08-22 19:56 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
dao+bmo: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Windows 7 Aero Basic (35.18 KB, image/png)
2011-08-22 20:08 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details
Windows 7 Classic (7.32 KB, image/png)
2011-08-22 20:09 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details
Windows XP (28.37 KB, image/png)
2011-08-22 20:10 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
no flags Details

Description Dave Townsend [:mossop] 2011-08-13 14:26:00 PDT
From bug 596343 comment 173:

> >+++ b/toolkit/themes/winstripe/mozapps/extensions/selectAddons.css
> >+#footer {
> >+  padding: 15px 12px;
> >+  background-color: #f1f5fb;
> >+  box-shadow: 0px 1px 2px rgb(204,214,234) inset;
> >+}
> >+
> >+.progress-label,
> >+#footer-label {
> >+  font-style: italic;
> >+  color: GrayText;
> >+}
> 
> This is entirely broken. The footer styling won't fit non-aero themes such
> as any theme on Windows XP and the label is guaranteed to be invisible with 
> dark (e.g. high-contrast) themes, where GrayText isn't gray. Please file a
> bug on fixing this?
Comment 1 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-08-15 19:37:15 PDT
Created attachment 553349 [details] [diff] [review]
Patch v1

Turned out to be not a huge issue. Footer looked a little out of place, but not terribly so. And things were still actually readable on high-contrast themes, although I bet some dark themes made it impossible to read. Still, some tweaks seem to help make it look better.
Comment 2 Dão Gottwald [:dao] 2011-08-16 01:13:25 PDT
(In reply to Blair McBride (:Unfocused) from comment #1)
> And things were still actually readable on high-contrast themes,

For you or for those people who would select a high-contrast theme? ;-)
Comment 3 Dão Gottwald [:dao] 2011-08-16 01:16:23 PDT
Comment on attachment 553349 [details] [diff] [review]
Patch v1

> #footer {
>   padding: 15px 12px;
>-  background-color: #f1f5fb;
>-  box-shadow: 0px 1px 2px rgb(204,214,234) inset;
> }

What background color will this fall back to? White (e.g. window) or gray (e.g. -moz-dialog)?

> .progress-label,
> #footer-label {
>   font-style: italic;
>-  color: GrayText;
> }

Why are you removing this? GrayText is okay, as long as you're not mixing it with a hardcoded background.

>+@media all and (-moz-windows-compositor) {
>+  #footer {
>+    background-color: #f1f5fb;
>+    box-shadow: 0px 1px 2px rgb(204,214,234) inset;
>+  }
>+
>+  .progress-label,
>+  #footer-label {
>+    color: #6D6D6D;
>+  }
>+}

Are you sure you want -moz-windows-compositor? What's the expected look for aero basic?
Also, could you add selectAddons-aero.css?
Comment 4 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-08-22 19:53:12 PDT
(In reply to Dão Gottwald [:dao] from comment #3)
> What background color will this fall back to? White (e.g. window) or gray
> (e.g. -moz-dialog)?

-moz-dialog (ie, gray), so it still looks like a footer. The background grey and GrayText are different enough for the text to be readable, although I'm on the fence over whether the text should just be black to help with legibility (which was why I originally removed GrayText in the first patch).

> Are you sure you want -moz-windows-compositor? What's the expected look for
> aero basic?

I had a closer look at some of the windows/dialogs in Window 7 under Aero Basic, and they use the blue footer style. So made Aero Basic here get the styled footer too.
Comment 5 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-08-22 19:56:19 PDT
Created attachment 555017 [details] [diff] [review]
[checked-in] Patch v2
Comment 6 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-08-22 20:08:48 PDT
Created attachment 555018 [details]
Windows 7 Aero Basic
Comment 7 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-08-22 20:09:39 PDT
Created attachment 555019 [details]
Windows 7 Classic
Comment 8 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-08-22 20:10:31 PDT
Created attachment 555020 [details]
Windows XP
Comment 9 Dão Gottwald [:dao] 2011-08-22 23:41:39 PDT
Comment on attachment 555017 [details] [diff] [review]
[checked-in] Patch v2

> .progress-label,
> #footer-label {
>   font-style: italic;
>   color: GrayText;
> }

I think you should add %ifdef WINSTRIPE_AERO around font-style:italic, as this seems non-native on XP.
Comment 10 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-08-24 23:00:06 PDT
Done.

http://hg.mozilla.org/integration/fx-team/rev/cd8f12dc6dce
Comment 11 Rob Campbell [:rc] (:robcee) 2011-08-25 11:45:29 PDT
Comment on attachment 555017 [details] [diff] [review]
[checked-in] Patch v2

http://hg.mozilla.org/mozilla-central/rev/cd8f12dc6dce

Note You need to log in before you can comment on or make changes to this bug.