Bug 27546 - [GTK] context menu overriding API is very limited
Summary: [GTK] context menu overriding API is very limited
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Christian Dywan
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-07-22 10:10 PDT by Gustavo Noronha (kov)
Modified: 2010-01-07 12:55 PST (History)
5 users (show)

See Also:


Attachments
Rewrite of context menu handling using GtkUIManager (30.80 KB, patch)
2009-07-22 10:12 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Second draft (49.28 KB, patch)
2009-08-25 20:07 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Epiphany patch demonstrating API (23.51 KB, patch)
2009-08-25 20:08 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
targetinfo.patch (17.20 KB, patch)
2009-09-17 02:54 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
gettargetinfo.patch (11.73 KB, patch)
2009-09-17 02:56 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
gettargetinfo.patch (11.75 KB, patch)
2009-09-17 06:18 PDT, Xan Lopez
gustavo: commit-queue-
Details | Formatted Diff | Diff
gettargetinfo.patch (12.04 KB, patch)
2009-09-17 08:56 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Conditionalize font menu item, omit spell check (1.60 KB, patch)
2009-09-26 15:13 PDT, Christian Dywan
abarth: review-
Details | Formatted Diff | Diff
Add "frame" property to HitTestResult (3.73 KB, patch)
2009-09-26 17:07 PDT, Christian Dywan
xan.lopez: review-
Details | Formatted Diff | Diff
Omit the SearchWeb item in the GTK+ port (1.04 KB, patch)
2009-12-18 04:39 PST, Christian Dywan
gustavo: review+
gustavo: commit-queue-
Details | Formatted Diff | Diff
Omit the SearchWeb item in the GTK+ port #2 (1.07 KB, patch)
2009-12-18 05:31 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Omit the SearchWeb item in the GTK+ port #3 (2.06 KB, patch)
2009-12-18 05:52 PST, Christian Dywan
xan.lopez: review+
Details | Formatted Diff | Diff
Only show the Font menu item in rich text areas (1.93 KB, patch)
2009-12-18 13:17 PST, Christian Dywan
gustavo: review+
gustavo: commit-queue-
Details | Formatted Diff | Diff
Don't show spell check menu items in the GTK+ port (1.39 KB, patch)
2009-12-18 13:45 PST, Christian Dywan
gustavo: review+
gustavo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2009-07-22 10:10:55 PDT
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.
Comment 1 Gustavo Noronha (kov) 2009-07-22 10:12:54 PDT
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(-)
Comment 2 Gustavo Noronha (kov) 2009-07-22 10:16:35 PDT
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?
Comment 3 Christian Dywan 2009-08-03 13:52:09 PDT
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.
Comment 4 Gustavo Noronha (kov) 2009-08-25 20:07:07 PDT
Created attachment 38589 [details]
Second draft

A bit more, trying out possible APIs. A WebTargetInfo object appears =P. Epiphany patch using this incoming.
Comment 5 Gustavo Noronha (kov) 2009-08-25 20:08:40 PDT
Created attachment 38590 [details]
Epiphany patch demonstrating API

Hrm. It half-works, even.
Comment 6 Christian Dywan 2009-09-03 10:51:54 PDT
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.
Comment 7 Christian Dywan 2009-09-03 11:18:36 PDT
I wonder if we should expose the ID of the underlying element, so it could be used with javascript, or at some point GDOM.
Comment 8 Xan Lopez 2009-09-17 02:54:34 PDT
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
Comment 9 Xan Lopez 2009-09-17 02:56:22 PDT
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).
Comment 10 Xan Lopez 2009-09-17 06:18:17 PDT
Created attachment 39691 [details]
gettargetinfo.patch

Small fix, we have to use the focused frame if there's one to do the hit test.
Comment 11 Gustavo Noronha (kov) 2009-09-17 07:45:20 PDT
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+.
Comment 12 Gustavo Noronha (kov) 2009-09-17 07:47:12 PDT
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.
Comment 13 Xan Lopez 2009-09-17 08:56:33 PDT
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.
Comment 14 Jan Alonzo 2009-09-18 04:42:07 PDT
Comment on attachment 39687 [details]
targetinfo.patch

+1. r=me.
Comment 15 Jan Alonzo 2009-09-18 04:49:16 PDT
(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?
Comment 16 Xan Lopez 2009-09-18 04:52:27 PDT
(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?
Comment 17 Jan Alonzo 2009-09-18 06:56:42 PDT
Comment on attachment 39696 [details]
gettargetinfo.patch

r=me with the naming changes.
Comment 18 Xan Lopez 2009-09-18 06:57:11 PDT
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.
Comment 19 Xan Lopez 2009-09-18 06:57:50 PDT
Comment on attachment 39696 [details]
gettargetinfo.patch

I committed this with the renaming changes in r48506.
Comment 20 Christian Dywan 2009-09-19 05:17:46 PDT
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.
Comment 21 Christian Dywan 2009-09-20 17:26:02 PDT
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.
Comment 22 Christian Dywan 2009-09-26 15:13:31 PDT
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.
Comment 23 Christian Dywan 2009-09-26 17:07:59 PDT
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.
Comment 24 Xan Lopez 2009-09-28 01:07:15 PDT
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.
Comment 25 Gustavo Noronha (kov) 2009-10-04 03:46:35 PDT
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+.
Comment 26 Christian Dywan 2009-10-04 10:35:52 PDT
> 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.
Comment 27 Adam Barth 2009-10-13 15:09:12 PDT
Comment on attachment 40181 [details]
Conditionalize font menu item, omit spell check

No ChangeLog.
Comment 28 Christian Dywan 2009-10-13 17:19:34 PDT
"No ChangeLog" doesn't feel like the result of a diligent review. I hope Xan reads the bug change notification in any case.
Comment 29 Christian Dywan 2009-12-18 04:39:20 PST
Created attachment 45144 [details]
Omit the SearchWeb item in the GTK+ port

This patch only removes the Search Web item.
Comment 30 WebKit Review Bot 2009-12-18 04:42:02 PST
style-queue ran check-webkit-style on attachment 45144 [details] without any errors.
Comment 31 Gustavo Noronha (kov) 2009-12-18 05:27:23 PST
Comment on attachment 45144 [details]
Omit the SearchWeb item in the GTK+ port

It's all wrong (the bug title and URL). Otherwise fine.
Comment 32 Christian Dywan 2009-12-18 05:31:26 PST
Created attachment 45145 [details]
Omit the SearchWeb item in the GTK+ port #2
Comment 33 WebKit Review Bot 2009-12-18 05:32:49 PST
style-queue ran check-webkit-style on attachment 45145 [details] without any errors.
Comment 34 Christian Dywan 2009-12-18 05:52:20 PST
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.
Comment 35 WebKit Review Bot 2009-12-18 05:53:21 PST
style-queue ran check-webkit-style on attachment 45146 [details] without any errors.
Comment 36 Xan Lopez 2009-12-18 12:22:01 PST
Comment on attachment 45146 [details]
Omit the SearchWeb item in the GTK+ port #3

r=me
Comment 37 Christian Dywan 2009-12-18 12:43:59 PST
Comment on attachment 45146 [details]
Omit the SearchWeb item in the GTK+ port #3

Committed.
Comment 38 Christian Dywan 2009-12-18 13:17:36 PST
Created attachment 45177 [details]
Only show the Font menu item in rich text areas
Comment 39 WebKit Review Bot 2009-12-18 13:20:11 PST
style-queue ran check-webkit-style on attachment 45177 [details] without any errors.
Comment 40 Christian Dywan 2009-12-18 13:45:05 PST
Created attachment 45182 [details]
Don't show spell check menu items in the GTK+ port
Comment 41 WebKit Review Bot 2009-12-18 13:46:54 PST
style-queue ran check-webkit-style on attachment 45182 [details] without any errors.
Comment 42 Gustavo Noronha (kov) 2009-12-28 06:15:30 PST
Comment on attachment 45177 [details]
Only show the Font menu item in rich text areas

Nice.
Comment 43 Eric Seidel (no email) 2009-12-28 22:38:02 PST
Attachment 45182 [details] was posted by a committer and has review+, assigning to Christian Dywan for commit.
Comment 44 Christian Dywan 2009-12-31 12:00:09 PST
Comment on attachment 45182 [details]
Don't show spell check menu items in the GTK+ port

2009-12-31  Christian Dywan  <christian@twotoasts.de>

        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):
Comment 45 Christian Dywan 2009-12-31 12:04:18 PST
Comment on attachment 45177 [details]
Only show the Font menu item in rich text areas

2009-12-31  Christian Dywan  <christian@twotoasts.de>

        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):
Comment 46 Eric Seidel (no email) 2010-01-06 23:35:43 PST
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.
Comment 47 Gustavo Noronha (kov) 2010-01-07 12:55:15 PST
(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.