Closed Bug 1260751 Opened 4 years ago Closed 4 years ago

GCLI close button is invisible on linux

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P2)

Unspecified
Linux
defect

Tracking

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

VERIFIED FIXED
Firefox 48
Tracking Status
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 + verified
firefox48 --- verified
firefox49 --- verified

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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?
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Does it happen in both the dark and light theme?

Yes.
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
No longer depends on: 1257348
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
(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)
(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.
Ok, let's fix that.
Assignee: nobody → poirot.alex
Tangentially related: bug 1260579
Priority: -- → P2
Attached patch remove background (obsolete) — Splinter Review
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)
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+
Attachment #8740415 - Attachment is obsolete: true
Attachment #8740415 - Flags: review?(bgrinstead)
https://hg.mozilla.org/integration/fx-team/rev/91274b84fa33cbe95c3a4f4951f77dee891cd8b6
Bug 1260751 - Make Developer Toolbar close button visible again on Linux. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/91274b84fa33
Status: NEW → RESOLVED
Closed: 4 years ago
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)
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+
Flags: qe-verify+
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
Flags: qe-verify+
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.