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)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: ht990332, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [uplift to aurora after bug 1277818])
Attachments
(4 files, 2 obsolete files)
226.33 KB,
image/png
|
Details | |
9.29 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
47.93 KB,
image/png
|
Details | |
9.38 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Comment 2•9 years ago
|
||
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?
Reporter | ||
Comment 3•9 years 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•9 years 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•9 years 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.
Reporter | ||
Comment 6•9 years ago
|
||
I tried the patch and tooltips are correctly themed now.
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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-
Comment 9•9 years ago
|
||
(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•9 years 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)
Comment 11•9 years ago
|
||
(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
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
Comment on attachment 8759148 [details] [diff] [review]
tooltip background patch
Let's figure out the Bug 1275407 first.
Attachment #8759148 -
Flags: review?(karlt)
Comment 16•9 years ago
|
||
There's a patch based on Bug 1277818. Thanks!
Attachment #8759148 -
Attachment is obsolete: true
Attachment #8759646 -
Flags: review?(karlt)
Comment 17•9 years ago
|
||
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•9 years ago
|
||
This is what I see after applying both this patch and the patch from bug 12777818.
Comment 19•9 years ago
|
||
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•9 years 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•9 years 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•9 years 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•9 years ago
|
||
Thanks, there's the patch for check-in
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cae389ff876
Comment 24•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
(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•8 years ago
|
Keywords: checkin-needed
Comment 28•8 years 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•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e887c08f20
Backed out changeset a7334430aef0 for valgrind leak
Comment 30•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 32•8 years ago
|
||
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?
Updated•8 years ago
|
Whiteboard: [uplift to aurora after bug 1234158]
Updated•8 years ago
|
Summary: tooltips are not themed when compiling against gtk+ 3.20 → tooltips are not themed when running against gtk+ 3.20
Updated•8 years ago
|
Whiteboard: [uplift to aurora after bug 1234158] → [uplift to aurora after bug 1277818]
Comment 33•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
status-firefox49:
--- → fixed
Comment 35•8 years ago
|
||
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•8 years ago
|
||
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.
Description
•