GCLI close button is invisible on linux

VERIFIED FIXED in Firefox 47

Status

DevTools
Graphic Commandline and Toolbar
P2
normal
VERIFIED FIXED
2 years ago
a month ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

({regression})

Trunk
Firefox 48
Unspecified
Linux
regression

Firefox Tracking Flags

(firefox45 unaffected, firefox46 unaffected, firefox47+ verified, firefox48 verified, firefox49 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
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?
(Assignee)

Comment 3

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

Yes.
(Assignee)

Comment 4

2 years ago
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.
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.
Duplicate of this bug: 1263683
Copying flags from Bug 1263683
Flags: needinfo?(philip.chee)
(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)
Blocks: 1257348
tracking-firefox47: --- → ?
No longer depends on: 1257348
status-firefox45: --- → unaffected
status-firefox46: --- → unaffected
status-firefox47: --- → affected
status-firefox48: --- → affected
Keywords: regression
OS: Unspecified → Linux
Version: unspecified → Trunk
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.
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

2 years ago
(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
Flags: needinfo?(philip.chee)

Comment 12

2 years ago
(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.
(Assignee)

Comment 13

2 years ago
Ok, let's fix that.
Assignee: nobody → poirot.alex
Tangentially related: bug 1260579
Priority: -- → P2
(Assignee)

Comment 15

2 years ago
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.
Attachment #8740415 - Flags: review?(bgrinstead)
(Assignee)

Comment 16

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

If you want to be more conservative. Here is an alternative fix.
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
Attachment #8740416 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8740415 - Attachment is obsolete: true
Attachment #8740415 - Flags: review?(bgrinstead)
(Assignee)

Comment 18

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

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91274b84fa33
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Alexandre, this seems like a recent Fx47 regression. Do you think we should uplift this to Beta47? If so, please nominate for uplift. Thanks!
Flags: needinfo?(poirot.alex)

Updated

2 years ago
tracking-firefox47: ? → +
(Assignee)

Comment 21

2 years ago
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
Flags: needinfo?(poirot.alex)
Attachment #8740416 - Flags: approval-mozilla-beta?
Comment on attachment 8740416 [details] [diff] [review]
replace background by background-color

Recent regression in Fx47, Beta47+
Attachment #8740416 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

2 years ago
Flags: qe-verify+

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/98197b9e27e6
status-firefox47: affected → fixed
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
QA Whiteboard: [testday-20160506]
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.
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
status-firefox48: fixed → verified
status-firefox49: --- → verified
Flags: qe-verify+

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.