[GTK] A MenuList should look like a GtkComboBox

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

Currently the MenuList is only a box. It should look like a GtkComboBox.

[Christian Dywan]

Created attachment 16918 [details]
Try to make a MenuList look like a GtkComboBox

This is not quite a GtkComboBox, very similar to it. I don't know how close the match can be as themes render it differently.

[Alp Toker]

I like the idea here but it doesn't look quite right yet (as you explained yourself).

I've been seriously thinking about porting http://lxr.mozilla.org/seamonkey/source/widget/src/gtk2/gtk2drawing.c wholesale, as Mozilla does a pretty good job of this, and we might spend months just discovering and working around the same quirks they did.

Can you take a look at that and see if it's feasible, Christian? I'd value your opinion here.

[Alp Toker]

Comment on attachment 16918 [details]
Try to make a MenuList look like a GtkComboBox

This doesn't match GTK+'s style well enough.

This shows how difficult it is to get right, so I've started work to port an existing solution to WebKit/GTK+.

[Alp Toker]

I've done the work to integrate gtkdrawing, will submit a patch soon.

[Alp Toker]

Created attachment 16970 [details]
New RenderThemeGtk implementation (code shared with Mozilla)

RenderThemeGtk.cpp is a complete rewrite; any matching lines in the diff are coincidence or have a shared source from another port.

It now uses code shared with Mozilla to do the dirty work, since the GTK+ style API contains a lot of quirks which would take a long time to re-discover.

Coding style for the borrowed code should not be modified or reformatted.

I do not know if we need to change the copyright header of the Mozilla sources to plain LGPL or not, but it would be nice to be able to continue sharing it under the original license. Thoughts?

[Alp Toker]

Created attachment 16971 [details]
New RenderThemeGtk implementation (code shared with Mozilla)

This is the actual patch. See comments of previous attachment.

Note also that the code is heavily TODO'd to avoid the assumption from contributors that I knew what I was doing when I wrote those parts, and especially to avoid having them copied into other ports until they are properly implemented.

This is just the start of this feature, but I expect others will be keen to contribute once it's in.

[Christian Dywan]

(In reply to comment #6) 
> This is just the start of this feature, but I expect others will be keen to
> contribute once it's in.

This is very nice and reusing mozilla code is better than starting from scratch.

What I don't really like is the GtkOptionMenu. It is deprecated since Gtk+ 2.4 and there should be a good reason to not use GtkComboBox in my opinion. If there is such a reason please add an appropriate comment in the code.

I like it that you already implemented the search control. I had code for this lying around as well. I wonder wether we should use an actual "find" icon like the mac port does, as the function of the dropdown button is not very obvious.

I agree that the remaining issues can be fixed in separate bugs, which will probably be combobox popup menu placement, check/ radio prelight, unwanted control background (Clearlooks), range control and file control.

[Adam Roben (:aroben)]

Comment on attachment 16971 [details]
New RenderThemeGtk implementation (code shared with Mozilla)

It looks like in many of the adjust*Style methods you are no longer allowing web developers to style form controls at all, where the old code allowed for at least limited styling. Are you sure this is the direction the GTK port should go in? In the Mac/Windows ports there are heuristics to determine whether the native look should be used or a more generic, styled look should be used for each control, which gives a nice balance between native look-and-feel and stylability. Maybe that's something you want to emulate?

+    // TODO: This strategy is possibly incorrect for the GTK+ port.

Would you mind changing all your TODOs to FIXMEs?

+static void setMozState(RenderTheme* theme, GtkWidgetState* state, RenderObject* o)

It would be slightly nicer for the second paramater to be a GtkWidgetState&, but it's not a big deal.
+    //state->active = theme->isChecked(o);

I think this commented-out code should be removed.

+    // It could be made a configuration option if 13 actually breaks site compatibility.

I think you mean "...if sizes other than 13 actually break..."

+void RenderThemeGtk::adjustSearchFieldResultsButtonStyle(CSSStyleSelector* selector, RenderStyle* style, Element* e) const
+{
+    adjustSearchFieldCancelButtonStyle(selector, style, e);
+}

This seems a little strange to me, but I suppose you're just saying that the cancel and results buttons should have the same style? It's a little confusing, though, since adjustSearchFieldResultsDecorationStyle doesn't call through to adjustSearchFieldCancelButtonStyle.

+bool RenderThemeGtk::paintSearchFieldResultsDecoration(RenderObject* o, const RenderObject::PaintInfo& i, const IntRect& rect)
+{
+    return paintSearchFieldCancelButton(o, i, rect);
+}

I'm very confused by this. On Mac/Windows, the results button and the results decoration are both magnifying glasses. The difference is that the results button has a little downward-pointing arrow that indicates it will show a menu when you click on it. I don't think you want to show a cancel button where the results decoration goes, because then you'd have cancel buttons on both sides of the control, but one wouldn't do anything.

+static void gtkStyleSetCb(GtkWidget* widget, GtkStyle* previous, RenderTheme* renderTheme)

Can you replace "Cb" with "Callback"?

r=me, though I do think you should consider the issue of stylability. You could land the code as-is and file a bug about it.

[Alp Toker]

Landed in r27398.