[GTK] context menu overriding API is very limited

[Gustavo Noronha (kov)]

Currently the API that allows applications to override WebKit's context menu is a 'populate-popup' signal that gives applications a GtkMenu. That is not nearly enough information to decide what should go in the menu, and also lacks proper ways to override only some parts of the menu.

What is needed is an API that tells the application what exactly was right-clicked, and allows the application to add/remove entries as it sees fit, in a sane way. That could be achieved by using GtkUIManager, so a good first step is to port the current context menu handling to use GtkUIManager.

[Gustavo Noronha (kov)]

Created attachment 33267 [details]
Rewrite of context menu handling using GtkUIManager

 WebCore/platform/ContextMenu.h                     |    6 +
 WebCore/platform/ContextMenuItem.h                 |    9 +-
 WebCore/platform/PlatformMenuDescription.h         |    4 +-
 WebCore/platform/gtk/ContextMenuGtk.cpp            |  142 +++++++++++-
 WebCore/platform/gtk/ContextMenuItemGtk.cpp        |  241 +++++++++++++-------
 WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp |  112 +---------
 WebKit/gtk/webkit/webkitwebview.cpp                |   83 +++++++-
 7 files changed, 385 insertions(+), 212 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 33267 [details]
Rewrite of context menu handling using GtkUIManager

This is very much work in progress, but seems to already provide 99%-identical behavior. I tested Midori which does various magics with the current menu, and I get this, when I right click an image, although the menus seem to be working OK:

(midori:636): Gtk-CRITICAL **: gtk_label_set_label: assertion `GTK_IS_LABEL (label)' failed

I am posting the patch to gather comments on the implementation, and would like to read opinions on what kind of API we would provide here. I guess an object which passes along information from the HitTest result object, along with the GtkUIManager, and what else?

[Christian Dywan]

There's a condition which triggers this warning in Midori in the current code, because it's so hard to customize the menu. You can safely ignore it.

I only had a quick look yet, but this is looking interesting. I will read the code more closely in a bit.

First thoughts: Did you consider an array of GtkActionEntry instead of the case statement? It would have the benefit that we can define labels, action names and stock ID's in the same place.

And the gtk_widget_show_all before swhowing the menu looks wrong, applications should be able to rely on the visibility.

[Gustavo Noronha (kov)]

Created attachment 38589 [details]
Second draft

A bit more, trying out possible APIs. A WebTargetInfo object appears =P. Epiphany patch using this incoming.

[Gustavo Noronha (kov)]

Created attachment 38590 [details]
Epiphany patch demonstrating API

Hrm. It half-works, even.

[Christian Dywan]

Taking notes of what I notice when reading the patch:

- Can we list somewhere what  WEBKIT_WEB_TARGET_INFO_CONTEXT_MEDIA can be?

- WEBKIT_WEB_TARGET_INFO_CONTEXT_EDITABLE
We probably need to store the text in the target info?

- WEBKIT_WEB_TARGET_INFO_CONTEXT_SELECTION
We have no "public" way to obtain the selection. webkit_web_view_get_selected_text is there but semi-private.

- You're not freeing all strings in webkit_web_target_info_finalize

- I wonder if "WebKitCopyLinkToClipboard" is necessary or redundant. Can this conflict in practical use?

+    GtkAction* gtkAction = ContextMenuItem::createNativeMenuItem(platformItem);
+    if (!gtkAction) {
+        g_warning("Item not added: %s", platformItem.title.utf8().data());
+        return;
+    }

I think this should say "Failed to create menu item for action: %s".

The example is nice. I should try to rewrite the menu in Midori and see how it goes, also regarding hiding and adding actions.

[Christian Dywan]

I wonder if we should expose the ID of the underlying element, so it could be used with javascript, or at some point GDOM.

[Xan Lopez]

Created attachment 39687 [details]
targetinfo.patch

In the spirit of doing this piece by piece, I've split the WebKitWebTargetInfo stuff into a new patch. I've removed the 'Web' from its name, since I believe that's only useful when there can be confusions arising from too generic names (like 'View'), but that should not happen here. It also comes with other random fixes/cleanups

[Xan Lopez]

Created attachment 39689 [details]
gettargetinfo.patch

And on top of the previous patch, a small API addition to get the target info for a point in the view. This is needed in Epiphany in multiple places where we want to decide what to do depending on whether the user clicked on an image, input, nothing, etc. I tried to do some tests for it, but it's pretty difficult to go beyond the most basic stuff (small explanation in the test code itself).

[Xan Lopez]

Created attachment 39691 [details]
gettargetinfo.patch

Small fix, we have to use the focused frame if there's one to do the hit test.

[Gustavo Noronha (kov)]

Comment on attachment 39691 [details]
gettargetinfo.patch

> +typedef struct {
> +  char *data;

Misplaced *

> +} TargetInfoFixture;;

Double ;

> +WEBKIT_API WebKitTargetInfo*
> +webkit_web_view_get_target_info                 (WebKitWebView        *webView,
> +                                                 double                x,
> +                                                 double                y);

I think we should use gdouble here and at the function documentation, for consistency? Other than that, you got my 1/2 r+.

[Gustavo Noronha (kov)]

Comment on attachment 39687 [details]
targetinfo.patch

This looks good to me. Adding more documentation regarding what a 'media' is, as suggested by kalikiana would be good. My 1/2 r+ with that doc improvement.

[Xan Lopez]

Created attachment 39696 [details]
gettargetinfo.patch

So, the previous gettargetinfo patch was also wrong, we need to be more careful with the coordinate transformations. I'm now using the same code EventHandler uses, which obviously was the best idea to begin with.

[Jan Alonzo]

Comment on attachment 39687 [details]
targetinfo.patch

+1. r=me.

[Jan Alonzo]

(In reply to comment #13)
> Created an attachment (id=39696) [details]
> gettargetinfo.patch
> 
> So, the previous gettargetinfo patch was also wrong, we need to be more careful
> with the coordinate transformations. I'm now using the same code EventHandler
> uses, which obviously was the best idea to begin with.

We have WebKitWebViewTargetInfo as well. How are we going to distinguish these two?

[Xan Lopez]

(In reply to comment #15)
> (In reply to comment #13)
> > Created an attachment (id=39696) [details] [details]
> > gettargetinfo.patch
> > 
> > So, the previous gettargetinfo patch was also wrong, we need to be more careful
> > with the coordinate transformations. I'm now using the same code EventHandler
> > uses, which obviously was the best idea to begin with.
> 
> We have WebKitWebViewTargetInfo as well. How are we going to distinguish these
> two?

You are right (and I blame kov for picking WebKitWebTargetInfo in the first place!). What about WebKitEventContext?

[Jan Alonzo]

Comment on attachment 39696 [details]
gettargetinfo.patch

r=me with the naming changes.

[Xan Lopez]

Comment on attachment 39687 [details]
targetinfo.patch

I committed this in r48505 renaming the object to WebKitHitTestResult, since it's just a wrapper for the core object.

[Xan Lopez]

Comment on attachment 39696 [details]
gettargetinfo.patch

I committed this with the renaming changes in r48506.

[Christian Dywan]

I tried the new hit test result in Midori. So I can reliably identify editables, images and videos without guessing from context menu items, so I ended up replacing the stock menu except for the editable case.

The editable menu can't be replicated because there is font, spelling and input method/ unicode character items. And thus I'd like to improve those a bit:
- Font is always there, disabled. I suggested it's omitted if not used.
- Spelling is always there, even if disabled. I suggest to omit it if spelling is disabled.
- Web Search is there, non-functional. Imho it looks out of place and makes the menu look cluttered. I think we should remove it there (and separately think about how to make the selection menu Web Search useful).

I am left unsure what the DOCUMENT flag is good for, it seems to be set always, from a bit of testing. I would have assumed "document, as in not a link, image or anything interactive" but that's not what it is.

[Christian Dywan]

It is also impossible to replicate the Inspect page menu item. Time to resolve bug 22551.

And there is no way to replicate Open frame in new Tab/ Window. I think we need a "frame" property in WebKitHitTestResult so we know what frame is under the mouse.

[Christian Dywan]

Created attachment 40181 [details]
Conditionalize font menu item, omit spell check

This patch changes the context menu in this way:

- "Web Search" is never shown in editables.
- "Font" is only shown if there is rich text editing.
- "Spell checking" is never shown.
I originally thought of conditionalizing it but now I think it's not useful since it basically only provides options that belong in a Preferences dialogue. At least that's what I found.

[Christian Dywan]

Created attachment 40186 [details]
Add "frame" property to HitTestResult

This change adds a "frame" property to WebKitHitTestResult. Note that this is *not* the same as the targetFrame() of WebCore::HitTestResult.

[Xan Lopez]

Comment on attachment 40186 [details]
Add "frame" property to HitTestResult

Can you add a simple test for this to the unit testing? Also, in the doc 'frame' should be #WebKitWebFrame I guess.

[Gustavo Noronha (kov)]

Comment on attachment 40181 [details]
Conditionalize font menu item, omit spell check

>  #endif
> +#if !PLATFORM(GTK)
>              appendItem(SearchWebItem);
>              appendItem(*separatorItem());
> -     
> +#endif

I think this makes sense, since we don't have an implementation for that, and I don't see it happening in the near future.

>          if (!inPasswordField) {
> +#if PLATFORM(GTK)
> +            if (frame->editor()->canEditRichly()) {
> +                appendItem(*separatorItem());
> +                ContextMenuItem  FontMenuItem(SubmenuType, ContextMenuItemTagFontMenu, 
> +                    contextMenuItemTagFontMenu());
> +                createAndAppendFontSubMenu(m_hitTestResult, FontMenuItem);
> +                appendItem(FontMenuItem);
> +            }
> +#else


Conditionally displaying the font menu also sounds good, but I am not so sure about the spelling check one. What you're saying is that this UI should be the browser's responsibility, alone? I'd like to hear what Xan thinks here before giving you r+.

[Christian Dywan]

> Conditionally displaying the font menu also sounds good, but I am not so
> sure about the spelling check one. What you're saying is that this UI should
> be the browser's responsibility, alone

Yes. I played around with the spelling items and at least to me they seemed redundant. Midori has a preference to enable or disable spell check, and a way to choose the languages.

[Adam Barth]

Comment on attachment 40181 [details]
Conditionalize font menu item, omit spell check

No ChangeLog.

[Christian Dywan]

"No ChangeLog" doesn't feel like the result of a diligent review. I hope Xan reads the bug change notification in any case.

[Christian Dywan]

Created attachment 45144 [details]
Omit the SearchWeb item in the GTK+ port

This patch only removes the Search Web item.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45144 [details] without any errors.

[Gustavo Noronha (kov)]

Comment on attachment 45144 [details]
Omit the SearchWeb item in the GTK+ port

It's all wrong (the bug title and URL). Otherwise fine.

[Christian Dywan]

Created attachment 45145 [details]
Omit the SearchWeb item in the GTK+ port #2

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45145 [details] without any errors.

[Christian Dywan]

Created attachment 45146 [details]
Omit the SearchWeb item in the GTK+ port #3

Third iteration, to actually omit both items. And I made sure the SearchWebItem is not defined for Gtk so it will fail compile if the code is changed in the future to use it.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45146 [details] without any errors.

[Xan Lopez]

Comment on attachment 45146 [details]
Omit the SearchWeb item in the GTK+ port #3

r=me

[Christian Dywan]

Comment on attachment 45146 [details]
Omit the SearchWeb item in the GTK+ port #3

Committed.

[Christian Dywan]

Created attachment 45177 [details]
Only show the Font menu item in rich text areas

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45177 [details] without any errors.

[Christian Dywan]

Created attachment 45182 [details]
Don't show spell check menu items in the GTK+ port

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45182 [details] without any errors.

[Gustavo Noronha (kov)]

Comment on attachment 45177 [details]
Only show the Font menu item in rich text areas

Nice.

[Eric Seidel (no email)]

Attachment 45182 [details] was posted by a committer and has review+, assigning to Christian Dywan for commit.

[Christian Dywan]

Comment on attachment 45182 [details]
Don't show spell check menu items in the GTK+ port

2009-12-31  Christian Dywan  <[e-mail]>

        Reviewed by Gustavo Noronha Silva.

        [GTK] context menu overriding API is very limited
        https://bugs.webkit.org/show_bug.cgi?id=27546

        Don't show spell checking menu items in text areas in the GTK+ port.

        * platform/ContextMenu.cpp:
        (WebCore::ContextMenu::populate):

[Christian Dywan]

Comment on attachment 45177 [details]
Only show the Font menu item in rich text areas

2009-12-31  Christian Dywan  <[e-mail]>

        Reviewed by Gustavo Noronha Silva.

        [GTK] context menu overriding API is very limited
        https://bugs.webkit.org/show_bug.cgi?id=27546

        Only show font menu items in rich text areas in the GTK+ port.

        * platform/ContextMenu.cpp:
        (WebCore::ContextMenu::populate):

[Eric Seidel (no email)]

Should this still be open?  It looks like all the reviewed patches have been landed.

If there is more work to do here, I suggest we open a new bug.

[Gustavo Noronha (kov)]

(In reply to comment #46)
> Should this still be open?  It looks like all the reviewed patches have been
> landed.
> 
> If there is more work to do here, I suggest we open a new bug.

I think that's a good idea.