Closed Bug 1075415 Opened 5 years ago Closed 5 years ago

Bookmarks chevron does not get inverted with dark lightweight themes

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- verified
firefox36 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached image bookmarks-chevron.png
STR:
Apply this lightweight theme https://addons.mozilla.org/en-US/firefox/addon/three-wolf-moon-shirt/
Show bookmarks toolbar
Shrink window so that the chevron for more bookmarks appears

Notice that this icon is not inverted, but all of the others are (see screenshot)
This is a problem on Windows and OS X, but not on Linux because we use default button styling in the bookmarks toolbar...
OS: Mac OS X → All
Summary: Chevron does not get inverted with dark lightweight themes → Bookmarks chevron does not get inverted with dark lightweight themes
Looked a bit more into this.  The element is using the the toolbarbutton.chevron selector.

In OSX [0] it is using: toolkit/themes/osx/global/icons/chevron.png and toolkit/themes/osx/global/icons/chevron@2x.png

In Linux [1] it is using chrome://global/skin/toolbar/chevron.gif, which I'm assuming is a built-in icon as per Comment 1.

In Windows [2] it is using toolkit/themes/windows/global/toolbar/chevron.gif

[0]: http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#212
[1]: http://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#1908
[2]: http://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#1975
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Looked a bit more into this.  The element is using the the
> toolbarbutton.chevron selector.
> 
> In OSX [0] it is using: toolkit/themes/osx/global/icons/chevron.png and
> toolkit/themes/osx/global/icons/chevron@2x.png
> 
> In Linux [1] it is using chrome://global/skin/toolbar/chevron.gif, which I'm
> assuming is a built-in icon as per Comment 1.

The Linux icon comes from the Windows icon, because of this construction:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/moz.build
Attached patch chevron-inverted.patch (obsolete) — Splinter Review
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8506575 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8506575 [details] [diff] [review]
chevron-inverted.patch

Nevermind, this is not building in windows and linux because of a typo
Attachment #8506575 - Flags: review?(gijskruitbosch+bugs)
Had to only use the inverted icon when there wasn't a hover happening on linux - at least on Ubuntu the native hover styling would cause the white icon to be invisible.  This wasn't an issue on Windows or OSX.  I think it looks OK to switch back to the normal icon on hover in this case, but I'm not sure if this would hold true across all linux platforms - is the native styling the same across all distros?
Attachment #8506575 - Attachment is obsolete: true
Attachment #8507461 - Flags: review?(gijskruitbosch+bugs)
Attached image screenshot.png
A screenshot with the inverted chevron icon using a dark lw theme
Comment on attachment 8507461 [details] [diff] [review]
chevron-inverted.patch

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

Apologies for the delay here, I was traveling and/or jetlagged. (OK, I'm still jetlagged, but not so badly anymore)

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Created attachment 8507461 [details] [diff] [review]
> chevron-inverted.patch
> 
> Had to only use the inverted icon when there wasn't a hover happening on
> linux - at least on Ubuntu the native hover styling would cause the white
> icon to be invisible.  This wasn't an issue on Windows or OSX.  I think it
> looks OK to switch back to the normal icon on hover in this case, but I'm
> not sure if this would hold true across all linux platforms - is the native
> styling the same across all distros?

I don't know (I suspect not, in fact), but I don't know how we can fix that; we do bright text detection but not for hover states. I think it's just a bug that the hover state swaps colors around, but fixing the hover state for all of the bookmarks toolbar here is kind of out of scope... Can you file a followup bug and fx-backlog+ it, and make it dependent on bug 734326 ?

(also, d'oh, I forgot to hit submit on this earlier, sorry...)
Attachment #8507461 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/96c58bbd5dbb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Comment on attachment 8507461 [details] [diff] [review]
chevron-inverted.patch

Approval Request Comment
[Feature/regressing bug #]: 1054353
[User impact if declined]: We need this for 1054353
[Risks and why]: It's a CSS only change for dark lightweight themes
Attachment #8507461 - Flags: approval-mozilla-aurora?
Attachment #8507461 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 12.04 32-bit using:
- Latest Nightly, build ID: 20141029030207
- Firefox Developer Edition, build ID: 20141028155607
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.