1260751 - GCLI close button is invisible on linux

Status: VERIFIED FIXED

Description

[Alexandre Poirot [:ochameau]]

Bug 1257348 broke the close button on linux.
It still closes the toolbar, but it is invisible no matter if you hover it or not.

Comment 1

[Brian Grinstead [:bgrins]]

It's using the toolkit .close-icon class: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#311 vs https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#326.

Comment 2

[Brian Grinstead [:bgrins]]

I wonder if this filter is inappropriate: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/commandline.inc.css#64.  Does it happen in both the dark and light theme?

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Does it happen in both the dark and light theme?

Yes.

Comment 4

[Alexandre Poirot [:ochameau]]

http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/commandline.inc.css#40
takes the lead over:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#311

If I remove #developer-toolbar > toolbarbutton from commandline.inc.css, it is visible, but only in light theme. It becomes somewhat visible in dark with may be there is an issue with the filter thing you linked.

Comment 5

[Brian Grinstead [:bgrins]]

Yeah I wonder if we could remove background: transparent from that rule.  If there's a different button that actually needs that we could override it separately.

Comment 7

[Brian Grinstead [:bgrins]]

Copying flags from Bug 1263683

Comment 8

[Brian Grinstead [:bgrins]]

(copied from https://bugzilla.mozilla.org/show_bug.cgi?id=1263683#c4)

[Tracking Requested - why for this release]: Very recent regression in nightly & now aurora (via an uplift of the regressing patch)

Comment 9

[Daniel Holbert [:dholbert]]

Comment 4 is exactly the problem. The first rule there (the one with "background: transparent") wins out, because its selector is more specific.

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Yeah I wonder if we could remove background: transparent from that rule.

Probably better would be to use a just-as-specific selector for the later rule -- change it from
 .close-icon {...}
to:
 #developer-toolbar > .close-icon { ... }

Then it would be just as specific as the "transparent" rule and it would be allowed to win.

Comment 10

[Daniel Holbert [:dholbert]]

It looks like the only reason this works on Windows is that we use "list-style-image" instead of "background" to draw the close-button image there:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css?rev=ac6f2fb6777e&mark=327-327#324

...as compared to Linux where we use background-image (which gets nerfed by a more-specific "background:transparent" rule as noted above):
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css?rev=341484e03a4a&mark=315-315#309

Comment 11

[Philip Chee]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> It looks like the only reason this works on Windows is that we use
> "list-style-image" instead of "background" to draw the close-button image
> there:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> global.css?rev=ac6f2fb6777e&mark=327-327#324
> 
> ...as compared to Linux where we use background-image (which gets nerfed by
> a more-specific "background:transparent" rule as noted above):
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> global.css?rev=341484e03a4a&mark=315-315#309

I'm sorry but I don't have a linux environment to test. Would changing the linux version to use list-style-image fix the problem?

Phil

Comment 12

[:Gijs (he/him)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> Comment 4 is exactly the problem. The first rule there (the one with
> "background: transparent") wins out, because its selector is more specific.
> 
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > Yeah I wonder if we could remove background: transparent from that rule.
> 
> Probably better would be to use a just-as-specific selector for the later
> rule -- change it from
>  .close-icon {...}
> to:
>  #developer-toolbar > .close-icon { ... }
> 
> Then it would be just as specific as the "transparent" rule and it would be
> allowed to win.

The .close-icon rule is meant to be global (the clue is in the filename). It shouldn't be changed to be specific to the #developer-toolbar. It's also used (among others) for notification bars, tab close icons, ...

http://mxr.mozilla.org/mozilla-central/source/devtools/client/themes/commandline.inc.css#40 is just wrong and needs to be fixed. Could be as simple as just using background-color instead of background:. It's not really clear from bug 873848, which introduced it, why it's necessary at all, so that seems like the safest thing to do.

Comment 13

[Alexandre Poirot [:ochameau]]

Ok, let's fix that.

Comment 14

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

Tangentially related: bug 1260579

Comment 15

[Alexandre Poirot [:ochameau]]

Created attachment 8740415 [details] [diff] [review]
remove background

That simply removes the background rule, as suggested by Gijs.
It seems useless on Linux and Windows. I haven't tested on Mac.

Comment 16

[Alexandre Poirot [:ochameau]]

Created attachment 8740416 [details] [diff] [review]
replace background by background-color

If you want to be more conservative. Here is an alternative fix.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8740416 [details] [diff] [review]
replace background by background-color

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

Works for me.  Please update the commit message to indicate it's a fix for linux

Comment 18

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/91274b84fa33cbe95c3a4f4951f77dee891cd8b6
Bug 1260751 - Make Developer Toolbar close button visible again on Linux. r=bgrins

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91274b84fa33

Comment 20

[Ritu Kothari (:ritu)]

Alexandre, this seems like a recent Fx47 regression. Do you think we should uplift this to Beta47? If so, please nominate for uplift. Thanks!

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8740416 [details] [diff] [review]
replace background by background-color

Approval Request Comment
[Feature/regressing bug #]: Regressed by bug 1257348
[User impact if declined]: gcli close button invisible on linux
[Describe test coverage new/current, TreeHerder]: in nightly and aurora
[Risks and why]: naive css change, only impact gcli
[String/UUID change made/needed]: none

Comment 22

[Ritu Kothari (:ritu)]

Comment on attachment 8740416 [details] [diff] [review]
replace background by background-color

Recent regression in Fx47, Beta47+

Comment 23

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-beta/rev/98197b9e27e6

Comment 24

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce this bug on Nightly 48.0a1 (2016-03-30) (Build ID: 20160330030326) on Linux, 64 Bit.

This Bug is now verified as fixed on Latest Firefox Beta 47.0b3 (Build ID: 20160505125249)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0

and also Latest Firefox Developer Edition 48.0a2 (2016-05-09) ; (Build ID: 20160509004024)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
OS: Linux 3.19.0-58-generic x86-64

Comment 25

[Mihai Boldan, QA [:mboldan]]

I managed to reproduce this issue on Firefox 48.0a1 (2016-04-11) and on Ubuntu 14.04 x64.
The issue is no longer reproducible on Firefox 49.0a1 (2016-05-18), Firefox 48.0a2 (2016-05-18) and Firefox 47.0b6.
The tests were performed on Ubuntu 14.04 x64.
I am marking this issue Verified Fixed.