[GTK] Add API to allow overriding popup menus

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Our API uses GTK+ as the default implementation of several UI elements like the file chooser, print dialog, color picker, context menu, etc. but in all those cases it allows to override the default implementation. It's not possible to override the popup menu used for select elements.

[Carlos Garcia Campos]

Created attachment 311960 [details]
Patch

[Build Bot]

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


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenu.h"
ERROR: Source/WebKit2/PlatformGTK.cmake:222:  Alphabetical sorting problem. "UIProcess/API/gtk/WebKitOptionMenuItem.cpp" should be before "UIProcess/API/gtk/WebKitOptionMenuPrivate.h".  [list/order] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitPopupMenu.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 23 files


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

[Carlos Garcia Campos]

Michael?

[Michael Catanzaro]

I've been meaning to review your combo box patch for a while but haven't gotten to it yet... can you briefly describe the advantages to using your custom treeview implementation over what we have now? They look similar to me.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> I've been meaning to review your combo box patch for a while but haven't
> gotten to it yet... can you briefly describe the advantages to using your
> custom treeview implementation over what we have now? They look similar to
> me.

You are mixing bugs here. This is the WebKit bug to provide API to override popup menus. We needs this to allow users to override the default GTK+ menu, but not because our current implementation is not good enough. Popup menus is the only thing where we expose GTK+ unconditionally, so it has always been in my TODO, to provide API to be able to override it.

To make sure the API is good to implement your own menu, I implemented a new custom menu in epiphany using the new API, not because it has any advantages, but just to try the API. However, it indeed has advantages:

 - It scales much better, because it has a maximum size and context are scrollable, instead of the current menu that takes the whole work area and has scroll button on top/bottom.

 - It uses a tree view which allows us to better format group labels, making them bold instead of current menu that just makes them insensitive, like any other disabled item.

 - It behaves like other browsers (chromium/firefox). When you hover the items, they are selected in the tree view, but the combo text doesn't change. In case of typeahead find, the matched element is selected in the tree view and the combo box text is also updated.

All this is specially noticeable with long menus, that's why I included a screenshot of the redhat bugzilla products menu, just open that menu in you current ephy to see the differences.

As I said in ephy bug, there's actually no reason why that implementation couldn't be the default, but I think we can test it for a while this cycle in ephy, and move it to WebKit during the next cycle if we all are happy with it.

I'm sorry this was not briefly in the end.

[Michael Catanzaro]

Comment on attachment 311960 [details]
Patch

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

Nice patch! Here is the first half of your review. I still need to review the second half of the patch.

I don't think I like the names you chose here. The terminology used in GTK+ is combo box. The terminology used in HTML is select element, though I suppose that refers to the entire element and not just the menu. You could have named it, for example, WebKitSelectMenu with WebKitWebView::show-select-menu, but you've chosen WebKitOptionMenu, which matches neither GTK+ nor HTML. What are your thoughts on this? Perhaps it needs to be discussed on the mailing list.

> Source/WebKit2/ChangeLog:12
> +        contains WebKitOptionMenuItem elements representng the items to be displayed.

representing

> Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenu.h:29
> +#include <webkit2/WebKitOptionMenuItem.h>
> +#include <webkit2/WebKitDefines.h>

Alphabetize

> Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:27
> +G_DEFINE_BOXED_TYPE(WebKitOptionMenuItem, webkit_option_menu_item, webkit_option_menu_item_copy, webkit_option_menu_item_free)

You're missing all the class documentation for WebKitOptionMenuItem. I think that's important, even if it doesn't need to be a lot for such a simple class.

> Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:102
> + * Whether a#WebKitOptionMenuItem is a group label.

Missing space

> Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:119
> + * Whether a#WebKitOptionMenuItem is a group child.

Missing space

> Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:136
> + * Whether a#WebKitOptionMenuItem is enabled.

Missing space

> Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:153
> + * Whether a#WebKitOptionMenuItem is the currently selected one.

Missing space

> Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItemPrivate.h:32
> +        , isGroupChild(item.m_text.startsWith("    "))

This seems really fragile. I know this is probably hard, but please find a way to determine if it's a group child that doesn't depend on the amount of whitespace in a user-visible string.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 311960 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311960&action=review
> 
> Nice patch! Here is the first half of your review. I still need to review
> the second half of the patch.
> 
> I don't think I like the names you chose here. The terminology used in GTK+
> is combo box. The terminology used in HTML is select element, though I
> suppose that refers to the entire element and not just the menu. You could
> have named it, for example, WebKitSelectMenu with
> WebKitWebView::show-select-menu, but you've chosen WebKitOptionMenu, which
> matches neither GTK+ nor HTML. What are your thoughts on this? Perhaps it
> needs to be discussed on the mailing list.

Yes, this is the dropdown menu part of the combobox. I used option menu on purpose, as a more generic name, just in case we ever implement datalist and maybe we could reuse this same class. But I'm open to any change in the names in any case.

> > Source/WebKit2/ChangeLog:12
> > +        contains WebKitOptionMenuItem elements representng the items to be displayed.
> 
> representing
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenu.h:29
> > +#include <webkit2/WebKitOptionMenuItem.h>
> > +#include <webkit2/WebKitDefines.h>
> 
> Alphabetize
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:27
> > +G_DEFINE_BOXED_TYPE(WebKitOptionMenuItem, webkit_option_menu_item, webkit_option_menu_item_copy, webkit_option_menu_item_free)
> 
> You're missing all the class documentation for WebKitOptionMenuItem. I think
> that's important, even if it doesn't need to be a lot for such a simple
> class.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:102
> > + * Whether a#WebKitOptionMenuItem is a group label.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:119
> > + * Whether a#WebKitOptionMenuItem is a group child.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:136
> > + * Whether a#WebKitOptionMenuItem is enabled.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItem.cpp:153
> > + * Whether a#WebKitOptionMenuItem is the currently selected one.
> 
> Missing space
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitOptionMenuItemPrivate.h:32
> > +        , isGroupChild(item.m_text.startsWith("    "))
> 
> This seems really fragile. I know this is probably hard, but please find a
> way to determine if it's a group child that doesn't depend on the amount of
> whitespace in a user-visible string.

I don't know a batter way. We have unit tests checking this, anyway, so a change in this would be caught by the tests.

[Michael Catanzaro]

Comment on attachment 311960 [details]
Patch

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

Nice work, as usual.

> Source/WebKit2/UIProcess/API/gtk/WebKitPopupMenu.h:29
> +class WebKitPopupMenu final : public WebPopupMenuProxyGtk {

I'm confused by your decision to make a subclass of WebPopupMenuProxyGtk, and especially by your decision to not include "Proxy" in the name of the class. It's quite unclear that this object is a UI proxy for a web process object. Can you elaborate on why you chose this design?

Would it be better for WebKitPopupMenu to contain a WebPopupMenuProxyGtk member variable rather than to inherit from it, as I suspect, or would this require too much extra code and indirection?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:-256
> -    void (*_webkit_reserved3) (void);

Running out of space for more virtual signals... I think we'll be hitting our limit within the next couple of years. Oh well.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 311960 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311960&action=review
> 
> Nice work, as usual.

Thanks.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPopupMenu.h:29
> > +class WebKitPopupMenu final : public WebPopupMenuProxyGtk {
> 
> I'm confused by your decision to make a subclass of WebPopupMenuProxyGtk,
> and especially by your decision to not include "Proxy" in the name of the
> class. It's quite unclear that this object is a UI proxy for a web process
> object. Can you elaborate on why you chose this design?

I just followed what we already do for the context menu, I don't remember why i did it that way for the context menu, though.

> Would it be better for WebKitPopupMenu to contain a WebPopupMenuProxyGtk
> member variable rather than to inherit from it, as I suspect, or would this
> require too much extra code and indirection?

I don't see why it would be better, but worse neither.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:-256
> > -    void (*_webkit_reserved3) (void);
> 
> Running out of space for more virtual signals... I think we'll be hitting
> our limit within the next couple of years. Oh well.

[Carlos Garcia Campos]

Committed r218325: <http://trac.webkit.org/changeset/218325>

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #9) 
> I don't see why it would be better, but worse neither.

It would make the inheritance less confusing.

Of course, matching the context menu is valuable, but perhaps that should be changed too.