Closed Bug 984170 Opened 6 years ago Closed 6 years ago

Bookmarks have incorrect text colour on lightweight themes

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 + verified
firefox30 + verified
firefox31 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Keywords: regression, Whiteboard: [Australis:P3])

Attachments

(3 files, 3 obsolete files)

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
Attached patch Option A1 - Inherit color (obsolete) — Splinter Review
Attachment #8391955 - Flags: ui-review?(shorlander)
I can barely see the difference
Attachment #8391955 - Attachment description: Option A - Inherit color → Option A1 - Inherit color
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.
Attachment #8391959 - Flags: ui-review?(shorlander)
This is more risky and I'd need to test it more
Attachment #8391955 - Flags: review+
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.
Attachment #8391959 - Flags: review+
Duplicate of this bug: 984439
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
Attachment #8391959 - Flags: ui-review?(shorlander) → ui-review+
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
Attachment #8391959 - Flags: checkin+
Attachment #8391959 - Flags: approval-mozilla-aurora?
Attachment #8391955 - Attachment is obsolete: true
Attachment #8391955 - Flags: ui-review?(shorlander)
Attachment #8391969 - Attachment is obsolete: true
Attachment #8391957 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1e326e9bcc1b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
I will request beta approval after baking on Nightly then Aurora for some time
Comment on attachment 8391959 [details] [diff] [review]
Option A2 - Inherit color and font-weight

[Beta Approval Request Comment]
See comment 9
Attachment #8391959 - Flags: approval-mozilla-beta?
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?
Attachment #8391959 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(MattN+bmo)
Duplicate of this bug: 986830
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).
Flags: needinfo?(MattN+bmo)
Attachment #8391959 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Duplicate of this bug: 988094
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;
}
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.