Bug 51454 – [GTK] Implement spin buttons in RenderThemeGtk

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Spin buttons are currently rendered as a text field, we should paint the inner up/down buttons.

[Carlos Garcia Campos]

Created attachment 77195 [details]
Implement spin buttons for gtk3

This patch implements spin buttons using gtk3 (I leave the gtk2 implementation to Martin :-P), the patch applies on top of the patch attached to bug 50820.

[Martin Robinson]

Nice patch! Should it be reviewed? It doesn't have an r? flag.

[Carlos Garcia Campos]

It doesn't have r because it doesn't apply, it depends on gtk-style-context port, so I didn't want bots to try to apply it.

[Carlos Garcia Campos]

Created attachment 78505 [details]
Implement spin buttons in RenderThemeGtk3

Updated patch for current git master and with some other issues fixed.

[Martin Robinson]

Comment on attachment 78505 [details]
Implement spin buttons in RenderThemeGtk3

View in context: https://bugs.webkit.org/attachment.cgi?id=78505&action=review

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:779
> +    GtkBorder padding;
> +    gtk_style_context_get_padding(context, static_cast<GtkStateFlags>(0), &padding);
> +
> +    int width = spinButtonArrowSize(context) + padding.left + padding.right;
> +    style->setWidth(Length(width, Fixed));
> +    style->setMinWidth(Length(width, Fixed));
> +}

Is it possible to just use reasonable values here rather than staying entirely true to the theme. If so, it might be better, as it will allow us to have the same layout regardless of the GTK+ theme / version.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 78505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78505&action=review
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:779
> > +    GtkBorder padding;
> > +    gtk_style_context_get_padding(context, static_cast<GtkStateFlags>(0), &padding);
> > +
> > +    int width = spinButtonArrowSize(context) + padding.left + padding.right;
> > +    style->setWidth(Length(width, Fixed));
> > +    style->setMinWidth(Length(width, Fixed));
> > +}
> 
> Is it possible to just use reasonable values here rather than staying entirely true to the theme. If so, it might be better, as it will allow us to have the same layout regardless of the GTK+ theme / version.

gtk2 doesn't support spin buttons, so we can leave this and once gtk2 supports spin buttons, if the test results don't match we can try to use another value.

[Martin Robinson]

(In reply to comment #6)
> gtk2 doesn't support spin buttons, so we can leave this and once gtk2 supports spin buttons, if the test results don't match we can try to use another value.

Carlos convinced me recently that we can just switch all the bots over to GTK 3.x when the first stable release comes out. After that we can still keep a GTK+ 2.x builder around, but not run the tests there. This should help us avoid the metrics problem entirely.

[Carlos Garcia Campos]

Created attachment 79250 [details]
Updated patch

Removed juntion sides for the frame since we are only drawing the buttons, not the frame.

[Martin Robinson]

Comment on attachment 79250 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79250&action=review

Looks good. Consider my suggestions below.

> Source/WebCore/ChangeLog:10
> +        Paint inner up/down buttons for spin button elements when building
> +        with GTK+ 3.x.
> +

Please make a note here that test results will land with the GTK+ 2.x version of this patch.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:761
> +    const PangoFontDescription* fontDesc = gtk_style_context_get_font(context, static_cast<GtkStateFlags>(0));

Please don't abbreviate fontDesc.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:837
> +    // Paint arrow centered inside button.
> +    IntRect arrowRect;
> +    gdouble angle;
> +    if (arrowType == GTK_ARROW_UP) {
> +        angle = 0;
> +        arrowRect.setY(rect.y());
> +        arrowRect.setHeight(rect.height() / 2 - 2);
> +    } else {
> +        angle = G_PI;
> +        arrowRect.setY(rect.y() + buttonRect.y());
> +        arrowRect.setHeight(rect.height() - arrowRect.y() - 2);
> +    }
> +    arrowRect.setWidth(rect.width() - 3);
> +    if (direction == GTK_TEXT_DIR_LTR)
> +        arrowRect.setX(rect.x() + 1);
> +    else
> +        arrowRect.setX(rect.x() + 2);
> +
> +    gint width = arrowRect.width() / 2;
> +    width -= width % 2 - 1; // Force odd.
> +    gint height = (width + 1) / 2;

For quite some time, I wondered why this logic was so hairy and thought surely there must be a better way. Then I set out to implement this code for GTK+ 2.x. After some time doing that and looking at the GTK+ code, I understand completely now.

I think we should would leave a comment at the top explaining that we're trying to emulate gtkspinbutton.c as closely as possible here. I'd also prefer "int" to "gint" in most places. :)

[Carlos Garcia Campos]

Committed r76577: <http://trac.webkit.org/changeset/76577>