1381815 - [gtk] Radio and checkbox indicator blurred

Status: RESOLVED FIXED

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

Description

[Jan Horak [:jhorak]]

The radio and checkbox state indicator (not frame or background) is rendered in wrong size. It uses indicator_size and indicator_spacing to determine size, but since GTK 3.20 this is obsolete and no longer supported. (see [1]). This results to blurred image.

Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1374746

[1] https://developer.gnome.org/gtk3/stable/GtkCheckButton.html#GtkCheckButton--s-indicator-size

Comment 1

[Jan Horak [:jhorak]]

Attachment: Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

In the GTK < 3.20 the size of radio and checkbox toggle is determined by indicator
spacing and indicator size. By  GTK 3.20+ it is replaced by standard box model
(padding, margin, border). The patch fixes that while keeping the functionality
for older GTK. The values are also cached by similar way as scrollbar metrics
are cached now.

Review commit: https://reviewboard.mozilla.org/r/158844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158844/

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

https://reviewboard.mozilla.org/r/158844/#review165596

::: widget/gtk/gtk3drawing.cpp:2617
(Diff revision 1)
> +      GtkStateFlags state_flags = gtk_style_context_get_state(style);
> +      gtk_style_context_get(style, state_flags,
> +                            "min-height",&(metrics->min_size.height),
> +                            "min-width", &(metrics->min_size.width),
> +                            nullptr);

Notice that gtk_builtin_icon_get_preferred_size will fallback to default_size_property for
each direction when there is no min-* property, and "indicator-size" is the 
default_size_property on the builtin_icon.

Some themes may be depending on this, and so please include this fallback to
avoid the risk of regressing such themes.

::: widget/gtk/gtk3drawing.cpp:2632
(Diff revision 1)
> +    } else {
> +      gint indicator_size, indicator_spacing;
> +      gtk_widget_style_get(GetWidget(MOZ_GTK_CHECKBUTTON_CONTAINER),
> +                           "indicator_size", &indicator_size,
> +                           "indicator_spacing", &indicator_spacing,
> +                           NULL);

nullptr, which can be different from NULL in C++, which is significant when passing to varargs functions.

::: widget/gtk/gtkdrawing.h:90
(Diff revision 1)
> +  GtkBorder padding;
> +  GtkBorder border;

These two are only used together and so please collapse into a single member named something like |borderAndPadding|.

::: widget/gtk/gtkdrawing.h:405
(Diff revision 1)
>  moz_gtk_checkbox_get_metrics(gint* indicator_size, gint* indicator_spacing);
>  
>  /**
> + * Get metrics of the toggle (radio or checkbox)
> + * isRadio:            [IN] true when requesting metrics for the radio button
> + * returns:    instance of ToggleGTKMetrics struct

s/instance of/pointer to/ so as not to imply a new instance, which would need to be freed.

::: widget/gtk/nsNativeThemeGTK.cpp:1035
(Diff revision 1)
> -      gint indicator_size, indicator_spacing;
> -
> -      if (aWidgetType == NS_THEME_CHECKBOX) {
> -        moz_gtk_checkbox_get_metrics(&indicator_size, &indicator_spacing);
> -      } else {
> +      const ToggleGTKMetrics* metrics = GetToggleMetrics(aWidgetType == NS_THEME_RADIO);
> +      aExtra->top = metrics->margin.top;
> +      aExtra->right = metrics->margin.right;
> +      aExtra->bottom = metrics->margin.bottom;
> +      aExtra->left = metrics->margin.left;
> -        moz_gtk_radio_get_metrics(&indicator_size, &indicator_spacing);
> -      }
> -
> -      aExtra->top = indicator_spacing;
> -      aExtra->right = indicator_spacing;
> -      aExtra->bottom = indicator_spacing;
> -      aExtra->left = indicator_spacing;

The purpose of GetExtraSizeForWidget was to indicate that the focus ring from
moz_gtk_toggle_paint() is drawn outside the widget.  The post-3.20 code will no longer draw
focus and so there is no need to include the margin here.

If you'd like to keep the current behavior for pre-3.20 then I suggest just
including a gint indicator_spacing member in ToggleGTKMetrics, and setting that to
zero for post-3.20.

However, even 3.18 doesn't draw focus around the indicator (but around the
label), and since
https://hg.mozilla.org/mozilla-central/rev/ad249ce81497#l1.12
Gecko draws its own focus around the indicator.

I think it would therefore be best to remove the gtk_render_focus() altogether
and then GetExtraSizeForWidget() no longer needs cases for CHECKBOX and RADIO.
If you'd like to do that then please include that change in the commit message.

::: widget/gtk/nsNativeThemeGTK.cpp:1600
(Diff revision 1)
> -
> -      if (aWidgetType == NS_THEME_CHECKBOX) {
> +      aResult->width = metrics->min_size.width + metrics->padding.left + metrics->padding.right + metrics->border.left + metrics->border.right;
> +      aResult->height = metrics->min_size.height + metrics->padding.top + metrics->padding.bottom + metrics->border.top + metrics->border.bottom;

Can you include the border and padding in the min_size member in GetToggleMetrics(), please, so that GetMinimumWidgetSize doesn't need to check the GTK version?

(Call the member "minBorderSize", to be clear, if you like.)

moz_gtk_toggle_paint() would no longer need to add the border and padding, but would still need to subtract for the render_checkbox().

Comment 3

[Hussam Al-Tayeb]

The patch fixes the blurring of the checkbox in html packages (for example the one near "Need more information form" on this page).
Please also take a look at the checkbutton when you right click on the hamburger menu. That needs fixing as well.
Thank you.

Comment 4

[Jan Horak [:jhorak]]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158844/diff/1-2/

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

https://reviewboard.mozilla.org/r/158844/#review167596

::: widget/gtk/gtk3drawing.cpp:2628
(Diff revisions 1 - 2)
> +      metrics->minSizeWithBorder.width += metrics->borderAndPadding.left + metrics->borderAndPadding.right;
> +      metrics->minSizeWithBorder.height += metrics->borderAndPadding.top + metrics->borderAndPadding.bottom;

Please wrap to remain within 80 columns.

::: widget/gtk/gtkdrawing.h:89
(Diff revisions 1 - 2)
>  } ScrollbarGTKMetrics;
>  
>  typedef struct {
>    bool initialized;
> -  MozGtkSize min_size;
> +  MozGtkSize minSizeWithBorder;
>    GtkBorder margin;

No need for margin, or indicator_spacing, now.

::: widget/gtk/gtk3drawing.cpp:329
(Diff revision 2)
> -    // XXX we should assert rect->height >= indicator_size too
> -    // after bug 369581 is fixed.
> -    MOZ_ASSERT(rect->width >= indicator_size,
> +    MOZ_ASSERT(rect->width >= metrics->minSizeWithBorder.width,
> +               "GetMinimumWidgetSize was ignored");
> +    MOZ_ASSERT(rect->width >= metrics->minSizeWithBorder.height,

Bug 369581 is not fixed, and so I assume the height assert can still fail.  Better to leave the comment and not add the height assert, please.  (Comparing width with height didn't same quite right anyway.)

::: widget/gtk/gtk3drawing.cpp:356
(Diff revision 2)
> +        gint indicator_width = metrics->minSizeWithBorder.width - metrics->borderAndPadding.left - metrics->borderAndPadding.right;
> +        gint indicator_height = metrics->minSizeWithBorder.height - metrics->borderAndPadding.top - metrics->borderAndPadding.bottom;

Please wrap to remain with 80 columns.

::: widget/gtk/gtk3drawing.cpp:2614
(Diff revision 2)
> +      if (metrics->minSizeWithBorder.height == 0) {
> +        metrics->minSizeWithBorder.height = 16; // TOGGLE_WIDTH from gtkcellrenderertoggle.c
> +      }
> +      if (metrics->minSizeWithBorder.width == 0) {
> +        metrics->minSizeWithBorder.width = 16; // TOGGLE_WIDTH from gtkcellrenderertoggle.c

calc_indicator_size() in gtkcellrenderertoggle.c version 3.20 uses
TOGGLE_WIDTH only if indicator-size is not set.

However, style here is from a GtkCheckButton, and so I assume it is better to
look at gtkcheckbutton.c and gtkbuiltinicon.c, which don't fallback from
indicator-size to TOGGLE_WIDTH.

I'm expecting indicator-size to be used for fallback.

Comment 6

[Jan Horak [:jhorak]]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158844/diff/2-3/

Comment 7

[Jan Horak [:jhorak]]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158844/diff/3-4/

Comment 8

[Jan Horak [:jhorak]]

(In reply to Hussam Al-Tayeb from comment #3)
> The patch fixes the blurring of the checkbox in html packages (for example
> the one near "Need more information form" on this page).
> Please also take a look at the checkbutton when you right click on the
> hamburger menu. That needs fixing as well.
Thanks for pointing it out. I'm going to address that in next patch.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

https://reviewboard.mozilla.org/r/158844/#review168186

Looks good, thanks.  Just file style variations.

::: widget/gtk/gtk3drawing.cpp:356
(Diff revision 4)
> +        gint indicator_width = metrics->minSizeWithBorder.width -
> +          metrics->borderAndPadding.left - metrics->borderAndPadding.right;
> +        gint indicator_height = metrics->minSizeWithBorder.height -
> +          metrics->borderAndPadding.top - metrics->borderAndPadding.bottom;
> +        if (isradio) {
> +           gtk_render_option(style, cr, indicator_x, indicator_y,
> +                             indicator_width, indicator_height);
> +        } else {
> +           gtk_render_check(style, cr, indicator_x, indicator_y,
> +                            indicator_width, indicator_height);
> -    }
> +        }
> -
> +    } else {
> -    if (isradio) {
> +      if (isradio) {

This file uses 4-space indentation.  Similarly elsewhere.

Comment 10

[Jan Horak [:jhorak]]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158844/diff/4-5/

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

https://reviewboard.mozilla.org/r/158844/#review168504

Comment 12

[vitalik.perevertun]

I think this is same bug https://bugzilla.mozilla.org/show_bug.cgi?id=1311499

> The post-3.20 code will no longer draw
focus and so there is no need to include the margin here.

This is not true.
I use personal theme that render focus(i use modified last patch from bug 1311499).
I can give screenshot. gtk code check if border style is not GTK_BORDER_STYLE_NONE then render him.
But probably, this does not make much sense.
Default Adwaita theme not render him.


Also i don't see margin size in patch https://reviewboard.mozilla.org/r/158844/#review168504.
Is it counted?

Comment 13

[Jan Horak [:jhorak]]

Please attach screenshot and the custom theme, if possible.

Comment 14

[vitalik.perevertun]

Attachment: checkbox_ff55.png

firefox 55 with patch from bug 1311499

Comment 15

[vitalik.perevertun]

> Please attach screenshot and the custom theme, if possible.

https://www.gnome-look.org/p/1186425/
https://dl.opendesktop.org/api/files/download/id/1502188127/onetwo-dev.tar.xz

this is my theme.

Comment 16

[Jan Horak [:jhorak]]

(In reply to vitalik.perevertun from comment #12)
> I think this is same bug https://bugzilla.mozilla.org/show_bug.cgi?id=1311499
> 
> > The post-3.20 code will no longer draw
> focus and so there is no need to include the margin here.
> 
> This is not true.
> I use personal theme that render focus(i use modified last patch from bug
> 1311499).
> I can give screenshot. gtk code check if border style is not
> GTK_BORDER_STYLE_NONE then render him.
> But probably, this does not make much sense.
> Default Adwaita theme not render him.

Could you please provide link to the sources, for example by using github repo:
https://github.com/GNOME/gtk
What I see is that gtk_render_focus was in gtk_check_button_paint 3.18 and in 3.20 it seems to be used just for GtkCellArea, GtkTreeView, GtkIconView, GtkCalendar, GtkDrawingArea and GtkLabel.
What Gtk version do you use on your system?

The screenshot of the theme: https://cn.pling.com/img/b/a/6/d/e0e4a43989dd84d9ac552d70829e68cb9eb9.png seems to be a little awkward, because the focus indicator is somehow rendered twice.

Comment 17

[vitalik.perevertun]

> Could you please provide link to the sources, for example by using github repo:

Sources of theme?

> What Gtk version do you use on your system?

$ rpm -qa gtk3*
gtk3-debuginfo-3.22.15-1.x86_64
gtk3-3.22.15-1.x86_64
gtk3-devel-3.22.15-1.x86_64

> The screenshot of the theme: https://cn.pling.com/img/b/a/6/d/e0e4a43989dd84d9ac552d70829e68cb9eb9.png seems to be a little awkward, because the focus indicator is somehow rendered twice.

Maybe. Theme is not completed yet.

Comment 18

[Jan Horak [:jhorak]]

(In reply to vitalik.perevertun from comment #17)
> > Could you please provide link to the sources, for example by using github repo:
> 
> Sources of theme?
No, what I'm asking is where do you see:
 "Gtk code check if border style is not GTK_BORDER_STYLE_NONE then render him"

> > What Gtk version do you use on your system?
> 
> $ rpm -qa gtk3*
> gtk3-debuginfo-3.22.15-1.x86_64
> gtk3-3.22.15-1.x86_64
> gtk3-devel-3.22.15-1.x86_64
> 
> > The screenshot of the theme: https://cn.pling.com/img/b/a/6/d/e0e4a43989dd84d9ac552d70829e68cb9eb9.png seems to be a little awkward, because the focus indicator is somehow rendered twice.
> 
> Maybe. Theme is not completed yet.
I mean no offence, just the double focus indicator is unusual.

Comment 19

[vitalik.perevertun]

> No, what I'm asking is where do you see:
> "Gtk code check if border style is not GTK_BORDER_STYLE_NONE then render him"

It's easy:)

https://git.gnome.org/browse/gtk+/tree/gtk/gtkrender.c?h=gtk-3-22#n428
https://git.gnome.org/browse/gtk+/tree/gtk/gtkrenderborder.c?h=gtk-3-22#n824


>  border_style[0] = _gtk_css_border_style_value_get (gtk_css_style_get_value (style, GTK_CSS_PROPERTY_OUTLINE_STYLE));
>  if (border_style[0] != GTK_BORDER_STYLE_NONE)
>    {



> I mean no offence, just the double focus indicator is unusual.

It's ok. I understand.
I just not have many time for this.
Much has not yet been done.

Comment 20

[Hussam Al-Tayeb]

(In reply to Jan Horak from comment #8)
> (In reply to Hussam Al-Tayeb from comment #3)
> > The patch fixes the blurring of the checkbox in html packages (for example
> > the one near "Need more information form" on this page).
> > Please also take a look at the checkbutton when you right click on the
> > hamburger menu. That needs fixing as well.
> Thanks for pointing it out. I'm going to address that in next patch.

Hello. Any updates on this part? (The checkbutton in firefox menus)
Thank you.

Comment 21

[vitalik.perevertun]

> Hello. Any updates on this part? (The checkbutton in firefox menus)

Hello.
I have patch that fixes checkboxes in menu.
If you can build firefox from sources, i will attach patch here.

Comment 22

[Hussam Al-Tayeb]

(In reply to vitalik.perevertun from comment #21)
> > Hello. Any updates on this part? (The checkbutton in firefox menus)
> 
> Hello.
> I have patch that fixes checkboxes in menu.
> If you can build firefox from sources, i will attach patch here.

I can build firefox from source. I used to build from aurora branch till it was closed so now I just stick to beta branch. But the last merge wasn't so long ago so the last patch applied correctly.

Comment 23

[Hussam Al-Tayeb]

The current patch doesn't apply for 57beta.

Comment 24

[Jan Horak [:jhorak]]

Comment on attachment 8887954 [details]
Bug 1381815 - fixing dimensions of radio and checkbox for GTK 3.20+;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158844/diff/5-6/

Comment 25

[Jan Horak [:jhorak]]

Attached updated patch for current trunk.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38a9db7104d9
fixing dimensions of radio and checkbox for GTK 3.20+; r=karlt

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/38a9db7104d9

Comment 28

[vitalik.perevertun]

Why margin size ignored?

In my theme checkbox is not correct rendered(default theme also). 
> File 	firefox-58.0a1.en-US.linux-x86_64.test_packages.json 	1K 	19-Oct-2017 01:04
I need margin size.

Default Adwaita theme also use him.
https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/gtk-contained.css?h=gtk-3-22#n1121

..
check, radio { margin: 0 4px; min-height: 14px; min-width: 14px;

Comment 29

[vitalik.perevertun]

> File 	firefox-58.0a1.en-US.linux-x86_64.tar.bz2 	60M 	19-Oct-2017 01:04

Tested on this firefox build.

Comment 30

[Jan Horak [:jhorak]]

(In reply to vitalik.perevertun from comment #28)
> Why margin size ignored?
> 
> In my theme checkbox is not correct rendered(default theme also). 
> > File 	firefox-58.0a1.en-US.linux-x86_64.test_packages.json 	1K 	19-Oct-2017 01:04
> I need margin size.
> 
> Default Adwaita theme also use him.
> https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/gtk-contained.
> css?h=gtk-3-22#n1121
> 
> ..
> check, radio { margin: 0 4px; min-height: 14px; min-width: 14px;

Please fill a different bug for this and link to this bug for reference.

Comment 31

[Hussam Al-Tayeb]

(In reply to Jan Horak [:jhorak] from comment #8)
> (In reply to Hussam Al-Tayeb from comment #3)
> > The patch fixes the blurring of the checkbox in html packages (for example
> > the one near "Need more information form" on this page).
> > Please also take a look at the checkbutton when you right click on the
> > hamburger menu. That needs fixing as well.
> Thanks for pointing it out. I'm going to address that in next patch.

The checkbox when right clicking on the hamburger menu (or any rightclick menu that has a checkbox) is still blurry. Can you please take a look?
Thank you.