Bug 52258 – [GTK] Move toggle button rendering out of gtk2drawing.c

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

[Martin Robinson]

This bug tracks removing toggle button painting out of gtk2drawing.c and adding a proper implementation in RenderThemeGtk2.cpp.

[Martin Robinson]

Created attachment 78612 [details]
Patch

[WebKit Review Bot]

Attachment 78612 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:110:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Created attachment 78616 [details]
Patch with fixes related to focus rects

[WebKit Review Bot]

Attachment 78616 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:110:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 78616 [details]
Patch with fixes related to focus rects

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

Unofficial review.

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:106
> +    gint focusWidth = 0, focusPad = 0;
> +    gboolean interiorFocus = 0;

Style properties return the default value when they are not set, so we don't need to initialize these variables.

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:219
> +        widgetContext.gtkPaintFocus(focusRect, widget, GTK_STATE_ACTIVE, "checkbutton");

Why GTK_STATE_ACTIVE, I think you should use the current widget state, getGtkStateType(theme, renderObject).

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:225
> +    if (!info.context->paintingDisabled())

We don't need this, I already removed it from RenderThemeGtk3, RenderTheme.cpp already checks it every time it calls any painting method.

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:237
> +    if (!info.context->paintingDisabled())

Ditto.

> Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:178
> +    GdkRectangle paintRect = { m_paintRect.x + rect.x(), m_paintRect.y + rect.y(), rect.width(), rect.height() };

I think we should paint the indicator centered, mozilla code did it 
y = rect->y + (rect->height - indicator_size) / 2;

> Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:186
> +    GdkRectangle paintRect = { m_paintRect.x + rect.x(), m_paintRect.y + rect.y(), rect.width(), rect.height() };

Ditto.

[Carlos Garcia Campos]

Comment on attachment 78616 [details]
Patch with fixes related to focus rects

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

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:201
> +    bool indeterminate = theme->isIndeterminate(renderObject);
> +    gtk_toggle_button_set_inconsistent(GTK_TOGGLE_BUTTON(widget), TRUE);

I guess here you mean gtk_toggle_button_set_inconsistent(GTK_TOGGLE_BUTTON(widget), indeterminate);

[Carlos Garcia Campos]

(In reply to comment #5)
> > Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:178
> > +    GdkRectangle paintRect = { m_paintRect.x + rect.x(), m_paintRect.y + rect.y(), rect.width(), rect.height() };
> 
> I think we should paint the indicator centered, mozilla code did it 
> y = rect->y + (rect->height - indicator_size) / 2;

nevermind, in current code rect->height == indicator_size

> > Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:186
> > +    GdkRectangle paintRect = { m_paintRect.x + rect.x(), m_paintRect.y + rect.y(), rect.width(), rect.height() };
> 
> Ditto.

[Martin Robinson]

(In reply to comment #5)

> Unofficial review.

Thanks for the review!

> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:106
> > +    gint focusWidth = 0, focusPad = 0;
> > +    gboolean interiorFocus = 0;
> 
> Style properties return the default value when they are not set, so we don't need to initialize these variables.

Done.

> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:219
> > +        widgetContext.gtkPaintFocus(focusRect, widget, GTK_STATE_ACTIVE, "checkbutton");
> 
> Why GTK_STATE_ACTIVE, I think you should use the current widget state, getGtkStateType(theme, renderObject).

Mozilla was using always using GTK_STATE_ACTIVE, but it doesn't seem to break rendering to always use the widget state, so I have changed it.

> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:225
> > +    if (!info.context->paintingDisabled())
> 
> We don't need this, I already removed it from RenderThemeGtk3, RenderTheme.cpp already checks it every time it calls any painting method.
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:237
> > +    if (!info.context->paintingDisabled())
> 
> Ditto.

Removed.

> > Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:178
> > +    GdkRectangle paintRect = { m_paintRect.x + rect.x(), m_paintRect.y + rect.y(), rect.width(), rect.height() };
> I think we should paint the indicator centered, mozilla code did it 

This isn't actually the location of the indicator, but the location that the indicator will be painted onto the scratch buffer.

> I guess here you mean gtk_toggle_button_set_inconsistent(GTK_TOGGLE_BUTTON(widget), indeterminate);

Nice catch. Fixed.

[Martin Robinson]

Created attachment 78817 [details]
Patch fixing some of the issues Carlos mentioned

[WebKit Review Bot]

Attachment 78817 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:110:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

(In reply to comment #9)
> Created an attachment (id=78817) [details]
> Patch fixing some of the issues Carlos mentioned

I've tried the patch and the focus rect is never shown for check and radio buttons

[Martin Robinson]

Created attachment 79435 [details]
Patch fixing focus rect issue

[WebKit Review Bot]

Attachment 79435 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:111:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #11)
> (In reply to comment #9)
> > Created an attachment (id=78817) [details] [details]
> > Patch fixing some of the issues Carlos mentioned
> 
> I've tried the patch and the focus rect is never shown for check and radio buttons

This was an issue with the default focus rects (so not all themes). I've updated the patch to fix it. The problem was that m_paintRect was used in WidgetRenderingContext instead of the local paintRect.

[Gustavo Noronha (kov)]

Comment on attachment 79435 [details]
Patch fixing focus rect issue

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

Except for these spelling nits I see no issues with this patch, so r=me!

> Source/WebCore/ChangeLog:11
> +        (WebCore::adjustRectForFocus): Added this function which inflate a rect based

inflates

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:189
> +    // We do not call gtk_toggle_button_set_active here, because begin a series of animation

because begin? Are the series of animation timeouts started by gtk_toggle_button_set_active?

[Martin Robinson]

Committed r76175: <http://trac.webkit.org/changeset/76175>

[Martin Robinson]

(In reply to comment #15)
> (From update of attachment 79435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79435&action=review
> 
> Except for these spelling nits I see no issues with this patch, so r=me!
> 
> > Source/WebCore/ChangeLog:11
> > +        (WebCore::adjustRectForFocus): Added this function which inflate a rect based
> 
> inflates
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:189
> > +    // We do not call gtk_toggle_button_set_active here, because begin a series of animation
> 
> because begin? Are the series of animation timeouts started by gtk_toggle_button_set_active?

Thanks for the review. I've fixed the spelling errors and clarified these comments.