1271523 - tooltips are not themed when running against gtk+ 3.20

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Hussam Al-Tayeb]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160509214821

Steps to reproduce:

I just did a new build against gtk+ 3.20.3 and the tooltips are not themed.
I will attach a screenshot

Comment 1

[Hussam Al-Tayeb]

Attachment: Notice the "close tab" tooltip.

Comment 2

[Karl Tomlinson (:karlt)]

Do you have reason to believe that the trigger is compiling against 3.20, rather than running against 3.20?

Have you tried running a Mozilla build against 3.20?

What GTK theme is that?

Comment 3

[Hussam Al-Tayeb]

The theme is Adwaita.

No, I cannot know for sure the reason is gtk+ 3.20 and I have not had a lower version in some time. I was following the 3.19.xx development series and now I have 3.20.4 installed.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

I've seen this bug on nightly/Fedora 24/gtk3.20. So it's reproductible with standard mozilla build without any gtk3.20 patches. IMHO we need to fix that by CSS nodes for gtk3.20.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

According to https://developer.gnome.org/gtk3/stable/GtkWindow.html we need to set "tooltip" class to GtkWindow to fetch the .tooltip CSS style.

Comment 6

[Hussam Al-Tayeb]

I tried the patch and tooltips are correctly themed now.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Implementation of tooltips for Gtk3.20. They're not rendered correctly because the GtkWindow does not pick up the css tooltip clas even when it's added. 

We need to construct the style from path with correct style + type. I created GetBaseNodeStyle() because it's IMHO the simplest way. We may rewrite that if more classes need this approach.

It's also needed to distinguish between background (window) and foreground styles. I added the MOZ_GTK_TOOLTIP_WINDOW for the background style. 

Gtk3.20 also uses border/padding for the tooltips, I guess it's no harm to use for gtk < 3.20 too.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8756294 [details] [diff] [review]
patch

In reply to Martin Stránský from comment #6)
> Implementation of tooltips for Gtk3.20. They're not rendered correctly
> because the GtkWindow does not pick up the css tooltip clas even when it's
> added.

This is because GTK 3.20 changed to a "tooltip" node name, instead of the
class name.

> We need to construct the style from path with correct style + type. I
> created GetBaseNodeStyle() because it's IMHO the simplest way. We may
> rewrite that if more classes need this approach.

Yes, I agree that constructing a style context is going to be simpler than
deriving a new class from GTK_TYPE_WINDOW and using
gtk_widget_class_set_css_name().

> It's also needed to distinguish between background (window) and foreground
> styles. I added the MOZ_GTK_TOOLTIP_WINDOW for the background style. 

Yes, the foreground color needs a different style context, but GTK does not
remove GTK_STYLE_CLASS_BACKGROUND from the window style for the text color.
This is bug 1250704 which exists even with pre-3.20 GTK.  See comments there.

I would be happy for the foreground color to be addressed separately in bug
1250704, but we won't need the MOZ_GTK_TOOLTIP(|_WINDOW) distinction, so I
don't want to add it temporarily.

> Gtk3.20 also uses border/padding for the tooltips, I guess it's no harm
> to use for gtk < 3.20 too.

Sounds reasonable.

>-  GtkWidgetPath* path =
>-    gtk_widget_path_copy(gtk_style_context_get_path(aParentStyle));
>+  GtkWidgetPath* path = (aParentStyle) ?
>+    gtk_widget_path_copy(gtk_style_context_get_path(aParentStyle)) :
>+    gtk_widget_path_new();
> 
>-  gtk_widget_path_append_type(path, G_TYPE_NONE);
>+  gtk_widget_path_append_type(path, aType);

These changes look good, thanks, but please remove the unnecessary () in
"(aParentStyle) ?"

>+    GtkStyleContext* style = ClaimStyleContext(MOZ_GTK_TOOLTIP_WINDOW, direction);
>     gtk_render_background(style, cr, rect->x, rect->y, rect->width, rect->height);
>     gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);
>     return MOZ_GTK_SUCCESS;
> }

Currently debug assertions require a style context release here and similarly below.

(I'm open to alternative suggestions to avoid this when unnecessary, such as
having a method with a different name [perhaps just "StyleContext()"] for
styles that we know never need a release.  However, I'd like assertions that the methods are used correctly to be the same for pre and post 3.20 where practical.)

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #5)
> According to https://developer.gnome.org/gtk3/stable/GtkWindow.html we need
> to set "tooltip" class to GtkWindow to fetch the .tooltip CSS style.

That documentation is out of date already :/

Comment 10

[Martin Stránský [:stransky] (ni? me)]

> > It's also needed to distinguish between background (window) and foreground
> > styles. I added the MOZ_GTK_TOOLTIP_WINDOW for the background style. 
> 
> Yes, the foreground color needs a different style context, but GTK does not
> remove GTK_STYLE_CLASS_BACKGROUND from the window style for the text color.
> This is bug 1250704 which exists even with pre-3.20 GTK.  See comments there.
> 
> I would be happy for the foreground color to be addressed separately in bug
> 1250704, but we won't need the MOZ_GTK_TOOLTIP(|_WINDOW) distinction, so I
> don't want to add it temporarily.

Yes, I was thinking about it but - we can use two contexts (foreground/background - the name does not matter) or add/remove GTK_STYLE_CLASS_BACKGROUND from main context. But the latter needs also context save/restore (at least in pre 3.20).

So do you object the new WidgetNodeType entry (and store the _BACKGROUND style differently)? And do you mind the context save/restore? I'd rather remove the save/restore from all the code, IMHO it's cleaner to have more contexts than keep different states.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #10)
> > > It's also needed to distinguish between background (window) and foreground
> > > styles. I added the MOZ_GTK_TOOLTIP_WINDOW for the background style. 
> > 
> > Yes, the foreground color needs a different style context, but GTK does not
> > remove GTK_STYLE_CLASS_BACKGROUND from the window style for the text color.
> > This is bug 1250704 which exists even with pre-3.20 GTK.  See comments there.
> > 
> > I would be happy for the foreground color to be addressed separately in bug
> > 1250704, but we won't need the MOZ_GTK_TOOLTIP(|_WINDOW) distinction, so I
> > don't want to add it temporarily.
> 
> Yes, I was thinking about it but - we can use two contexts
> (foreground/background - the name does not matter) or add/remove
> GTK_STYLE_CLASS_BACKGROUND from main context. But the latter needs also
> context save/restore (at least in pre 3.20).

We need only MOZ_GTK_TOOLTIP for the background style and that should have
GTK_STYLE_CLASS_BACKGROUND.  No need to add/remove.  Even 3.20 adds
CLASS_BACKGROUND in gtk_window_init().

In the patch here, the context without CLASS_BACKGROUND is used only for text
color, but that is not the appropriate style context for text color.  The text
color needs a style context for a label child of a box child of the tooltip.
That style context is used only once, and so need not be cached.  It can be
created and unref'ed after use.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1250704#c12

>  So do you object the new WidgetNodeType entry

Yes.

> (and store the _BACKGROUND style differently)?

No.  MOZ_GTK_TOOLTIP should always have CLASS_BACKGROUND.  Currently it gets
that from the GtkWindow, but WidgetStyleCache can add the class to the CSS
node it creates, which could also be made to work for pre-3.20.

> And do you mind the context save/restore? I'd rather
> remove the save/restore from all the code, IMHO it's cleaner to have more
> contexts than keep different states.

There's no need to save/restore here.  I'd like to save/restore only where
GTK saves/restores.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Attachment: tooltip background patch

Thanks, there is a patch for background only. 
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de52d77aff22

Comment 13

[Bill Gianopoulos [:WG9s]]

(In reply to Martin Stránský from comment #12)
> Created attachment 8759148 [details] [diff] [review]
> tooltip background patch
> 
> Thanks, there is a patch for background only. 
> Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de52d77aff22

THhis does not really work.  Without this patch, using the Adwaita theme, the foreground is black and the background is light gray.  The corect display is white foreground and black background.  By applying the theme correctly on the background only you end up with both the foreground and background being black.  This makes the tooltips on the history sidebar, as an example of the issue, display as solid back rectangles.

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #13)
> (In reply to Martin Stránský from comment #12)
> > Created attachment 8759148 [details] [diff] [review]
> > tooltip background patch
> > 
> > Thanks, there is a patch for background only. 
> > Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de52d77aff22
> 
> THhis does not really work.  Without this patch, using the Adwaita theme,
> the foreground is black and the background is light gray.  The corect
> display is white foreground and black background.  By applying the theme
> correctly on the background only you end up with both the foreground and
> background being black.  This makes the tooltips on the history sidebar, as
> an example of the issue, display as solid back rectangles.

Moral of the story "You can't theme one without the other" (sounds like lyrics from an old Sammy Cahn song!; I am the master of the arcane reference)

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8759148 [details] [diff] [review]
tooltip background patch

Let's figure out the Bug 1275407 first.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Attachment: tooltip background patch

There's a patch based on Bug 1277818. Thanks!

Comment 17

[Bill Gianopoulos [:WG9s]]

T(In reply to Martin Stránský from comment #16)
> Created attachment 8759646 [details] [diff] [review]
> tooltip background patch
> 
> There's a patch based on Bug 1277818. Thanks!

This still results in illegible tooltips for me.

Comment 18

[Bill Gianopoulos [:WG9s]]

Attachment: Screenshot of my issue

This is what I see after applying both this patch and the patch from bug 12777818.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8759646 [details] [diff] [review]
tooltip background patch

This is good thanks.

Given that this happens to show up bug 1250704 with 3.20 Adwaita,
I suggest delaying landing until we have a fix for that.

>+CreateTooltipWidget()
>+{
>+  GtkWidget* widget = CreateWindowWidget();
>+  GtkStyleContext* style = gtk_widget_get_style_context(widget);

Can you assert this is only called with pre-3.20 GTK, please?

>+      /* We need to create this from path because there
>+       * is not a base widget for tooltips */

More precisely, "We create this from the path because GtkTooltipWindow is not public."  GtkTooltipWindow is the base widget.  (Perhaps we could persuade GTK to create the class and then look it up to create an object, but that wouldn't be a good idea for a private class.)

>+      style = CreateCSSNode(GTK_STYLE_CLASS_TOOLTIP, nullptr, GTK_TYPE_TOOLTIP);

Given the first argument is not a class, but a node name, I would use "tooltip".
GTK_STYLE_CLASS_TOOLTIP is OK if you prefer that.

Comment 20

[Hussam Al-Tayeb]

(In reply to Bill Gianopoulos [:WG9s] from comment #18)
> Created attachment 8760302 [details]
> Screenshot of my issue
> 
> This is what I see after applying both this patch and the patch from bug
> 12777818.

Same here. The text in the tooltips is now in black.

Comment 21

[Martin Stránský [:stransky] (ni? me)]

(In reply to Hussam Al-Tayeb from comment #20)
> (In reply to Bill Gianopoulos [:WG9s] from comment #18)
> > Created attachment 8760302 [details]
> > Screenshot of my issue
> > 
> > This is what I see after applying both this patch and the patch from bug
> > 12777818.
> 
> Same here. The text in the tooltips is now in black.

That's the correct background of the tooltip for Adwaita, if you see that the patch works correctly. I have the same on Fedora 24. The wrong text color is Bug 1250704.

Comment 22

[Hussam Al-Tayeb]

A combination of patches from bugs 1277818 1250704 1271523 1275407 is causing a crash when I right click on the firefox toolbar (above the add bookmarks icon).
(firefox:7633): Gtk-CRITICAL **: gtk_clipboard_set_with_data: assertion 'targets != NULL' failed
But I will wait till the dust settles down a bit before I file a new bug report.

Comment 23

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch for check-in

Thanks, there's the patch for check-in
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cae389ff876

Comment 24

[Bill Gianopoulos [:WG9s]]

(In reply to Hussam Al-Tayeb from comment #22)
> A combination of patches from bugs 1277818 1250704 1271523 1275407 is
> causing a crash when I right click on the firefox toolbar (above the add
> bookmarks icon).
> (firefox:7633): Gtk-CRITICAL **: gtk_clipboard_set_with_data: assertion
> 'targets != NULL' failed
> But I will wait till the dust settles down a bit before I file a new bug
> report.

I suspect this is the same crash I mentioned in bug 1277818 comment 7.  I just did not have very good steps to reproduce.

Comment 25

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #24)
> (In reply to Hussam Al-Tayeb from comment #22)
> > A combination of patches from bugs 1277818 1250704 1271523 1275407 is
> > causing a crash when I right click on the firefox toolbar (above the add
> > bookmarks icon).
> > (firefox:7633): Gtk-CRITICAL **: gtk_clipboard_set_with_data: assertion
> > 'targets != NULL' failed
> > But I will wait till the dust settles down a bit before I file a new bug
> > report.
> 
> I suspect this is the same crash I mentioned in bug 1277818 comment 7.  I
> just did not have very good steps to reproduce.

If you have trouble reproducing because it is hard to click in that blank area, if you only have 1 tab open just click on the blank part of the tabbar.

Comment 26

[Bill Gianopoulos [:WG9s]]

(In reply to Hussam Al-Tayeb from comment #22)
> A combination of patches from bugs 1277818 1250704 1271523 1275407 is
> causing a crash when I right click on the firefox toolbar (above the add
> bookmarks icon).
> (firefox:7633): Gtk-CRITICAL **: gtk_clipboard_set_with_data: assertion
> 'targets != NULL' failed
> But I will wait till the dust settles down a bit before I file a new bug
> report.

This issue definitely seems to be caused by the patch for bug 1275407.  I cannot reproduce it in a build with just the other patches.  As Martin said in Bug 1277818, we will work on this issue in bug 1275407.

Comment 27

[Bill Gianopoulos [:WG9s]]

(In reply to Hussam Al-Tayeb from comment #22)
> A combination of patches from bugs 1277818 1250704 1271523 1275407 is
> causing a crash when I right click on the firefox toolbar (above the add
> bookmarks icon).
> (firefox:7633): Gtk-CRITICAL **: gtk_clipboard_set_with_data: assertion
> 'targets != NULL' failed
> But I will wait till the dust settles down a bit before I file a new bug
> report.

BTW.  If you are interested in running nightlies with those patches (except for the scrolbar one that is causing the crashes) I am including those in my automated nightly builds available at http://www.wg9s.com/mozilla/firefox/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7334430aef0
Construct tooltip style from path for Gtk >= 3.20, r=karlt

Comment 29

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e887c08f20
Backed out changeset a7334430aef0 for valgrind leak

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed63fbe3d85
Construct tooltip style from path for Gtk >= 3.20, r=karlt

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ed63fbe3d85

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 8760743 [details] [diff] [review]
patch for check-in

Approval Request Comment
[Feature/regressing bug #]: GTK3 port
[User impact if declined]:
Bug 1250704 (incorrect tooltip text color, unreadable with some themes)
is the regressing feature that depends on this.
There is also benefit of appropriately themed tooltip backgrounds for those using the new and different GTK 3.20 platform.
[Describe test coverage new/current, TreeHerder]:
manual testing only.
[Risks and why]: 
Has been on m-c since 2016-06-10.
[String/UUID change made/needed]:
none.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8760743 [details] [diff] [review]
patch for check-in

Cleanup for gtk themes. This should land on aurora after the patch from bug 1277818.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e729f06b361f

Comment 35

[Jan Alexander Steffens [:heftig]]

Since this got uplifted to Aurora I have unreadable black-on-black tooltips (GTK 3.20, Adwaita), similar to bug 1276596. Before this change, tooltips were opaque black-on-light-gray; themed wrongly but readable.

Comment 36

[Jan Alexander Steffens [:heftig]]

Never mind; I see comment 21 explains bug 1250704 still needs an uplift.