1075415 - Bookmarks chevron does not get inverted with dark lightweight themes

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Brian Grinstead [:bgrins]]

Attachment: 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)

Comment 1

[:Gijs (out for now; he/him)]

This is a problem on Windows and OS X, but not on Linux because we use default button styling in the bookmarks toolbar...

Comment 2

[Brian Grinstead [:bgrins]]

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/[e-mail]

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

Comment 3

[Brian Grinstead [:bgrins]]

Stephen, can you please provide an inverted version of these icons?

1) https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/toolbar/chevron.gif
2) https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/icons/chevron.png
3) https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/icons/[e-mail]

Comment 4

[:Gijs (out for now; he/him)]

(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/[e-mail]
> 
> 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

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: chevron-inverted.zip

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: chevron-inverted.patch

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8506575 [details] [diff] [review]
chevron-inverted.patch

Nevermind, this is not building in windows and linux because of a typo

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: 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?

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: screenshot.png

A screenshot with the inverted chevron icon using a dark lw theme

Comment 10

[:Gijs (out for now; he/him)]

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...)

Comment 11

[Brian Grinstead [:bgrins]]

https://tbpl.mozilla.org/?tree=Try&rev=f1e7f0f283e4

Comment 12

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/96c58bbd5dbb

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96c58bbd5dbb

Comment 14

[Brian Grinstead [:bgrins]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9046bff55bf9

Comment 16

[Cornel Ionce [:noni] [Hubs QA]]

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