If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

tooltips are not themed when running against gtk+ 3.20

RESOLVED FIXED in Firefox 49

Status

()

Core
Widget: Gtk
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Hussam Al-Tayeb, Unassigned)

Tracking

(Blocks: 1 bug)

49 Branch
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed)

Details

(Whiteboard: [uplift to aurora after bug 1277818])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Comment 1

a year ago
Created attachment 8750602 [details]
Notice the "close tab" tooltip.
(Reporter)

Updated

a year ago
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
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?
Depends on: 1234158
(Reporter)

Comment 3

a year ago
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

a year ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

a year ago
According to https://developer.gnome.org/gtk3/stable/GtkWindow.html we need to set "tooltip" class to GtkWindow to fetch the .tooltip CSS style.
Blocks: 1264079
(Reporter)

Comment 6

a year ago
I tried the patch and tooltips are correctly themed now.

Comment 7

a year ago
Created attachment 8756294 [details] [diff] [review]
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.
Attachment #8756294 - Flags: review?(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.)
Attachment #8756294 - Flags: review?(karlt) → review-
(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

a year ago
> > 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.
Flags: needinfo?(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.
Flags: needinfo?(karlt)
See Also: → bug 1250704

Comment 12

a year ago
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
Attachment #8756294 - Attachment is obsolete: true
Attachment #8759148 - Flags: review?(karlt)
(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.
(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

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

Let's figure out the Bug 1275407 first.
Attachment #8759148 - Flags: review?(karlt)

Updated

a year ago
Depends on: 1277818

Comment 16

a year ago
Created attachment 8759646 [details] [diff] [review]
tooltip background patch

There's a patch based on Bug 1277818. Thanks!
Attachment #8759148 - Attachment is obsolete: true
Attachment #8759646 - Flags: review?(karlt)
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.
Created attachment 8760302 [details]
Screenshot of my issue

This is what I see after applying both this patch and the patch from bug 12777818.
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.
Attachment #8759646 - Flags: review?(karlt) → review+
(Reporter)

Comment 20

a year ago
(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

a year ago
(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.
(Reporter)

Comment 22

a year ago
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

a year ago
Created attachment 8760743 [details] [diff] [review]
patch for check-in

Thanks, there's the patch for check-in
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cae389ff876
(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.
(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.
(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.
(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/

Updated

a year ago
Keywords: checkin-needed

Updated

a year ago
Blocks: 1250704

Comment 28

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7334430aef0
Construct tooltip style from path for Gtk >= 3.20, r=karlt
Keywords: checkin-needed

Comment 29

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e887c08f20
Backed out changeset a7334430aef0 for valgrind leak

Comment 30

a year ago
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed63fbe3d85
Construct tooltip style from path for Gtk >= 3.20, r=karlt

Comment 31

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ed63fbe3d85
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Attachment #8760743 - Flags: approval-mozilla-aurora?
Whiteboard: [uplift to aurora after bug 1234158]
Summary: tooltips are not themed when compiling against gtk+ 3.20 → tooltips are not themed when running against gtk+ 3.20
Whiteboard: [uplift to aurora after bug 1234158] → [uplift to aurora after bug 1277818]
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.
Attachment #8760743 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 34

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e729f06b361f
status-firefox49: --- → fixed
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.
Never mind; I see comment 21 explains bug 1250704 still needs an uplift.
You need to log in before you can comment on or make changes to this bug.