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.
Created attachment 76720 [details] Patch for this issue
Created attachment 79291 [details] Patch rebased to HEAD
(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.
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.
Removed the r? flag here, because I think this one deserves to go another round.
Created attachment 79323 [details] Patch rebased against the latest
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 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.
Created attachment 79610 [details] Patch fixing paintRect issue
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 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 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
(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.
Committed r76388: <http://trac.webkit.org/changeset/76388>
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