Bug 182224

Summary: REGRESSION(r227544): [GTK] contextMenuEvent is NULL on CONTEXT_MENU call
Product: WebKit Reporter: Jan-Michael Brummer <jan.brummer>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, mcatanzaro
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Linux   
See Also: http://https//bugzilla.gnome.org/show_bug.cgi?id=792976
https://bugs.webkit.org/show_bug.cgi?id=181996
https://bugs.webkit.org/show_bug.cgi?id=182350
Bug Depends on: 182381    
Bug Blocks:    
Attachments:
Description Flags
Patch fixing contextMenuEvent
mcatanzaro: review-, mcatanzaro: commit-queue-
Patch mcatanzaro: review+

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 EWS Watchlist 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