Closed Bug 1308936 Opened 8 years ago Closed 8 years ago

[gtk3] Tooltips: wrong size and incorrectly drawn on some themes

Categories

(Core :: Widget: Gtk, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jhorak, Assigned: jhorak)

Details

(Keywords: polish, Whiteboard: tpi:+)

Attachments

(1 file, 5 obsolete files)

Tooltips on GTK3 are composed from various GTK gui elements, the tree for tooltip is [1] and [2]:
Tooltip window
  Horizontal Box
    Icon
    Label

All of included elements can have box model specified (padding/margin/border). We currently only draw Tooltip element. To fix this we need to draw all elements and compute right dimension of tooltip element.

Bug 1268241 might be related.

[1] https://github.com/GNOME/gtk/blob/5ea69a136bd7e4970b3a800390e20314665aaed2/gtk/gtktooltipwindow.c#L62
[2] https://github.com/GNOME/gtk/blob/5ea69a136bd7e4970b3a800390e20314665aaed2/gtk/ui/gtktooltipwindow.ui
Attached patch tooltip-dimensions-fix.patch (obsolete) — Splinter Review
What I generally feel bad about is change in popup.css:
-  padding: 4px;
+  padding: 6px;
The 6 pixels comes from there: https://github.com/GNOME/gtk/blob/5ea69a136bd7e4970b3a800390e20314665aaed2/gtk/ui/gtktooltipwindow.ui#L10
changing this value would break/change gtk2 tooltips.
Attachment #8799418 - Flags: feedback?(karlt)
Keywords: polish
Priority: -- → P4
Whiteboard: tpi:+
Comment on attachment 8799418 [details] [diff] [review]
tooltip-dimensions-fix.patch

CreateStyleForWidget() is good because it works with GTK versions before and
after 3.20.

Resolving a style context can be slow, so usually ClaimStyleContext() would be
used to cache, but tooltips don't animate and so this is probably fine as is.

moz_gtk_draw_styled_frame() can be used to simplify with the labelStyle.
(Using it for the boxStyle is also an option, but not such a clear win because
the margin would get subtracted twice.)  It is better not used for the
GtkWindow because GtkWindow doesn't subtract margins AFAICS.

moz_gtk_subtract_margin() can also be used to simplify a little.

>   /* GTK hardcodes this to 4px */
>-  padding: 4px;
>+  padding: 6px;

(In reply to Jan Horak from comment #1)
> The 6 pixels comes from there:
> https://github.com/GNOME/gtk/blob/5ea69a136bd7e4970b3a800390e20314665aaed2/
> gtk/ui/gtktooltipwindow.ui#L10
> changing this value would break/change gtk2 tooltips.

Thanks for the links.
I don't care too much about GTK2, but I assume Redhat are keen to keep
supporting it.

If you choose to use 6px of padding here then please update the comment and
include this link.

However, I'd suggest moving this from popup.css to
moz_gtk_get_widget_border().  Then it would be in the same file that uses the 6px when drawing, and different values can be used for GTK2 and GTK3.
Attachment #8799418 - Flags: feedback?(karlt) → feedback+
Attached patch tooltip-dimensions-fix v2 (obsolete) — Splinter Review
Please have a look. Changes described in patch description.
Assignee: nobody → jhorak
Attachment #8799418 - Attachment is obsolete: true
Attachment #8807114 - Flags: review?(karlt)
Comment on attachment 8807114 [details] [diff] [review]
tooltip-dimensions-fix v2

Good, thanks, but please change indentation in lines added here to 4 space,
and make some changes to the comments as indicated below.

>+    // The tooltip element has hardcoded 6px margin which is basically margin
>+    // between window and tooltip. The curious is that border of tooltip element
>+    // is drawn on over the 6px margin.

It is the box element that has the margin, and the distinction between the
window and tooltip is unclear (because there is no distinct tooltip element),
so please specify this precisely.

"The box element has hard-coded 6px margin-* GtkWidget properties, which are
added between the window dimensions and the CSS margin box of the horizontal
box."

For the second sentence, I suspect "the frame of the tooltip window is drawn in
the 6px margin."

>+    // For drawing Horizontal Box we have to inset drawing area by that 6px plus
>+    // its own margin.

s/own/CSS/.

>+            // In GTK 3 the spacing between box is set to 6. See details there:
>+            // https://github.com/GNOME/gtk/blob/5ea69a136bd7e4970b3a800390e20314665aaed2/gtk/ui/gtktooltipwindow.ui#L10

"There are 6 pixels of additional margin around the box."

The link should be #L11 because "spacing" is between the icon and label, and so not involved here.
Attachment #8807114 - Flags: review?(karlt) → review+
Attached patch tooltip-dimensions-fix v3 (obsolete) — Splinter Review
Copying review+ from previous patch. Fixed indentation and comments. Thanks for the review.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07afceae5146f8383595c6bbfba1ed84bd2107e6
Attachment #8807114 - Attachment is obsolete: true
Attachment #8809098 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8809098 [details] [diff] [review]
tooltip-dimensions-fix v3

>+    // The box element has hard-coded 6px margin-* GtkWidget properties, which are
>+    // added between the window dimensions and the CSS margin box of the horizontal
>+    // box.
>+    // The box element has hard-coded 6px margin-* GtkWidget which is basically margin
>+    // between window and box. The frame of the tooltip window is drawn in
>+    // the 6px margin.

I assume you meant to remove the second of these three sentences.

>+            GtkStyleContext* boxStyle =
>+              CreateStyleForWidget(gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 0),
>+                                   style);
>+            moz_gtk_add_margin_border_padding(boxStyle, left, top, right, bottom);
>+
>+            GtkStyleContext* labelStyle =
>+              CreateStyleForWidget(gtk_label_new(nullptr), boxStyle);
>+            moz_gtk_add_margin_border_padding(labelStyle, left, top, right, bottom);

CreateStyleForWidget lines should be indented by 4 spaces rather than 2.
Attached patch tooltip-dimensions-fix v4 (obsolete) — Splinter Review
Um...this is embarrassing, you're right. I hope that's fine now.
Attachment #8809098 - Attachment is obsolete: true
Attachment #8809328 - Flags: review+
Attached patch tooltip-dimensions-fix v5 (obsolete) — Splinter Review
No, it is not, forgot to hg qrefresh.
Attachment #8809328 - Attachment is obsolete: true
Attachment #8809334 - Flags: review+
Thank you.  This is what I pushed, which just has some whitespace differences.
(There were some lines over 80 columns.)
Attachment #8809334 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/bb7bc01deac5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: