Closed
Bug 1260751
Opened 6 years ago
Closed 6 years ago
GCLI close button is invisible on linux
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P2)
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)
783 bytes,
patch
|
bgrins
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
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•6 years ago
|
||
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•6 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•6 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.
Comment 5•6 years ago
|
||
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 8•6 years ago
|
||
(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)
Updated•6 years ago
|
status-firefox45:
--- → unaffected
status-firefox46:
--- → unaffected
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Keywords: regression
OS: Unspecified → Linux
Version: unspecified → Trunk
Comment 9•6 years ago
|
||
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•6 years ago
|
||
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•6 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•6 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 15•6 years ago
|
||
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•6 years ago
|
||
If you want to be more conservative. Here is an alternative fix.
Comment 17•6 years ago
|
||
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•6 years ago
|
Attachment #8740415 -
Attachment is obsolete: true
Attachment #8740415 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 18•6 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/91274b84fa33
Status: NEW → RESOLVED
Closed: 6 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)
Assignee | ||
Comment 21•6 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+
Flags: qe-verify+
Comment 23•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/98197b9e27e6
Comment 24•6 years ago
|
||
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]
Comment 25•6 years ago
|
||
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.
Updated•4 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•