1293023 - Don't hardcode black as the text color in the downloads panel footer

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P3)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot on Ubuntu

This is wrong e.g. on default Ubuntu and even more wrong with dark Gtk themes and in high-contrast settings on Windows.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

color: inherit; is the actual fix, the rest is just drive-by cleanup

Comment 2

[:Paolo Amadini]

Comment on attachment 8778640 [details] [diff] [review]
patch

Review of attachment 8778640 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/downloads/downloads.inc.css
@@ +42,5 @@
>  }
>  
>  .downloadsPanelFooter > toolbarseparator {
>    margin: 0;
> +  border-style: none;

Unfortunately "border-style: none;" doesn't work, the color is still the one from the three-dimensional border. I tried "border-width: 0;" as well.

While inelegant, "border: 0;" is the same styling used in the main menu:

https://dxr.mozilla.org/mozilla-central/rev/d42aacfe34af25e2f5110e2ca3d24a210eabeb33/browser/themes/shared/customizableui/panelUI.inc.css#594

I look forward to a general solution to the whole overrides situation in bug 1288747!

@@ +51,5 @@
>  
>  .downloadsPanelFooterButton {
>    -moz-appearance: none;
>    background-color: transparent;
> +  color: inherit;

Thanks for the fix! r+ in advance for a different patch that includes only this line.

@@ +56,3 @@
>    margin: 0;
>    padding: 0;
> +  min-height: 3em;

The value of 40px is the correct one to match the height of the footer of the application menu and the new Downloads Panel visual specification in bug 1269956 (see section 2.4.1).

Comment 3

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #2)
> >  .downloadsPanelFooter > toolbarseparator {
> >    margin: 0;
> > +  border-style: none;
> 
> Unfortunately "border-style: none;" doesn't work, the color is still the one
> from the three-dimensional border. I tried "border-width: 0;" as well.

Ah, I thought you just wanted to remove the border, didn't realize you actually want to render a different border.

The problem here is that the nonstandard -moz-border-left-colors and -moz-border-right-colors properties are harder to override than they should be. Luckily they are used sparsely.

> >    margin: 0;
> >    padding: 0;
> > +  min-height: 3em;
> 
> The value of 40px is the correct one to match the height of the footer of
> the application menu

We should fix that too then.

> and the new Downloads Panel visual specification in bug 1269956 (see section 2.4.1).

Designers love pixels. It's the engineers' responsibility not to implement such a spec blindly but to apply best practices. With em the button will shrink and grow for non-default OS text size configurations so that the button still more or less looks like the designer intended. With px the button will look overly spacious or crammed in such cases. Is there any actual reason to prefer px here?

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 5

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #3)
> Designers love pixels. It's the engineers' responsibility not to implement
> such a spec blindly but to apply best practices. With em the button will
> shrink and grow for non-default OS text size configurations so that the
> button still more or less looks like the designer intended. With px the
> button will look overly spacious or crammed in such cases. Is there any
> actual reason to prefer px here?

Icon sizes are defined in pixels, so as far as I can tell it may be a visual design reason related to the relative spacing of icons, rather than spacing of text.

Can you please file a separate bug to change the height of the buttons both here and in the main menu at once, and ask a designer for feedback, maybe Stephen or Philipp? Let's only fix what the bug subject says here, so we can move forward with the actual fix without blocking on the designer feedback.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2534a4bab86
Don't hardcode black as the text color in the downloads panel footer. r=paolo

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2534a4bab86

Comment 8

[Tanvir Rahman]

I have reproduced this bug with Nightly 51.0a1(2016-08-07) on Windows 10, 64 bit!

The Bug's fix is now verified on latest Nightly 51.0a1

Build ID 	20160811030201
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160810]

Comment 9

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]: bug 1001324 has been uplifted to aurora now.

Paolo, can you take care of uplifting this too?

Comment 10

[:Paolo Amadini]

Comment on attachment 8778857 [details]
patch v2

Ah, sorry, I missed this bug when tracking the regression for 50.

If you can upload a patch that matches the code that landed, it can be nominated for uplift.

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch v3

This is just a copy of https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/d2534a4bab86 ;)

Comment 12

[:Paolo Amadini]

Comment on attachment 8784327 [details] [diff] [review]
patch v3

Approval Request Comment
[Feature/regressing bug #]: Bug 1001324
[User impact if declined]: Incorrect button text color in the Downloads Panel for non-default and high contrast themes
[Describe test coverage new/current, TreeHerder]: Landed on mozilla-central
[Risks and why]: Pretty low, one line CSS change
[String/UUID change made/needed]: None

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

No need to track it since the fix is ready and will land soon on Aurora50.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8784327 [details] [diff] [review]
patch v3

Recent regression, low risk fix, Aurora50+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b7940941f45