Closed Bug 1293023 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 - fixed
firefox51 --- verified

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: access, regression)

Attachments

(2 files, 2 obsolete files)

Attached image 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.
Attached patch patch (obsolete) — Splinter Review
color: inherit; is the actual fix, the rest is just drive-by cleanup
Attachment #8778640 - Flags: review?(paolo.mozmail)
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).
Attachment #8778640 - Flags: review?(paolo.mozmail) → review-
(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?
Attached file patch v2 (obsolete) —
Attachment #8778640 - Attachment is obsolete: true
Attachment #8778857 - Flags: review?(paolo.mozmail)
(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.
Attachment #8778857 - Flags: review?(paolo.mozmail)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2534a4bab86
Don't hardcode black as the text color in the downloads panel footer. r=paolo
https://hg.mozilla.org/mozilla-central/rev/d2534a4bab86
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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]
Status: RESOLVED → VERIFIED
[Tracking Requested - why for this release]: bug 1001324 has been uplifted to aurora now.

Paolo, can you take care of uplifting this too?
Flags: needinfo?(paolo.mozmail)
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.
Attachment #8778857 - Attachment is patch: false
Flags: needinfo?(paolo.mozmail)
Attachment #8778857 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
Attached patch patch v3Splinter Review
This is just a copy of https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/d2534a4bab86 ;)
Flags: needinfo?(dao+bmo)
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
Attachment #8784327 - Flags: approval-mozilla-aurora?
No need to track it since the fix is ready and will land soon on Aurora50.
Comment on attachment 8784327 [details] [diff] [review]
patch v3

Recent regression, low risk fix, Aurora50+
Attachment #8784327 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.