1257811 - [GTK3] calc correct size of buttons/entries when border/padding is 0

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

Some themes (a default one in Fedora 24 for instance) does not specify border/padding for GtkEntry. It sets CSS min-height instead. It affects other elements like separators (http://osdir.com/ml/commits.gnome/2016-01/msg00254.html)

I think we should not calculate element sizes from border/padding but use a general gtk_widget_get_preferred_size() to let gtk compute the size for us.

This path tires does that for GtkButtons/Entries but we can extend it later for other widgets.

I'm a bit concerned if nsNativeThemeGTK::GetMinimumWidgetSize() is the correct way how to propagate final widget size. Another solution may be to retrieve min-height directly.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8732116 [details] [diff] [review]
patch

(In reply to Martin Stránský from comment #0)
> Some themes (a default one in Fedora 24 for instance) does not specify
> border/padding for GtkEntry. It sets CSS min-height instead.

Which theme is that?

Can you point me at the CSS, please?

They should specify border and padding so that the widgets are appropriately
sized when the font-size or label text layout or button images are large.

I can't imagine how this will work for a textarea (textfield-multiline), which
can have an arbitrary number of lines.

> It affects other elements like separators
> (http://osdir.com/ml/commits.gnome/2016-01/msg00254.html)

That's a simpler issue to solve as the separator is not a container and so
it's size does not depend on its children.

> I think we should not calculate element sizes from border/padding but use a
> general gtk_widget_get_preferred_size() to let gtk compute the size for us.

That sounds a good approach for the separator, arrows, and other items for
which there are no Gecko children.

However, it a button, for example, Gecko may have font-size, padding, and images from the HTML document.  Button text can also cover multiple lines.
e.g. data:text/html,<button>hello<br>world

> I'm a bit concerned if nsNativeThemeGTK::GetMinimumWidgetSize() is the
> correct way how to propagate final widget size. Another solution may be to
> retrieve min-height directly.

Perhaps an approach getting min-height directly might help deal with
document-specified padding in containers, but this may be tricky because
GetMinimumWidgetSize() is the border-box and so needs to include padding.

I don't think this would help with the different text sizes.

Can you provide an HTML example with screenshot, please, and identify the CSS in the theme?  That would help me think about an appropriate solution.

gtk_widget_get_preferred_size() sounds better for non-containers because it is
easier and uses the same code as GTK to sum borders, padding, and minimum
sizes.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> Comment on attachment 8732116 [details] [diff] [review]
> patch
> 
> (In reply to Martin Stránský from comment #0)
> > Some themes (a default one in Fedora 24 for instance) does not specify
> > border/padding for GtkEntry. It sets CSS min-height instead.
> 
> Which theme is that?
> 
> Can you point me at the CSS, please?

It's a default theme Adwaita in Gtk 3.20:

https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/gtk-contained.css#n2

a general padding is 0 and GtkEntry sets only:

min-height: 32px;
padding-left: 8px;
padding-right: 8px;

https://git.gnome.org/browse/gtk+/tree/gtk/theme/Adwaita/gtk-contained.css#n162

So the entry is low.

> They should specify border and padding so that the widgets are appropriately
> sized when the font-size or label text layout or button images are large.
> 
> I can't imagine how this will work for a textarea (textfield-multiline),
> which
> can have an arbitrary number of lines.

The textfield-multiline in FF is also rendered as a wrong widget. We use GtkEntry which is incorrect - it has round corners and scrollbars does not fit inside. We should introduce a new widget and draw it as GtkTextView with sharp corners which fits scrollbars.
 
> > It affects other elements like separators
> > (http://osdir.com/ml/commits.gnome/2016-01/msg00254.html)
> 
> That's a simpler issue to solve as the separator is not a container and so
> it's size does not depend on its children.
> 
> > I think we should not calculate element sizes from border/padding but use a
> > general gtk_widget_get_preferred_size() to let gtk compute the size for us.
> 
> That sounds a good approach for the separator, arrows, and other items for
> which there are no Gecko children.
> 
> However, it a button, for example, Gecko may have font-size, padding, and
> images from the HTML document.  Button text can also cover multiple lines.
> e.g. data:text/html,<button>hello<br>world

Yes, I see the problem here. From my testing nsNativeThemeGTK::GetMinimumWidgetSize(() doesn't seem to have any effect for in-html buttons - they're smaller than min-size set here.

> > I'm a bit concerned if nsNativeThemeGTK::GetMinimumWidgetSize() is the
> > correct way how to propagate final widget size. Another solution may be to
> > retrieve min-height directly.
> 
> Perhaps an approach getting min-height directly might help deal with
> document-specified padding in containers, but this may be tricky because
> GetMinimumWidgetSize() is the border-box and so needs to include padding.
> 
> I don't think this would help with the different text sizes.
> 
> Can you provide an HTML example with screenshot, please, and identify the
> CSS in the theme?  That would help me think about an appropriate solution.
> 
> gtk_widget_get_preferred_size() sounds better for non-containers because it
> is
> easier and uses the same code as GTK to sum borders, padding, and minimum
> sizes.

Another solution may be to let Gtk compute the preferred size from css theme for a one-line widget and calculate padding as (preferred_height - text_size_height). 

A screenshot of the small entries is here:
https://bugzilla.redhat.com/attachment.cgi?id=1119391

Let me know if you'd like to have other widgets.

Comment 4

[Karl Tomlinson (:karlt)]

Thanks, Martin.

For NS_THEME_TEXTFIELD and NS_THEME_NUMBER_INPUT, I think the best thing to do
is to read min-height from the style context.
I don't want to calculate a minimum height from the font-size for example,
which gtk_widget_get_preferred_size() may do.

I'm not certain that preventing the document from overriding the height is
best, but it may be, and if the document is using the GTK theme's widget, then
it probably makes sense to use its min-height.

Looking at gtk_css_gadget_get_preferred_size(), it seems that min-height
affects content box height and so border+padding should be added to that.

(It's also interesting to note that get_preferred_size includes margins, so we
may need to watch out for that if/when using that method.)

NS_THEME_FOCUS_OUTLINE is only related to GtkEntry by implementation, and, as
you say, NS_THEME_TEXTFIELD_MULTILINE should not be using GtkEntry.

I'd be happy with something similar for buttons, if it worked, but there's not
much point if it doesn't help, or is not required.  If buttons are inline non-replaced elements, and maybe they are, then min-height should have no effect [1].  Perhaps it makes a difference for xul.  I don't know.

I'd prefer not to use assumptions that min-height or size differences imply padding, but maybe we can reconsider if the need becomes obvious.  I'm guessing there are more important things to address.  At least Adwaita provides padding for buttons.  The missing padding on entry may just be an oversight, so maybe we only need the workaround for that.

Please make any new functions return void if the return value would always be
success.

[1] https://www.w3.org/TR/css3-box/#min-height

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: min size patch

Min-height patch for input elements, Thanks!

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8735765 [details] [diff] [review]
min size patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f76ca01b07

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.3

This one is against the new cpp file. Thanks.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8735864 [details] [diff] [review]
patch v.3

Border and padding needs to be added to min-height to adjust from content box to the border box that GetMinimumWidgetSize() returns.

>+    gtk_style_context_get(style, gtk_style_context_get_state (style),
>+                          "min-height", height,
>+                          NULL);

min-height is new in 3.20 and so the version needs to be checked before
fetching with varargs methods like this.

No space before "(style)".
Use nullptr instead of NULL, because NULL is 0 not (void*)0 in C++.

>+ * Get the desired height of a entry widget
>+ * size:    [OUT] the desired height
>+ *
>+ * returns: MOZ_GTK_SUCCESS if there was no error, an error code otherwise
>+ */
>+void moz_gtk_get_entry_height(gint* height);

Please call this moz_gtk_get_entry_min_height because this will be zero in
many themes.

>+  case NS_THEME_FOCUS_OUTLINE:
>+  case NS_THEME_NUMBER_INPUT:
>+  case NS_THEME_TEXTFIELD:
>+  case NS_THEME_TEXTFIELD_MULTILINE:

Please remove NS_THEME_TEXTFIELD_MULTILINE and NS_THEME_FOCUS_OUTLINE.

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.4

Thanks, there's the updated one.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8736221 [details] [diff] [review]
patch v.4

Btw. I used GTK_STATE_FLAG_NORMAL in the border/padding code as it's used in other places in the file and we may replace it at once for gtk3.20.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8736221 [details] [diff] [review]
patch v.4

>+    GtkBorder border;
>+    GtkBorder padding;
>+    gtk_style_context_get_border(style, GTK_STATE_FLAG_NORMAL, &border);
>+    gtk_style_context_get_padding(style, GTK_STATE_FLAG_NORMAL, &padding);
>+
>+    *height += (border.top + border.bottom + padding.top + padding.bottom);
>+
>+    // XXX: Subtract 1 pixel from the top/bottom padding to account for the default
>+    // padding in forms.css. See bug 1187385.
>+    if (*height >= 2)
>+        *height -= 2;

Please {} brace conditional blocks that are not jump statements.

But instead please share the code.
Whenever it seems necessary to copy code, it is time to refactor, but
calling moz_gtk_get_widget_border(MOZ_GTK_ENTRY, ... GTK_TEXT_DIR_NONE, false)
from here would be fine.

>+ * Get the desired height of a entry widget
>+ * size:    [OUT] the desired height

s/desired/minimum/ * 2

>       aResult->height += border.top + border.bottom;
>     }
>     break;
>+  case NS_THEME_NUMBER_INPUT:
>+  case NS_THEME_TEXTFIELD:
>+    {
>+        moz_gtk_get_entry_min_height(&aResult->height);
>+    }
>+    break;
>   case NS_THEME_TOOLBAR_SEPARATOR:
>     {
>       gint separator_width;

Although the existing indentation here is not Gecko style, please reduce the
indentation of moz_gtk_get_entry_min_height to align with surrounding code of
the same depth.

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> >+    GtkBorder border;
> >+    GtkBorder padding;
> >+    gtk_style_context_get_border(style, GTK_STATE_FLAG_NORMAL, &border);
> >+    gtk_style_context_get_padding(style, GTK_STATE_FLAG_NORMAL, &padding);
> >+
> >+    *height += (border.top + border.bottom + padding.top + padding.bottom);
> >+
> >+    // XXX: Subtract 1 pixel from the top/bottom padding to account for the default
> >+    // padding in forms.css. See bug 1187385.
> >+    if (*height >= 2)
> >+        *height -= 2;

Please ignore my comment about sharing code for this.
I expect the 1 pixel of padding from forms.css will not be added to what GetMinimumWidgetSize() returns, because that is already a border box.
Therefore don't subtract here.

That makes this code different from moz_gtk_get_widget_border(), so the code can't be shared.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.5 for check-in

Thanks, there's the fixed one. A also added height value reset when it's not initialized by min-height style.

Try:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b66583222488

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8737157 [details] [diff] [review]
patch v.5 for check-in

> A also added height value reset when it's not
> initialized by min-height style.

Ah, yes.  Thanks for that!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a30f2dc662b0

Comment 16

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/a30f2dc662b0