984170 - Bookmarks have incorrect text colour on lightweight themes

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Black bookmark text with a black text shadow

This looks quite bad and affects anyone showing bookmark toolbar items (including if the items are in the nav-bar).

I believe this was regressed by bug 909349 because it significantly increased the specificity of a selector by adding a :not(#bookmarks-menu-button)[1] which adds 100 to the specificity.

Since the inherited colour would be black it's getting set to #222, the easy fix (A) would be to remove the color property from that ruleset. The difference is very subtle IMO and this would reduce future regressions.

Other fixes:
B) Add !important to https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css?rev=542bdf3f5191#35
C) Add a new rule for the bookmark items with LWT to fix the colour which makes this CSS more complex.

I vote for option A to simplify our styles (followed by B).

[1] http://hg.mozilla.org/mozilla-central/diff/ccbe695b5f40/browser/themes/osx/browser.css#l1.12

Comment 1

[Matthew N. [:MattN]]

Attachment: Option A1 - Inherit color

Comment 2

[Matthew N. [:MattN]]

Attachment: Stock Nightly (left) vs. Option A1 (without a LWT)

I can barely see the difference

Comment 3

[Matthew N. [:MattN]]

Attachment: Option A2 - Inherit color and font-weight

Now that tab titles aren't bold on OS X (bug 979953), perhaps bookmarks shouldn't be either?

This patch removes the color and font-weight properties.

Comment 4

[Matthew N. [:MattN]]

Attachment: Stock Nightly (left) vs. Option A2 (without a LWT)

Comment 5

[Matthew N. [:MattN]]

Attachment: Option B - Make LWT color !important

This is more risky and I'd need to test it more

Comment 6

[:Gijs (he/him)]

Comment on attachment 8391959 [details] [diff] [review]
Option A2 - Inherit color and font-weight

rs=me on either of these approaches as long as you get ui-review.

Comment 8

[Stephen Horlander [:shorlander]]

Comment on attachment 8391959 [details] [diff] [review]
Option A2 - Inherit color and font-weight

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

Option A2 works for me

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8391959 [details] [diff] [review]
Option A2 - Inherit color and font-weight

Hover screenshots:
LWT: http://grab.by/vdWq
no LWT: http://grab.by/vdWs

https://hg.mozilla.org/integration/fx-team/rev/1e326e9bcc1b

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 909349
User impact if declined: Bookmarks on a toolbar are hard to read with dark text on a dark text shadow (see attachment 8391951 [details])
Testing completed (on m-c, etc.): Locally
Risk to taking this patch (and alternatives if risky): Low risk tiny styling change
String or IDL/UUID changes made by this patch: None

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1e326e9bcc1b

Comment 11

[Matthew N. [:MattN]]

I will request beta approval after baking on Nightly then Aurora for some time

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8391959 [details] [diff] [review]
Option A2 - Inherit color and font-weight

[Beta Approval Request Comment]
See comment 9

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8391959 [details] [diff] [review]
Option A2 - Inherit color and font-weight

Approved for aurora.
How long do you want us to wait for the beta approval?

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80bf4cf39898

I think it's baked long enough on Nightly (I had expected the Aurora approval sooner).

Comment 16

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6e0307c8760a

Comment 17

[Ioana (away)]

Reproduced on Firefox 29.0b1. Verified as fixed on Firefox 29.0b2 using several of the "Most Popular" AMO themes on Mac OS X 10.8.5.

Comment 19

[Kohei Yoshino [:kohei]]

Cool. I no longer need the following lines in userChrome.css with 29.0b2:

toolbarbutton.bookmark-item:not(.subviewbutton):not(#bookmarks-menu-button),
#personal-bookmarks[cui-areatype="toolbar"]:not([overflowedItem=true]) > #bookmarks-toolbar-placeholder {
  color: inherit !important;
  font-weight: normal !important;
}

Comment 20

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on Mac OS X 10.9 [1], using:
  * the latest Beta (Build ID: 20140505140302),
  * Aurora 31 2014-05-06,
with the most popular themes.

[1] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0