Closed Bug 1317574 Opened 8 years ago Closed 8 years ago

incorrect horizontal padding before check menuitem indicator

Categories

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

Product:
Core
Component:
Widget: Gtk
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

bug 1317574 use menuitem padding between menuitem and check indicator
58 bytes, text/x-review-board-request
stransky
: review+
Details
bug 1317574 rename radio/check menuitem and indicator as used with ClaimStyleContext
58 bytes, text/x-review-board-request
stransky
: review+
Details
bug 1317574 rename subtract_margin and rectangle_inset to Inset* and swap InsetByMargin parameters for consistency
58 bytes, text/x-review-board-request
stransky
: review+
Details
bug 1317574 adjust menuitem padding and check/radio indicator position for changes in GTK 3.20
58 bytes, text/x-review-board-request
stransky
: review+
Details 
This can be seen in Awaita 3.20.

It is more obvious in GTK versions > 3.20 because the style context for the indicator does not derive classes from its menuitem style context, and so does not pick up padding for the menuitem.
Priority: P1 → P3
Comment on attachment 8811106 [details]
bug 1317574 use menuitem padding between menuitem and check indicator

https://reviewboard.mozilla.org/r/93324/#review93438

::: widget/gtk/gtk3drawing.cpp:1897
(Diff revision 1)
>  
>      style = ClaimStyleContext(isradio ? MOZ_GTK_RADIOMENUITEM :
>                                          MOZ_GTK_CHECKMENUITEM,
>                                direction, state_flags);
> -    gtk_style_context_get_padding(style, state_flags, &padding);
>      gint offset = padding.left + 2;

That produces asymetrical placement of the checkbox - it's moved too right, probably due to the +2 hack.

I think the correct solution here is follow spacing we return in moz_gtk_get_widget_border(), i.e. to sum padding from container and item and remove the +2 hack:

style = ClaimStyleContext(container)     gtk_style_context_get_padding();
gint offset = padding.left;

style = ClaimStyleContext(item)
gtk_style_context_get_padding(style) 
offset += padding.left;
Comment on attachment 8811107 [details]
bug 1317574 rename radio/check menuitem and indicator as used with ClaimStyleContext

https://reviewboard.mozilla.org/r/93326/#review93440

Works for me.

::: widget/gtk/gtk3drawing.cpp:1892
(Diff revision 1)
>                                  "horizontal-padding", &horizontal_padding,
>                                  NULL);
>      gtk_style_context_get_padding(style, state_flags, &padding);
>      ReleaseStyleContext(style);
>  
> -    style = ClaimStyleContext(isradio ? MOZ_GTK_RADIOMENUITEM :
> +    bool isRadio = widgetType == MOZ_GTK_RADIOMENUITEM;

nit: I'd prefer "(widgetType == MOZ_GTK_RADIOMENUITEM)" for more readibility.
Attachment #8811107 - Flags: review?(stransky) → review+
Comment on attachment 8811106 [details]
bug 1317574 use menuitem padding between menuitem and check indicator

https://reviewboard.mozilla.org/r/93324/#review93442
Attachment #8811106 - Flags: review?(stransky)
Comment on attachment 8811106 [details]
bug 1317574 use menuitem padding between menuitem and check indicator

https://reviewboard.mozilla.org/r/93324/#review93438

> That produces asymetrical placement of the checkbox - it's moved too right, probably due to the +2 hack.
> 
> I think the correct solution here is follow spacing we return in moz_gtk_get_widget_border(), i.e. to sum padding from container and item and remove the +2 hack:
> 
> style = ClaimStyleContext(container)     gtk_style_context_get_padding();
> gint offset = padding.left;
> 
> style = ClaimStyleContext(item)
> gtk_style_context_get_padding(style) 
> offset += padding.left;

The +2 comes from gtk_real_check_menu_item_draw_indicator() in pre-3.20 GTK.

This patch is correcting moz_gtk_check_menu_item_paint() to get the padding
from the root CSS node of the menuitem as
gtk_real_check_menu_item_draw_indicator did:

https://git.gnome.org/browse/gtk+/tree/gtk/gtkcheckmenuitem.c?h=3.18.9#n546

The main difference between moz_gtk_check_menu_item_paint and
gtk_real_check_menu_item_draw_indicator is that GTK has

  (toggle_size - toggle_spacing - indicator_size) / 2;

to add half the extra available space when |toggle_size| is greater than the
requested size |toggle_spacing + indicator_size|.  |toggle_size| is determined
by the menu as a maximum of requested sizes from all menuitems.

Here, we don't have the precise spacing between the left edge of the menuitem
content box and the label.  I suspect some hard-coded numbers in
toolkit/themes/linux/global/menu.css may determine that.  I guess it all
happens to work out roughly OK just because icons are similar sizes to
indicators, and different themes have similar spacing between the indicators
and the labels.

In the GTK2 code, the difference between the left offset returned from
moz_gtk_get_widget_border() and the distance from the border box left edge to
the check left position is horizontal_padding + 2.

The left offset returned by moz_gtk_get_widget_border for GTK3 comes from the
menuitem (which is called the container).  There is only one set of padding
considered there.  This patch is making moz_gtk_check_menu_item_paint use the
same padding.  The difference between the left border width and the check
offset then becomes horizontal_padding + 2 again.

You are right that in GTK 3.20 there is no +2.  I'll put up another patch to
remove that.

In 3.20 there may also be padding in the GtkBuiltinIcon indicator_gadget.
Failure to consider that is causing bug 1311499.  To be consistent with
gtk_css_gadget_draw(), that padding should be added before gtk_render_check()
but not before gtk_render_background/frame.  It will only affect the
position of the check, not its box, and so I think it is best to use bug
1311499 to add this padding.
Comment on attachment 8811106 [details]
bug 1317574 use menuitem padding between menuitem and check indicator

(I can't seem to re-request review in reviewboard.)
Attachment #8811106 - Flags: review?(stransky)
Comment on attachment 8811107 [details]
bug 1317574 rename radio/check menuitem and indicator as used with ClaimStyleContext

https://reviewboard.mozilla.org/r/93326/#review94250

Fine, Thanks.
Comment on attachment 8812033 [details]
bug 1317574 rename subtract_margin and rectangle_inset to Inset* and swap InsetByMargin parameters for consistency

https://reviewboard.mozilla.org/r/93946/#review94256
Attachment #8812033 - Flags: review?(stransky) → review+
Comment on attachment 8811106 [details]
bug 1317574 use menuitem padding between menuitem and check indicator

https://reviewboard.mozilla.org/r/93324/#review94294

::: widget/gtk/gtk3drawing.cpp:1897
(Diff revision 1)
>  
>      style = ClaimStyleContext(isradio ? MOZ_GTK_RADIOMENUITEM :
>                                          MOZ_GTK_CHECKMENUITEM,
>                                direction, state_flags);
> -    gtk_style_context_get_padding(style, state_flags, &padding);
>      gint offset = padding.left + 2;

Ah, thanks for the explanation.
Attachment #8811106 - Flags: review?(stransky) → review+
Comment on attachment 8812034 [details]
bug 1317574 adjust menuitem padding and check/radio indicator position for changes in GTK 3.20

https://reviewboard.mozilla.org/r/93948/#review94266

::: widget/gtk/gtk3drawing.cpp:478
(Diff revision 1)
>                                   &margin);
>      rectangle_inset(rect, margin);
>  }
>  
> +static void
> +subtract_border_padding(GdkRectangle* rect, GtkStyleContext* style)

IMHO It whould be more readable and consistent to use the "inset" terminology here because the code actually adds offset and substract size. Something like:

rectangle_inset()
rectangle_inset_margin()
rectangle_inset_border_padding()

::: widget/gtk/gtk3drawing.cpp:1892
(Diff revision 1)
>  
>      if (checked) {
>        state_flags = static_cast<GtkStateFlags>(state_flags|checkbox_check_state);
>      }
>  
> +    bool pre_3_20 = gtk_get_minor_version() < 20;

Nit: An unstable/developer Gtk4 version may come soon and would be nice to count with it. But that may be addressed in a separate patch.
Attachment #8812034 - Flags: review?(stransky) → review+
Comment on attachment 8812034 [details]
bug 1317574 adjust menuitem padding and check/radio indicator position for changes in GTK 3.20

https://reviewboard.mozilla.org/r/93948/#review94266

> IMHO It whould be more readable and consistent to use the "inset" terminology here because the code actually adds offset and substract size. Something like:
> 
> rectangle_inset()
> rectangle_inset_margin()
> rectangle_inset_border_padding()

I went with Inset/InsetByMargin/InsetByBorderPadding to follow Gecko's CamelCase style, dropping the rectangle_ prefix unnecessary with C++'s function resolution by parameter type, and adding "By" to clarify that the margin is not what is inset.

> Nit: An unstable/developer Gtk4 version may come soon and would be nice to count with it. But that may be addressed in a separate patch.

I suspect GTK4 stability will be a way off, and by that time we may be able to drop some of the older GTK3 version checks.  Note that gtk_check_version(3,20,0) would return non-null for GTK4 as well as older GTK3 versions, so we'd need a function with a different behavior if code can be shared between GTK4 and later GTK3 versions.
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3ddbf4e0466
use menuitem padding between menuitem and check indicator r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/527d6a2d56f8
rename radio/check menuitem and indicator as used with ClaimStyleContext r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/a7e1f3af0df6
rename subtract_margin and rectangle_inset to Inset* and swap InsetByMargin parameters for consistency r=stransky+263117
https://hg.mozilla.org/integration/autoland/rev/6742c48baf82
adjust menuitem padding and check/radio indicator position for changes in GTK 3.20 r=stransky+263117
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: