Bug 51155 – [GTK] Menulist text often collides with separator

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

[Martin Robinson]

When rendering menulists in GTK+ the text of the button often collides with a separator (if the theme has one). Fixing this also presents a great opportunity to move the menulist rendering out of gtk2drawing.c.

[Martin Robinson]

Created attachment 76720 [details]
Patch for this issue

[Martin Robinson]

Created attachment 79291 [details]
Patch rebased to HEAD

[Martin Robinson]

(In reply to comment #2)
> Created an attachment (id=79291) [details]
> Patch rebased to HEAD

I've attached a new version of this patch rebased against ToT. It doesn't include the updated layout tests results, but I'll include them when this patch lands.

[WebKit Review Bot]

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

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

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


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

[Martin Robinson]

Removed the r? flag here, because I think this one deserves to go another round.

[Martin Robinson]

Created attachment 79323 [details]
Patch rebased against the latest

[Carlos Garcia Campos]

Comment on attachment 79323 [details]
Patch rebased against the latest

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

Great patch!, I've tried it and it works perfectly with the gtk+ default theme.

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:411
> +

hmm I didn't know that style property, we are not checking it, so I guess we are assuming that if the combobox widget doesn't have a separator in its children widget list it's because appears-as-list = 1

> Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:208
> +    GdkRectangle paintRect = { m_paintRect.x + rect.x(), m_paintRect.y + rect.y(), rect.width(), rect.height() };
> +    gtk_paint_arrow(gtk_widget_get_style(widget), m_target, stateType, shadowType, &m_paintRect, widget, detail,
> +                    static_cast<GtkArrowType>(arrowDirection), TRUE, paintRect.x, paintRect.y, paintRect.width, paintRect.height);

You are using m_paintRect here instead of paintRect

> Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:215
> +    GdkRectangle paintRect = { m_paintRect.x + rect.x(), m_paintRect.y + rect.y(), rect.width(), rect.height() };
> +    gtk_paint_vline(gtk_widget_get_style(widget), m_target, stateType, &m_paintRect, widget, detail,
> +                    paintRect.y, paintRect.y + paintRect.height, paintRect.x);

Ditto.

[Martin Robinson]

Comment on attachment 79323 [details]
Patch rebased against the latest

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

>> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:411
>> +
> 
> hmm I didn't know that style property, we are not checking it, so I guess we are assuming that if the combobox widget doesn't have a separator in its children widget list it's because appears-as-list = 1

Yep. We have to have the child widgets anyway, to ensure proper rendering, so we just check there. This was inspired by the Mozilla code.

>> Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:208
>> +                    static_cast<GtkArrowType>(arrowDirection), TRUE, paintRect.x, paintRect.y, paintRect.width, paintRect.height);
> 
> You are using m_paintRect here instead of paintRect

Very right. I had fixed this locally, but I should upload a new patch.

[Martin Robinson]

Created attachment 79610 [details]
Patch fixing paintRect issue

[WebKit Review Bot]

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

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

Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:318:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:438:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 6 files


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

[Xan Lopez]

Comment on attachment 79610 [details]
Patch fixing paintRect issue

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

Looks reasonable to me. r=me

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:335
> +    static GtkBorder defaultInnerBorder = {1, 1, 1, 1};

Hrm, is making this static useful/needed? It could be const, on the other hand.

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:365
> +        getComboBoxSeparatorWidth() + (3 * buttonWidgetStyle->xthickness);

Hrm, so where does the 3 come from?

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:408
> +    // | left border | Button text | xthickness | vseparator | xthickness | arrow | xthickness | right border |

Oh, I guess the 3 comes from here.

[Carlos Garcia Campos]

Comment on attachment 79610 [details]
Patch fixing paintRect issue

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

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:411
> +    // Menu list button painting strategy.
> +    // For buttons with appears-as-list set to false (having a separator):
> +    // | left border | Button text | xthickness | vseparator | xthickness | arrow | xthickness | right border |
> +    // For buttons with appears-as-list set to true (not having a separator):
> +    // | left border | Button text | arrow | xthickness | right border |
> +

This is not correct actually, it's not only a matter of showing a separator or not, appears-as-list has more implications. The main one is that the drop down list is not a menu, but a tree view, and the widgets to show the button are not the same. When appears-as-list is set, there's a frame, that might have a different bg color than a button depending on the theme, and a small button with the arrow, while when appears-as-list is not set the whole widget is a toggle button that contains an hbox with the separator and the arrow. I think we can just ignore this style property, since we are not going to show the same result than gtk+ and, even more important, the drop down will always be a menu in our case. 
See current implementations of the combo box depending on appears-as-list style property:
http://git.gnome.org/browse/gtk+/tree/gtk/gtkcombobox.c#n3246
http://git.gnome.org/browse/gtk+/tree/gtk/gtkcombobox.c#n2940

[Martin Robinson]

(In reply to comment #11)

Thanks for the review!

> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:335
> > +    static GtkBorder defaultInnerBorder = {1, 1, 1, 1};
>
> Hrm, is making this static useful/needed? It could be const, on the other hand.

I'll change this.

> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:365
> > +        getComboBoxSeparatorWidth() + (3 * buttonWidgetStyle->xthickness);
> 
> Hrm, so where does the 3 come from?

I'll leave a comment here referring to the one below.

[Martin Robinson]

Committed r76388: <http://trac.webkit.org/changeset/76388>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76388 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
editing/selection/select-box.html
transforms/2d/zoom-menulist.html