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)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jhorak, Assigned: jhorak)
Details
(Keywords: polish, Whiteboard: tpi:+)
Attachments
(1 file, 5 obsolete files)
8.78 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
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)
Updated•8 years ago
|
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
Please have a look. Changes described in patch description.
Assignee: nobody → jhorak
Attachment #8799418 -
Attachment is obsolete: true
Attachment #8807114 -
Flags: review?(karlt)
Comment 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Keywords: checkin-needed
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
Um...this is embarrassing, you're right. I hope that's fine now.
Attachment #8809098 -
Attachment is obsolete: true
Attachment #8809328 -
Flags: review+
Assignee | ||
Comment 8•8 years ago
|
||
No, it is not, forgot to hg qrefresh.
Attachment #8809328 -
Attachment is obsolete: true
Attachment #8809334 -
Flags: review+
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7bc01deac5 Draw tooltips correctly r=karlt
Comment 10•8 years ago
|
||
Thank you. This is what I pushed, which just has some whitespace differences. (There were some lines over 80 columns.)
Updated•8 years ago
|
Attachment #8809334 -
Attachment is obsolete: true
Comment 11•8 years ago
|
||
bugherder |
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.
Description
•