Bug 182224 - REGRESSION(r227544): [GTK] contextMenuEvent is NULL on CONTEXT_MENU call
Summary: REGRESSION(r227544): [GTK] contextMenuEvent is NULL on CONTEXT_MENU call
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 182381
Blocks:
  Show dependency treegraph
 
Reported: 2018-01-28 11:56 PST by Jan-Michael Brummer
Modified: 2018-02-01 05:07 PST (History)
6 users (show)

See Also:


Attachments
Patch fixing contextMenuEvent (1.76 KB, patch)
2018-01-28 11:59 PST, Jan-Michael Brummer
mcatanzaro: review-
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Patch (34.67 KB, patch)
2018-01-30 02:02 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan-Michael Brummer 2018-01-28 11:56:34 PST
After adding support for long press context menu epiphany crashes due to contextMenuEvent not beeing set (only called in button-press handler).
Comment 1 Jan-Michael Brummer 2018-01-28 11:59:08 PST
Created attachment 332492 [details]
Patch fixing contextMenuEvent
Comment 2 Build Bot 2018-01-28 11:59:52 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Michael Catanzaro 2018-01-28 14:18:49 PST
Comment on attachment 332492 [details]
Patch fixing contextMenuEvent

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

We need to update the documentation of WebKitWebView::context-menu as well, to indicate that event may now be a GdkEventTouch of type GDK_TOUCH_END. (It would be GDK_TOUCH_END, right?)

Then I'll ask Carlos Garcia to do the final review, because this could in theory break applications depending on the assumptions they make about the type of the GdkEvent... I'd like to do this anyway, since I think it will be fine in practice.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:936
> +        priv->contextMenuEvent.reset(gdk_event_copy(touchEvent));

Ah, that was easier than I expected.
Comment 4 Carlos Garcia Campos 2018-01-28 23:27:25 PST
Comment on attachment 332492 [details]
Patch fixing contextMenuEvent

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

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:936
>> +        priv->contextMenuEvent.reset(gdk_event_copy(touchEvent));
> 
> Ah, that was easier than I expected.

I'm not sure this is the right fix, though. I was surprised we were missing the event, while we were simulating the click, but then I realized that GestureController is bypassing the WebKitWebViewBase. This means we are not doing the same in both cases. For example, a tap is not focusing the view, while a real button press is. In case of gestures we are not notifying the input method filter either. I think the event handling should be done by WebKitWebViewBase to ensure consistency. I would add a GestureControllerClient, implemented by a helper class in WebKitWebViewBase that has methods like handleTap(), handleDrag(), handleLongPress(), etc, that receive the touch event and any other ting required to handle it. Then the client decides how to handle that, doing the conversion and sending the events to the page. If I'm asking too much, I can do it if you prefer.
Comment 5 Jan-Michael Brummer 2018-01-29 05:09:59 PST
Please proceed, as my vacation is over and i would love to see this feature in a stable release soon :)

I can of course test the changes.
Comment 6 Carlos Garcia Campos 2018-01-30 02:02:34 PST
Created attachment 332639 [details]
Patch
Comment 7 Michael Catanzaro 2018-01-30 08:18:05 PST
Comment on attachment 332639 [details]
Patch

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

OK, thanks. Let's try it.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1001
> +    ViewGestureController(WebKitWebViewBase* webViewBase)

explicit
Comment 8 Carlos Garcia Campos 2018-01-31 01:14:41 PST
Committed r227893: <https://trac.webkit.org/changeset/227893>
Comment 9 Michael Catanzaro 2018-01-31 14:24:51 PST
Regression: bug #182350