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

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Theme
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug, {access, regression})

Trunk
Firefox 51
access, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50- fixed, firefox51 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8778639 [details]
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8778640 [details] [diff] [review]
patch

color: inherit; is the actual fix, the rest is just drive-by cleanup
Attachment #8778640 - Flags: review?(paolo.mozmail)

Comment 2

2 years ago
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-
(Assignee)

Comment 3

2 years ago
(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?
(Assignee)

Comment 4

2 years ago
Created attachment 8778857 [details]
patch v2
Attachment #8778640 - Attachment is obsolete: true
Attachment #8778857 - Flags: review?(paolo.mozmail)

Comment 5

2 years ago
(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.
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected

Updated

2 years ago
Attachment #8778857 - Flags: review?(paolo.mozmail)

Comment 6

2 years ago
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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2534a4bab86
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Comment 8

2 years ago
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]
(Assignee)

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
(Assignee)

Comment 9

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

Paolo, can you take care of uplifting this too?
status-firefox50: unaffected → affected
tracking-firefox50: --- → ?
Flags: needinfo?(paolo.mozmail)

Comment 10

2 years ago
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)

Updated

2 years ago
Attachment #8778857 - Attachment is obsolete: true

Updated

2 years ago
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 11

2 years ago
Created attachment 8784327 [details] [diff] [review]
patch v3

This is just a copy of https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/d2534a4bab86 ;)
Flags: needinfo?(dao+bmo)

Comment 12

2 years ago
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.
tracking-firefox50: ? → -
Comment on attachment 8784327 [details] [diff] [review]
patch v3

Recent regression, low risk fix, Aurora50+
Attachment #8784327 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b7940941f45
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.