Closed Bug 1250704 Opened 8 years ago Closed 8 years ago

[GTK3] Tooltip text color wrong (elementary OS)

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 + fixed
firefox50 --- verified

People

(Reporter: freaktechnik, Assigned: karlt)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, Whiteboard: [uplift to aurora after bug 1277818 and bug 1271523])

Attachments

(4 files)

illegible_tooltip.png
(deleted), image/png
Details
Bug 1250704 - use style from WidgetStyleCache for tooltip colors.
(deleted), text/x-review-board-request
stransky
: review+
lizzard
: approval-mozilla-aurora+
Details
Bug 1250704 - use same widget heirarchy as GTK for tooltip text color.
(deleted), text/x-review-board-request
stransky
: review+
Details
bug 1250704 relax suppresion of GtkStyle set_color leak to accept change in caller
(deleted), text/x-review-board-request
n.nethercote
: review+
Details
Attached image illegible_tooltip.png (deleted) — 
Tooltips with the default theme on elementary OS 0.3 Freya have black instead of white text on a dark background. The background color (including its alpha channel) and tooltip shape seem correct.
Blocks: gtk3
Is this with GTK 3.14?
Blocks: ship-gtk3
3.14.15-0ubuntu1~14.04~ricotz1, yep.
Bug 1197165 made a change that is likely to help here.
Would you be able to test a recent Nightly, please?
Depends on: 1197165
I've tested this in Nightly 20160308030418 and the tooltips are identical to the screenshots I attached earlier.
Actually, sometimes they background isn't transparent anymore in this nightly, which is a bug.
Sorry for the spam, but that isn't actually a new bug and was already present at the point I took the screenshots (tested it on the same build), it seems the background of a tooltip gets redrawn repeatedly without clearing. Is there already a bug for that or should I open a new one?
I can reproduce the foreground color bug with GTK+ 3.16.7 and https://launchpad.net/egtk/4.x/4.0.6/+download/elementary.tar.xz

ISTR a bug report involving multiple tooltip text appearing/overlapping in the same place, but I don't have the number sorry.  Not sure whether that is what comment 6 and 7 described.  I'm not reproducing here.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Gecko gets the foreground color at
https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/widget/gtk/nsLookAndFeel.cpp#1006

which doesn't match what the theme is expecting:

.tooltip GtkLabel,
.overlay-bar GtkLabel {
    color: white;
    text-shadow: 0 1px 2px alpha (#000, 0.6);
}
(In reply to Martin Giger [:freaktechnik] from comment #7)
> it seems the background of a tooltip gets redrawn repeatedly without
> clearing. Is there already a bug for that or should I open a new one?

Bug 1258086 exists now.
GTK draws tooltip text in a GtkLabel in a GtkBox in the GtkWindow for the
tooltip.

3.20 Adwaita matches the style context using

  tooltip * {
    padding: 4px;
    background-color: transparent;
    color: white; }

I'd prefer not to keep additional style contexts for box and label children
cached because these will only be used once.  I suspect we'll need labels at
least for text colors of other widgets too, such as menuitem and menubar, and
so I think the best way to provide what nsLookAndFeel needs may be to provide
CreateGtkBoxStyle(GtkStyleContext* aParentStyle) and
CreateGtkLabelStyle(GtkStyleContext* aParentStyle) methods from
WidgetStyleCache.  These can use CreateCSSNode().
Or making CreateCSSNode() available would be another option.
I think the best is to make CreateCSSNode available and allows it to create a new style from path without parent. I'll post such patch as a separate bug.
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Comment on attachment 8760611 [details]
Bug 1250704 - use style from WidgetStyleCache for tooltip colors.

https://reviewboard.mozilla.org/r/58146/#review55080
Attachment #8760611 - Flags: review?(stransky) → review+
Comment on attachment 8760612 [details]
Bug 1250704 - use same widget heirarchy as GTK for tooltip text color.

https://reviewboard.mozilla.org/r/58148/#review55326

Looks OK to me, tested on 3.20 and pre 3.20. The floating reference is a bit unusual to me here but it's documented in the header file.

There's one missing piece - the default Gtk style Adwaita defines padding for tooltip and also for tooltip * but we use the top-level only. The Firefox tooltips are considerably smaller then - but we can address that later.
Attachment #8760612 - Flags: review?(stransky) → review+
It needs Bug 1271523 land prior this one.
Depends on: 1271523
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f125b1088b
use style from WidgetStyleCache for tooltip colors. r=stransky
https://hg.mozilla.org/integration/mozilla-inbound/rev/af36af25ecf4
use same widget heirarchy as GTK for tooltip text color. r=stransky
Keywords: checkin-needed
sorry had to back this out and the depend bug for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=29775389&repo=mozilla-inbound
Flags: needinfo?(karlt)
(In reply to Carsten Book [:Tomcat] from comment #22)
> sorry had to back this out and the depend bug for leaks like
> https://treeherder.mozilla.org/logviewer.html#?job_id=29775389&repo=mozilla-
> inbound

I wonder if these are really leaks, or if it is just we don't clean up the cache on shutdown.
128 bytes in 4 blocks are definitely lost in loss record 7,790 of 9,085
   at 0x4C293D5: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0xB33DE08: g_malloc (gmem.c:159)
   by 0xB3529CF: g_slice_alloc (gslice.c:1003)
   by 0xB352EDD: g_slice_copy (gslice.c:1051)
   by 0xB0AFCDF: boxed_proxy_lcopy_value (gboxed.c:252)
   by 0x7C86D52: gtk_style_context_get_valist (gtkstylecontext.c:1534)
   by 0x7C86F32: gtk_style_context_get (gtkstylecontext.c:1569)
   by 0x7B04227: set_color (gtkstyle.c:651)
   by 0x7B0457D: gtk_style_update_from_context (gtkstyle.c:717)
   by 0x7B045D1: gtk_style_constructed (gtkstyle.c:782)
   by 0xB0BC08E: g_object_newv (gobject.c:1747)
   by 0xB0BC65C: g_object_new_valist (gobject.c:1836)
   by 0xB0BC970: g_object_new (gobject.c:1551)
   by 0x7B010E6: _gtk_style_new_for_path (gtkstyle.c:826)
   by 0x7B01145: gtk_style_new (gtkstyle.c:853)
   by 0x7B0118C: gtk_widget_get_default_style (gtkstyle.c:4005)
   by 0x7D34CC5: gtk_widget_init (gtkwidget.c:3662)
   by 0xB0D3810: g_type_create_instance (gtype.c:1882)
   by 0xB0BC9B8: g_object_constructor (gobject.c:1855)
   by 0xB0BBEB0: g_object_newv (gobject.c:1719)
   by 0xB0BC98B: g_object_new (gobject.c:1548)
   by 0x7D44030: gtk_window_new (gtkwindow.c:1624)
   by 0xEB73D27: CreateWindowWidget() (WidgetStyleCache.cpp:30)
   by 0xEB7693D: CreateTooltipWidget (WidgetStyleCache.cpp:113)
   by 0xEB7693D: CreateWidget (WidgetStyleCache.cpp:148)
   by 0xEB7693D: GetWidget(WidgetNodeType) (WidgetStyleCache.cpp:160)
   by 0xEB776C7: GetWidgetStyleInternal (WidgetStyleCache.cpp:318)
   by 0xEB776C7: ClaimStyleContext(WidgetNodeType, GtkTextDirection, GtkStateFlags, unsigned int) (WidgetStyleCache.cpp:353)
   by 0xEB8B9A8: nsLookAndFeel::Init() (nsLookAndFeel.cpp:1142)
   by 0xEB65973: nsXPLookAndFeel::GetInstance() (nsXPLookAndFeel.cpp:264)
   by 0xEB65E46: mozilla::LookAndFeel::GetInt(mozilla::LookAndFeel::IntID, int*) (nsXPLookAndFeel.cpp:912)
   by 0xD757BFC: GetInt (LookAndFeel.h:561)
   by 0xD757BFC: nsChromeRegistryChrome::CheckForOSAccessibility() (nsChromeRegistryChrome.cpp:160)
   by 0xF18F496: ScopedXPCOMStartup::SetWindowCreator(nsINativeAppSupport*) (nsAppRunner.cpp:1620)
   by 0xF193547: XREMain::XRE_mainRun() (nsAppRunner.cpp:4121)
   by 0xF194452: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:4495)
   by 0xF1946D7: XRE_main (nsAppRunner.cpp:4603)
   by 0x405135: do_main(int, char**, char**, nsIFile*) (nsBrowserApp.cpp:254)
   by 0x40473D: main (nsBrowserApp.cpp:427)

Try testing confirms this is triggered by
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f125b1088b
which changed the callstack of the first GtkWidget creation.

good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ff010a2005a
bad: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b11d7c6ccfa6

Modifying the existing suppression takes care of that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ff2632fab0c
Flags: needinfo?(karlt)
(In reply to Martin Stránský from comment #19)
> The floating reference is a bit
> unusual to me here but it's documented in the header file.

Yes, GtkWidget's initially unowned behavior seems weird to me, like
nsISupports, but more explicit.  The fact that GTK chose this so that parent
containers can assume ownership made me feel less bad about the change from
unowned to deleted here.

> There's one missing piece - the default Gtk style Adwaita defines padding
> for tooltip and also for tooltip * but we use the top-level only. The
> Firefox tooltips are considerably smaller then - but we can address that
> later.

Oh, that's interesting, thanks.  Looks like that would match both the box and
the label, apply the padding twice, in addition to the tooltip padding.

If we should be drawing box and/or label background, and it seems that GTK
does, then I guess we need to add TOOLTIP_BOX and/or TOOLTIP_LABEL
WidgetNodeTypes.

If it were just the border/padding, then that could be calculated once and
saved instead of the style context, but using WidgetStyleCache is probably
simpler.
Review commit: https://reviewboard.mozilla.org/r/58652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58652/
Attachment #8760611 - Attachment description: bug 1250704 use style from WidgetStyleCache for tooltip colors → Bug 1250704 - use style from WidgetStyleCache for tooltip colors.
Attachment #8760612 - Attachment description: bug 1250704 use same widget heirarchy as GTK for tooltip text color → Bug 1250704 - use same widget heirarchy as GTK for tooltip text color.
Attachment #8761488 - Flags: review?(n.nethercote)
Comment on attachment 8760611 [details]
Bug 1250704 - use style from WidgetStyleCache for tooltip colors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58146/diff/1-2/
Comment on attachment 8760612 [details]
Bug 1250704 - use same widget heirarchy as GTK for tooltip text color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58148/diff/1-2/
Comment on attachment 8761488 [details]
bug 1250704 relax suppresion of GtkStyle set_color leak to accept change in caller

https://reviewboard.mozilla.org/r/58652/#review55632
Attachment #8761488 - Flags: review?(n.nethercote) → review+
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80425dc038ac
use style from WidgetStyleCache for tooltip colors. r=stransky
https://hg.mozilla.org/integration/mozilla-inbound/rev/daea8ab4c159
use same widget heirarchy as GTK for tooltip text color. r=stransky
https://hg.mozilla.org/integration/mozilla-inbound/rev/eebf02e5a399
relax suppresion of GtkStyle set_color leak to accept change in caller r=njn
https://hg.mozilla.org/mozilla-central/rev/80425dc038ac
https://hg.mozilla.org/mozilla-central/rev/daea8ab4c159
https://hg.mozilla.org/mozilla-central/rev/eebf02e5a399
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
[Tracking Requested - why for this release]:
This was a regression from the GTK3 port.  The issue has been reported for a number of themes.

Dependencies are required to land the patches here on 49.

Uplift to 48 may be possible also with possibly more dependencies.
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
tracking-firefox49: --- → ?
Keywords: regression
Hi Karl, should we uplift this to Beta48 and Aurora49? This doesn't seem severe enough to be included in 47 dot release.
status-firefox47: affected → wontfix
Flags: needinfo?(karlt)
(In reply to Ritu Kothari (:ritu) from comment #34)
> Hi Karl, should we uplift this to Beta48 and Aurora49?

I think we should uplift to Aurora49, thanks.

Uplift to Beta48 would require uplift of bug 1234158 and bug 1271477, and I expect others including bug 1248974, bug 1272194, bug 1271579, and bug 1274745.
That may be worthwhile to fix 1248974, which has those dependencies also, but there is significant risk in all that.

How does that sound to you?
If you are not scared off, then I can start on beta uplift requests.
Flags: needinfo?(karlt) → needinfo?(rkothari)
Whiteboard: [uplift to aurora after bug 1234158 and bug 1271523]
(In reply to Karl Tomlinson (ni?:karlt) from comment #35)
> (In reply to Ritu Kothari (:ritu) from comment #34)
> > Hi Karl, should we uplift this to Beta48 and Aurora49?
> 
> I think we should uplift to Aurora49, thanks.
> 
> Uplift to Beta48 would require uplift of bug 1234158 and bug 1271477, and I
> expect others including bug 1248974, bug 1272194, bug 1271579, and bug
> 1274745.
> That may be worthwhile to fix 1248974, which has those dependencies also,
> but there is significant risk in all that.
> 
> How does that sound to you?
> If you are not scared off, then I can start on beta uplift requests.
NI Sylvestre and Gerry. I think we are getting close to mid-Beta48 and not sure if we want to uplift patches from 7 dependent bugs to fix this in 48. Perhaps it could wait for 49. Sylvestre owns 48 and will make a final decision.
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(gchang)
Not interested for 48 as we already shipped 47 with it and we would like to avoid regressions.
thanks!
status-firefox48: affected → wontfix
Flags: needinfo?(sledru)
Flags: needinfo?(gchang)
Whiteboard: [uplift to aurora after bug 1234158 and bug 1271523] → [uplift to aurora after bug 1277818 and bug 1271523]
Comment on attachment 8760611 [details]
Bug 1250704 - use style from WidgetStyleCache for tooltip colors.

Approval Request Comment

Please consider this a request for all patches for this bug.

[Feature/regressing bug #]: GTK3 port
[User impact if declined]:
incorrect tooltip text color, which is unreadable with some themes.
[Describe test coverage new/current, TreeHerder]:
manual testing only.
[Risks and why]: 
There is a chance of unexpected consequences, but this has been on m-c since 2016-06-10.
The only unexpected issue reported was a small increase in resource usage in a synthetic test that previously didn't use tooltips at all.  There is the option of uplifting bug 1280951 to address that, but in real usage that wasn't really a regression because previous code would have consumed those resources on first use of tooltips anyway.
[String/UUID change made/needed]:
none.
Attachment #8760611 - Flags: approval-mozilla-aurora?
Tracking 49+ for this visible tooltip issue.
tracking-firefox49: ? → +
Blocks: 1268464
Comment on attachment 8760611 [details]
Bug 1250704 - use style from WidgetStyleCache for tooltip colors.

More GTK tooltip cleanup, ok to uplift to aurora.
Attachment #8760611 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hello! I tried to reproduce and to verify this issue on Nightly (2016-03-08), respectively latest Nightly 50.0a1 (2016-06-29), using the conditions mentioned in comment 8, but https://launchpad.net/egtk/4.x/4.0.6/+download/elementary.tar.xz can't be installed. Firefox throw this error "This add-on cannot be installed because it appears to be corrupt". Can you provide a proper file?
Flags: needinfo?(martin)
It is a GTK theme, not a Firefox theme. So it has to be installed to your system. Elementary OS ships with it by default for example, I don't know if there are other easy ways to use this GTK theme.

This issue is about colors that are given by the system not getting correctly applied by Firefox, which is why you need to use certain system themes.
Flags: needinfo?(martin)
(In reply to Martin Giger [:freaktechnik] from comment #43)
> It is a GTK theme, not a Firefox theme. So it has to be installed to your
> system. Elementary OS ships with it by default for example, I don't know if
> there are other easy ways to use this GTK theme.
> 
> This issue is about colors that are given by the system not getting
> correctly applied by Firefox, which is why you need to use certain system
> themes.

Thanks for your clarification! Unfortunately, we do not have available this Linux distribution. Considering these, can you please try to verify this bug?
Yes, this is fixed for me in the latest nightly (2016-06-30), and has been for quite a while in nightly.
Status: RESOLVED → VERIFIED
(In reply to Martin Giger [:freaktechnik] from comment #45)
> Yes, this is fixed for me in the latest nightly (2016-06-30), and has been
> for quite a while in nightly.

Can you please clarify which FF version you were referring to? I ask because Iulia Cristescu is referring to firefox50 and I am using FF 48.0b10. So, your comment is unclear.
(In reply to Ineuw from comment #46)
> Can you please clarify which FF version you were referring to? I ask because
> Iulia Cristescu is referring to firefox50 and I am using FF 48.0b10. So,
> your comment is unclear.

IT is not, as the date clearly identifies the version of Firefox I was using (which was version 50 at that point, and as the flags in the bug indicate the bug is fixed from 49 on and not earlier).
(In reply to Martin Giger [:freaktechnik] from comment #47)
> (In reply to Ineuw from comment #46)
> > Can you please clarify which FF version you were referring to? I ask because
> > Iulia Cristescu is referring to firefox50 and I am using FF 48.0b10. So,
> > your comment is unclear.
> 
> IT is not, as the date clearly identifies the version of Firefox I was using
> (which was version 50 at that point, and as the flags in the bug indicate
> the bug is fixed from 49 on and not earlier).

Thanks Martin, will patiently wait for the release.
Not fixed for me in 49. Tooltip background is corresponding to tooltip background in KDE application appearance settings, when tooltip text is corresponding to window text in settings (instead of tooltip text).
(In reply to etendren from comment #49)
> Not fixed for me in 49. Tooltip background is corresponding to tooltip
> background in KDE application appearance settings, when tooltip text is
> corresponding to window text in settings (instead of tooltip text).

Can you please clarify if this issue is running the Mozilla official released version, or the version provided by your Linux distributor?
(In reply to Bill Gianopoulos [:WG9s] from comment #50)
> (In reply to etendren from comment #49)
> > Not fixed for me in 49. Tooltip background is corresponding to tooltip
> > background in KDE application appearance settings, when tooltip text is
> > corresponding to window text in settings (instead of tooltip text).
> 
> Can you please clarify if this issue is running the Mozilla official
> released version, or the version provided by your Linux distributor?

official, downloaded from mozilla site.
(In reply to Vladimir from comment #51)
> (In reply to Bill Gianopoulos [:WG9s] from comment #50)
> > (In reply to etendren from comment #49)
> > > Not fixed for me in 49. Tooltip background is corresponding to tooltip
> > > background in KDE application appearance settings, when tooltip text is
> > > corresponding to window text in settings (instead of tooltip text).
> > 
> > Can you please clarify if this issue is running the Mozilla official
> > released version, or the version provided by your Linux distributor?
> 
> official, downloaded from mozilla site.

OK great! so it would seem this is a legitimate issue.  SO You should file a new bug about your KDE issue explaining what KDE theme you are using and showing screenshots illustrating what the issue is.  Although the subject of this bug seems very generic, it really was a bug about GNome 3 with the default adwaita theme and has been fixed.  We need a new bug on your KDE issue that blocks bug 1264079.
Using FF beta 50.0b2 on a 32bit Xubuntu 16.04 desktop, there is no change in the balloon display. It's still is black on black. On the other hand, my laptop with the identical but 64bit software, the balloons are white characters on a black background.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: