[Webkit-unassigned] [Bug 49904] [GTK] Add a signal to allow applications to handle its own context menu

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49904





--- Comment #20 from Carlos Garcia Campos <cgarcia at igalia.com>  2010-12-17 00:10:29 PST ---
(In reply to comment #19)
> Created an attachment (id=76818)
 --> (https://bugs.webkit.org/attachment.cgi?id=76818&action=review) [details]
> Suggested changes to the patch

Bz doesn't even allow me to comment the patch, so I'll copy paste my review here.

>     GtkMenu* menu = GTK_MENU(coreMenu->platformDescription());
>+    g_object_ref(menu);
>     if (!menu)
>-        return FALSE;
>+        return;

why do you need to ref the menu?, also you are calling g_object_ref(), which is not null safe, before the if (!menu).


>+    PlatformMouseEvent event(location, 
>+                             global,
>+                             RightButton,
>+                             MouseEventPressed,
>+                             0, // clickCount
>+                             false, // shift
>+                             false, // ctrl
>+                             false, // alt
>+                             false, // meta
>+                             gtk_get_current_event_time());
>+
>+    gboolean result = frame->eventHandler()->handleMousePressEvent(event);
>+    webkit_web_view_forward_context_menu_event(WEBKIT_WEB_VIEW(widget), frame, event, true);
>+    return result;

how can you know whether the popup menu will be shown or not? this function should return TRUE when a menu was activated, webkit_web_view_forward_context_menu_event has to be boolean to know whether the menu has been handled or not.

> }
> 
> #ifndef GTK_API_VERSION_2
>@@ -853,14 +853,16 @@ static gboolean webkit_web_view_button_press_event(GtkWidget* widget, GdkEventBu
>     priv->previousClickButton = event->button;
>     priv->previousClickTime = eventTime;
> 
>-    if (event->button == 3)
>-        return webkit_web_view_forward_context_menu_event(webView, PlatformMouseEvent(event));
>-
>     Frame* frame = core(webView)->mainFrame();
>     if (!frame->view())
>         return FALSE;
>-
>     gboolean result = frame->eventHandler()->handleMousePressEvent(platformEvent);
>+
>+    if (event->button == 3) {
>+        webkit_web_view_forward_context_menu_event(webView, frame, PlatformMouseEvent(event), false);
>+        return result;

the same here, depending on whether you return TRUE or FALSE the event will be propagated or not, if the menu hasn't been activated we should return FALSE.


>     /*
>+     * WebKitWebView::context-menu
>+     * @webView: the object which received the signal
>+     * @x: the X coordinate of the position where the context menu should be shown
>+     * @y: the Y coordinate of the position where the context menu should be shown
>+     * @keyboard_mode: %TRUE if the context menu was trigged using the keyboard
>+     * @hit_test_result: a #WebKitHitTestResult with the context of the current position.
>+     * @menu: the default menu for the element this context menu corresponds to
>+     *
>+     * Emitted when a context menu is about to be displayed to give the application a
>+     * chance to create and handle its own context menu. If you only want to add custom
>+     * options to the default context menu you can simply modify the @menu signal
>+     * parameter.
>+     * 
>+     * When keyboard_mode is %TRUE the given coordinates should be used to position the
>+     * popup menu, when the context menu has been triggered by a mouse event you could
>+     * either use the given coordinates or pass %NULL to the #GtkMenuPositionFunc
>+     * parameter of gtk_menu_popup() function.
>+     *
>+     * When the signal is handled and a popup menu has been created by the application,
>+     * %TRUE should be returned. If %FALSE is returned, a popup menu will appears as long
>+     * as the #WebKitWebSettings::enable-default-context-menu setting is active. If you
>+     * don't want any context menu to be shown, you can simply connect to this signal and
>+     * return %TRUE without doing anything else.
>+     *
>+     * Since: 1.3.8
>+     */
>+    webkit_web_view_signals[CONTEXT_MENU] = g_signal_new("context-menu",
>+            G_TYPE_FROM_CLASS(webViewClass),
>+            (GSignalFlags)G_SIGNAL_RUN_LAST,
>+            0,
>+            0, 0,
>+            webkit_marshal_BOOLEAN__INT_INT_BOOLEAN_OBJECT_OBJECT,
>+            G_TYPE_BOOLEAN, 5,
>+            G_TYPE_INT, G_TYPE_INT, G_TYPE_BOOLEAN,
>+            WEBKIT_TYPE_HIT_TEST_RESULT, GTK_TYPE_MENU);

The signal is still confusing to me, when the user has its own context menu, the menu parameter should be ignored, and when the user wants to add stuff to the menu, x, y, and keyboard_mode params are useless. I still see two different signals for two different use cases (create your own menu, add items to the default one). 
And we still have the problem of how the user knows whether the given menu already has the items he/she wants to add. I agree it would be useful to be able to build a context menu in the application with some options (or submenus) from webkit, like the input method or spell checker submenus, but providing a way to let the users create their own menus rather than modifying the default one.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.