Bug 52258

Summary: [GTK] Move toggle button rendering out of gtk2drawing.c
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, eric, mrobinson, webkit.review.bot, xan.lopez
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 52327, 52385    
Attachments:
Description Flags
Patch
none
Patch with fixes related to focus rects
none
Patch fixing some of the issues Carlos mentioned
none
Patch fixing focus rect issue none

Description Martin Robinson 2011-01-11 15:29:38 PST
This bug tracks removing toggle button painting out of gtk2drawing.c and adding a proper implementation in RenderThemeGtk2.cpp.
Comment 1 Martin Robinson 2011-01-11 15:51:40 PST
Created attachment 78612 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-11 15:54:39 PST
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.
Comment 3 Martin Robinson 2011-01-11 16:09:10 PST
Created attachment 78616 [details]
Patch with fixes related to focus rects
Comment 4 WebKit Review Bot 2011-01-11 16:11:43 PST
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.
Comment 5 Carlos Garcia Campos 2011-01-13 00:12:41 PST
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.
Comment 6 Carlos Garcia Campos 2011-01-13 00:59:24 PST
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);
Comment 7 Carlos Garcia Campos 2011-01-13 01:22:35 PST
(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.
Comment 8 Martin Robinson 2011-01-13 09:15:20 PST
(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.
Comment 9 Martin Robinson 2011-01-13 09:19:21 PST
Created attachment 78817 [details]
Patch fixing some of the issues Carlos mentioned
Comment 10 WebKit Review Bot 2011-01-13 09:22:06 PST
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.
Comment 11 Carlos Garcia Campos 2011-01-19 00:28:13 PST
(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
Comment 12 Martin Robinson 2011-01-19 09:44:51 PST
Created attachment 79435 [details]
Patch fixing focus rect issue
Comment 13 WebKit Review Bot 2011-01-19 09:47:02 PST
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.
Comment 14 Martin Robinson 2011-01-19 10:43:36 PST
(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.
Comment 15 Gustavo Noronha (kov) 2011-01-19 10:56:35 PST
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?
Comment 16 Martin Robinson 2011-01-19 15:52:41 PST
Committed r76175: <http://trac.webkit.org/changeset/76175>
Comment 17 Martin Robinson 2011-01-19 15:53:56 PST
(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.