Bug 51155

Summary: [GTK] Menulist text often collides with separator
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cgarcia, eric, gns, webkit.review.bot, xan.lopez
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 52385    
Bug Blocks: 52836    
Attachments:
Description Flags
Patch for this issue
none
Patch rebased to HEAD
none
Patch rebased against the latest
none
Patch fixing paintRect issue none

Description Martin Robinson 2010-12-15 17:12:44 PST
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.
Comment 1 Martin Robinson 2010-12-15 18:02:56 PST
Created attachment 76720 [details]
Patch for this issue
Comment 2 Martin Robinson 2011-01-18 10:12:29 PST
Created attachment 79291 [details]
Patch rebased to HEAD
Comment 3 Martin Robinson 2011-01-18 10:13:05 PST
(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.
Comment 4 WebKit Review Bot 2011-01-18 10:14:55 PST
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.
Comment 5 Martin Robinson 2011-01-18 11:15:51 PST
Removed the r? flag here, because I think this one deserves to go another round.
Comment 6 Martin Robinson 2011-01-18 13:56:45 PST
Created attachment 79323 [details]
Patch rebased against the latest
Comment 7 Carlos Garcia Campos 2011-01-20 00:45:23 PST
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.
Comment 8 Martin Robinson 2011-01-20 09:08:49 PST
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.
Comment 9 Martin Robinson 2011-01-20 10:08:57 PST
Created attachment 79610 [details]
Patch fixing paintRect issue
Comment 10 WebKit Review Bot 2011-01-20 10:10:51 PST
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.
Comment 11 Xan Lopez 2011-01-21 03:23:21 PST
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.
Comment 12 Carlos Garcia Campos 2011-01-21 03:56:11 PST
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
Comment 13 Martin Robinson 2011-01-21 12:15:27 PST
(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.
Comment 14 Martin Robinson 2011-01-21 13:35:45 PST
Committed r76388: <http://trac.webkit.org/changeset/76388>
Comment 15 WebKit Review Bot 2011-01-21 14:50:10 PST
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