Closed Bug 1271523 Opened 9 years ago Closed 8 years ago

tooltips are not themed when running against gtk+ 3.20

Categories

(Core :: Widget: Gtk, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: ht990332, Unassigned)

References

(Blocks 1 open bug)

Details

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

Attachments

(4 files, 2 obsolete files)

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
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
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.
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
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: gtk-3.20
I tried the patch and tooltips are correctly themed now.
Attached patch patch (obsolete) — Splinter Review
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 :/
> > 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: → 1250704
Attached patch tooltip background patch (obsolete) — Splinter Review
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 on attachment 8759148 [details] [diff] [review] tooltip background patch Let's figure out the Bug 1275407 first.
Attachment #8759148 - Flags: review?(karlt)
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.
Attached image 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+
(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.
(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.
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.
(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/
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
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/74e887c08f20 Backed out changeset a7334430aef0 for valgrind leak
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
Status: NEW → RESOLVED
Closed: 8 years ago
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+
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.

Attachment

General

Created:
Updated:
Size: