678761 - Add-on selection UI footer won't look good on non-aero themes

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

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] (UNAVAILABLE)]

Attachment: 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]]

(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]]

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] (UNAVAILABLE)]

(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] (UNAVAILABLE)]

Attachment: [checked-in] Patch v2

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Windows 7 Aero Basic

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Windows 7 Classic

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Windows XP

Comment 9

[Dão Gottwald [:dao]]

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] (UNAVAILABLE)]

Done.

http://hg.mozilla.org/integration/fx-team/rev/cd8f12dc6dce

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 555017 [details] [diff] [review]
[checked-in] Patch v2

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

Comment 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/6604a5a31e33