This bug tracks removing toggle button painting out of gtk2drawing.c and adding a proper implementation in RenderThemeGtk2.cpp.
Created attachment 78612 [details] Patch
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.
Created attachment 78616 [details] Patch with fixes related to focus rects
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 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 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);
(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.
(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.
Created attachment 78817 [details] Patch fixing some of the issues Carlos mentioned
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.
(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
Created attachment 79435 [details] Patch fixing focus rect issue
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.
(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 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?
Committed r76175: <http://trac.webkit.org/changeset/76175>
(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.