[GtkTooltip] Use WidgetCache to get colors at nsLookAndFeel

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Transfer GtkTooltip to the widget cache.
(Assignee)

Comment 1

2 years ago
Created attachment 8812750 [details] [diff] [review]
patch
Attachment #8812750 - Flags: review?(karlt)
Comment on attachment 8812750 [details] [diff] [review]
patch

>+    case MOZ_GTK_TOOLTIP_BOX:
>+      style = CreateChildCSSNode("box",
>+                                 MOZ_GTK_TOOLTIP);
>+      break;
>+    case MOZ_GTK_TOOLTIP_BOX_LABEL:
>+      style = CreateChildCSSNode("label",
>+                                 MOZ_GTK_TOOLTIP_BOX);

This is losing the "horizontal" class on the box, which can be observed by
adding "tooltip box.horizontal {padding: 100px;}" to gtk.css, for example.

However, rather than having to work out which classes should be on each
context, please continue to use the CreateStyleForWidget() approach for root
nodes, where possible, and it is possible here.  That automatically finds the
correct classes for us and means the same code can be used for pre/post 3.20
GTK versions.

The best way to do this I think is to move MOZ_GTK_TOOLTIP and its child
widget contexts to GetWidgetRootStyle().  That probably requires a version
test for the MOZ_GTK_TOOLTIP case, but I expect that node is a rare exception.

The box and label are each actually root nodes of separate widgets, not child
gadgets within the GtkTooltipWindow.  Perhaps the only difference is that GTK
gives the nodes an object type, and so gtk_style_context_get_style() can find
class style properties.  (gtkcssmatcher.c prefers the object name to the type,
and so the type is not used for matching when there is a name.)  Although
Gecko doesn't call gtk_style_context_get_style() on these particular contexts,
I'd prefer to follow the same CreateStyleForWidget() pattern for all widget
root nodes where that is possible.

The changes to the other files all look good, thanks.
Attachment #8812750 - Flags: review?(karlt)
(Assignee)

Comment 3

2 years ago
Created attachment 8813268 [details] [diff] [review]
v.2 patch
Attachment #8813268 - Flags: review?(karlt)
(Assignee)

Updated

2 years ago
Attachment #8812750 - Attachment is obsolete: true
Comment on attachment 8813268 [details] [diff] [review]
v.2 patch

>+    case MOZ_GTK_TOOLTIP:
>+    case MOZ_GTK_TOOLTIP_BOX:
>+    case MOZ_GTK_TOOLTIP_BOX_LABEL: {

This works, but are you intentionally avoiding the pattern used for other
widgets in this function?:

    case MOZ_GTK_TOOLTIP_BOX_LABEL:
      style = CreateStyleForWidget(gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 0),
                                   MOZ_GTK_TOOLTIP);
      break;

which essentially removes node-specific sStyleStorage[] use/manipulation.

>+    ReleaseStyleContext(boxStyle);
> 
>     // Label drawing
>     InsetByBorderPadding(&rect, boxStyle);

Please move ReleaseStyleContext() to after InsetByBorderPadding(), as boxStyle
is used there also.  r+ with this change.
Attachment #8813268 - Flags: review?(karlt) → review+
(Assignee)

Comment 5

2 years ago
Created attachment 8813569 [details] [diff] [review]
patch v.3 for check-in

Ah, Thanks. Yes I think it's better to follow the hierarchy scheme here.
(Assignee)

Updated

2 years ago
Blocks: 1319075
Comment on attachment 8813569 [details] [diff] [review]
patch v.3 for check-in

Thanks!
Attachment #8813569 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → stransky

Comment 8

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba00502fa16
Move complete GtkTooltip stack to WidgetCache. r=karlt
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ba00502fa16
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.