Bug 64790 – [GTK][WK2] Handle doneWithKeyEvent in GTK port

• Component: WebKit2
• Status: RESOLVED FIXED

[Ravi Phaneendra Kasibhatla]

WebKitWebViewBase widget should forward key_press event to parent widget only if it is not handled by Web Process. Currently all key_press events are passed to parent widget.

For ex:
Press tab key in www.gmail.com username field. Instead of focusing password field, focus moves to toolbar.

[Carlos Garcia Campos]

Renaming, since didNotHandleKeyEvent was renamed to doneWithKeyEvent in r77694. It seems we added the new method but we didn't remove the old one.

[Carlos Garcia Campos]

I guess we should always return TRUE from key_press_event and key_release_event, and propagate the event again in doneWithKeyEvent() when not handled by the web process.

[Ravi Phaneendra Kasibhatla]

Created attachment 101337 [details]
Fix the tab switch issue

[Martin Robinson]

Comment on attachment 101337 [details]
Fix the tab switch issue

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

Looks good, but I have a few nits.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:216
> +                                             reinterpret_cast<const GdkEventKey*>(event.nativeEvent()));

The indentation is off here and you should just do: &event.nativeEvent()->key or similar.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:330
> +                GTK_WIDGET(webViewBase),
> +                const_cast<GdkEventKey*>(keyEvent));

No need to break this line. It isn't right to simply cast away the constness here. The NativeWebKeyboardEvent is not expecting the GdkEventKey to change. Probably it's better to make a copy of the event and send that.

[Ravi Phaneendra Kasibhatla]

Created attachment 101429 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

Addressing review comments of mrobinson & cgarcia.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #4)
> (From update of attachment 101337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101337&action=review
> 
> Looks good, but I have a few nits.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:216
> > +                                             reinterpret_cast<const GdkEventKey*>(event.nativeEvent()));
> 
> The indentation is off here and you should just do: &event.nativeEvent()->key or similar.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:330
> > +                GTK_WIDGET(webViewBase),
> > +                const_cast<GdkEventKey*>(keyEvent));
> 
> No need to break this line. It isn't right to simply cast away the constness here. The NativeWebKeyboardEvent is not expecting the GdkEventKey to change. Probably it's better to make a copy of the event and send that.
When invoking GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event, we need to do const_cast<GdkEventKey*> since the API expects a non-const GdkEventKey* and we have a const GdkEventKey*.
Addressed the other points in the comment.

[Carlos Garcia Campos]

Comment on attachment 101429 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

With the patch the focus never moves to the toolbar, I'm not sure what the right behaviour is, but with GtkLauncher the toolbar is focused at some point after pressing tab several times.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:329
> +void webkitWebViewBaseDidNotHandleKeyEvent(WebKitWebViewBase* webViewBase, const GdkEventKey* keyEvent)
> +{
> +    GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(GTK_WIDGET(webViewBase), const_cast<GdkEventKey*>(keyEvent));
> +}
> +

I'm not sure about this, maybe it would be better to use gtk_main_do_event() so that the event is properly handled again by GTK and added to the stack. Event callbacks expect functions gtk_get_current_event* to work. We would need to add a boolean flag to tell the view we are forwarding an event not handled by the web process and simply call gtk_main_do_event(). In key_press and key_release, we check that flag and if set, we simply unset it and call key_press_event on parent widget.

[Ravi Phaneendra Kasibhatla]

Created attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

Addresses the comments given by Carlos for using gtk_main_do_event()

[Ravi Phaneendra Kasibhatla]

(In reply to comment #7)
> (From update of attachment 101429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101429&action=review
> 
> With the patch the focus never moves to the toolbar, I'm not sure what the right behaviour is, but with GtkLauncher the toolbar is focused at some point after pressing tab several times.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:329
> > +void webkitWebViewBaseDidNotHandleKeyEvent(WebKitWebViewBase* webViewBase, const GdkEventKey* keyEvent)
> > +{
> > +    GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(GTK_WIDGET(webViewBase), const_cast<GdkEventKey*>(keyEvent));
> > +}
> > +
> 
> I'm not sure about this, maybe it would be better to use gtk_main_do_event() so that the event is properly handled again by GTK and added to the stack. Event callbacks expect functions gtk_get_current_event* to work. We would need to add a boolean flag to tell the view we are forwarding an event not handled by the web process and simply call gtk_main_do_event(). In key_press and key_release, we check that flag and if set, we simply unset it and call key_press_event on parent widget.
Done.

[Carlos Garcia Campos]

Comment on attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> +        webkitWebViewBaseDidNotHandleKeyEvent(webkitWebViewBase, event.nativeEvent());

We don't need this method, we can just call gtk_main_do_event here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> +    WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv;
> +    priv->shouldForwardEvent = TRUE;

This could be a single line webkitWebViewBase->priv->shouldForwardEvent = TRUE;

[Martin Robinson]

Comment on attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

I have a few concerns below. Please also take into account Carlos's suggestions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:56
> +    gboolean shouldForwardEvent;

I think that shouldForwardKeyboardEvent would be a better name here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:208
> +    // Since WebProcess key event handling is not synchronous, make it 2-pass handling.
> +    // First, always return TRUE in event handler.
> +    // Second, PageClientImpl::doneWithKeyEvent return status of event handling in WebProcess.
> +    // Third, using status, determine whether to pass the event to parent or not using gtk_main_do_event().

I think this would be easier to read as a paragraph. Something like:

// Since WebProcess key event handling is not synchronous, we must handle this event in two passes. When
// the WebProcess is done handling the event it will call PageClientImpl::doneWithKeyEvent, where we will
// be able to determine whether to pass the event to our parent or not using gtk_main_do_event().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:212
> +    if (priv->shouldForwardEvent) {
> +        priv->shouldForwardEvent = FALSE;
> +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> +    }

I'm not sure how I understand how priv->shouldForwardEvent can be true before you call PageProxy::handleKeyboardEvent below.

[Martin Robinson]

Comment on attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:345
> +    gtk_main_do_event(const_cast<GdkEvent*>(event));

I still believe that it is incorrect to simply cast away the constness of this GdkEvent. It's probably more correct to make a copy of the event and send that.

[Ravi Phaneendra Kasibhatla]

Created attachment 101854 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

[Ravi Phaneendra Kasibhatla]

(In reply to comment #10)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> > +        webkitWebViewBaseDidNotHandleKeyEvent(webkitWebViewBase, event.nativeEvent());
> 
> We don't need this method, we can just call gtk_main_do_event here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> > +    WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv;
> > +    priv->shouldForwardEvent = TRUE;
> 
> This could be a single line webkitWebViewBase->priv->shouldForwardEvent = TRUE;
Done.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #11)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> I have a few concerns below. Please also take into account Carlos's suggestions.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:56
> > +    gboolean shouldForwardEvent;
> 
> I think that shouldForwardKeyboardEvent would be a better name here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:208
> > +    // Since WebProcess key event handling is not synchronous, make it 2-pass handling.
> > +    // First, always return TRUE in event handler.
> > +    // Second, PageClientImpl::doneWithKeyEvent return status of event handling in WebProcess.
> > +    // Third, using status, determine whether to pass the event to parent or not using gtk_main_do_event().
> 
> I think this would be easier to read as a paragraph. Something like:
> 
> // Since WebProcess key event handling is not synchronous, we must handle this event in two passes. When
> // the WebProcess is done handling the event it will call PageClientImpl::doneWithKeyEvent, where we will
> // be able to determine whether to pass the event to our parent or not using gtk_main_do_event().
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:212
> > +    if (priv->shouldForwardEvent) {
> > +        priv->shouldForwardEvent = FALSE;
> > +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    }
> 
> I'm not sure how I understand how priv->shouldForwardEvent can be true before you call PageProxy::handleKeyboardEvent below.
shouldForwardKeyboardEvent will not be true when we are calling PageProxy::handleKeyboardEvent. It would be FALSE and hence event would be sent to PageProxy for handling. On handling the input event, when we get the call back to PageClientImpl::doneWithKeyEvent, we check for handled status. If it is false, then we call webkitWebViewBaseSetForwardEvent which set shouldForwardKeyboardEvent to TRUE. Then we call gtk_main_do_event which triggers key_press_event where we pass the event to parent widget.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #12)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:345
> > +    gtk_main_do_event(const_cast<GdkEvent*>(event));
> 
> I still believe that it is incorrect to simply cast away the constness of this GdkEvent. It's probably more correct to make a copy of the event and send that.
Done.

[Martin Robinson]

Comment on attachment 101854 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

I'm a bit concerned about the interaction of key release and following key press events.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:210
> +    if (!wasEventHandled) {

Please just use an early return here.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:212
> +        webkitWebViewBaseSetForwardEvent(webkitWebViewBase);

With the way the code is written now, what happens when this is called on a key release event? Won't this setting then effect the behavior of the next key press?

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> +        GdkEvent inputEvent = *event.nativeEvent();

It's probably better to use: GOwnPtr<GdkEvent> inputEvent(gdk_event_copy(event.nativeEvent())); here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:214
> +    // Since WebProcess key event handling is not synchronous, handle the event in two passes.
> +    // When WebProcess processes the input event, it will call PageClientImpl::doneWithKeyEvent 
> +    // with event handled status which determines whether to pass the input event to parent or not 
> +    // using gtk_main_do_event().
> +    if (priv->shouldForwardKeyboardEvent) {
> +        priv->shouldForwardKeyboardEvent = FALSE;
> +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> +    }
>      priv->pageProxy->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event)));
> -
> -    return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> +    return TRUE;

I should clarify my confusion here. I do not understand how priv->shouldForwardKeyboardEvent can be true when webkitWebViewBaseKeyPressEvent is called. The only way I can see this happening is if during a previous key release event, priv->shouldForwardKeyboardEvent was set to true. I do not think previous key release events should affect future key press events (see my comment above).

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> +void webkitWebViewBaseSetForwardEvent(WebKitWebViewBase* webkitWebViewBase)
> +{
> +    webkitWebViewBase->priv->shouldForwardKeyboardEvent = TRUE;
> +}

I think you can just eliminate this method entirely and do this directly in PageClientImpl::doneWithKeyEvent.

[Carlos Garcia Campos]

(In reply to comment #12)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:345
> > +    gtk_main_do_event(const_cast<GdkEvent*>(event));
> 
> I still believe that it is incorrect to simply cast away the constness of this GdkEvent. It's probably more correct to make a copy of the event and send that.

We don't need to copy the event, gtk_main_do_event() doesn't modify the event at all.

[Carlos Garcia Campos]

(In reply to comment #17)
> (From update of attachment 101854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101854&action=review
> 
> I'm a bit concerned about the interaction of key release and following key press events.
> 
> 
> With the way the code is written now, what happens when this is called on a key release event? Won't this setting then effect the behavior of the next key press?

shouldForwardKeyboardEvent is set to false in both key press and release event handlers, so it won't affect any other events.

> > -
> > -    return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    return TRUE;
> 
> I should clarify my confusion here. I do not understand how priv->shouldForwardKeyboardEvent can be true when webkitWebViewBaseKeyPressEvent is called. The only way I can see this happening is if during a previous key release event, priv->shouldForwardKeyboardEvent was set to true. I do not think previous key release events should affect future key press events (see my comment above).

shouldForwardKeyboardEvent is only true when a key press/release event is not handled by the web process, in this case we simply take the same event and send it to gtk to be processed again. Then key_press or key_release will be called, and shouldForwardKeyboardEvent will be true, so the handler will send the event to the parent widget and will set shouldForwardKeyboardEvent to false again so it won't affect future events. It's not a previous event, we are processing the same event again, so other key events are not affected.

[Ravi Phaneendra Kasibhatla]

Created attachment 101997 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 4

Revision 4 - based on latest comments from Martin.

[Ravi Phaneendra Kasibhatla]

Comment on attachment 101854 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

Updated with revision 4 patch.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #17)
> (From update of attachment 101854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101854&action=review
> 
> I'm a bit concerned about the interaction of key release and following key press events.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:210
> > +    if (!wasEventHandled) {
> 
> Please just use an early return here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:212
> > +        webkitWebViewBaseSetForwardEvent(webkitWebViewBase);
> 
> With the way the code is written now, what happens when this is called on a key release event? Won't this setting then effect the behavior of the next key press?
Will be addressed in new enum type implementation.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> > +        GdkEvent inputEvent = *event.nativeEvent();
> 
> It's probably better to use: GOwnPtr<GdkEvent> inputEvent(gdk_event_copy(event.nativeEvent())); here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:214
> > +    // Since WebProcess key event handling is not synchronous, handle the event in two passes.
> > +    // When WebProcess processes the input event, it will call PageClientImpl::doneWithKeyEvent 
> > +    // with event handled status which determines whether to pass the input event to parent or not 
> > +    // using gtk_main_do_event().
> > +    if (priv->shouldForwardKeyboardEvent) {
> > +        priv->shouldForwardKeyboardEvent = FALSE;
> > +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    }
> >      priv->pageProxy->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event)));
> > -
> > -    return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    return TRUE;
> 
> I should clarify my confusion here. I do not understand how priv->shouldForwardKeyboardEvent can be true when webkitWebViewBaseKeyPressEvent is called. The only way I can see this happening is if during a previous key release event, priv->shouldForwardKeyboardEvent was set to true. I do not think previous key release events should affect future key press events (see my comment above).
Carlos has explained in the below comment about the this design. We reset the flag in each key_press_event or key_release_event handler to prevent any effect on future events. Like we discussed, I have made it an enum rather than bool, to pass on specifically the event not handled to parent.

I still though have one concern in calling gtk_main_do_event(). Consider a usecase, where key_press gets handled by web process but not key_release. Since we are calling gtk_main_do_event(), we again get key_press & key_release callbacks to WebKitWebViewBase widget. With our flag for handled status, we will pass key_release to parent widget, but key_press gets propagated again to web process. Wouldn't that be an issue?
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> > +void webkitWebViewBaseSetForwardEvent(WebKitWebViewBase* webkitWebViewBase)
> > +{
> > +    webkitWebViewBase->priv->shouldForwardKeyboardEvent = TRUE;
> > +}
> 
> I think you can just eliminate this method entirely and do this directly in PageClientImpl::doneWithKeyEvent.
Done.

[Martin Robinson]

(In reply to comment #22)

> I still though have one concern in calling gtk_main_do_event(). Consider a usecase, where key_press gets handled by web process but not key_release. Since we are calling gtk_main_do_event(), we again get key_press & key_release callbacks to WebKitWebViewBase widget. With our flag for handled status, we will pass key_release to parent widget, but key_press gets propagated again to web process. Wouldn't that be an issue?

Yes, that's an issue. We need to throw the event away in that case.

[Carlos Garcia Campos]

Comment on attachment 101997 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 4

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:220
> +    GOwnPtr<GdkEvent> inputEvent(gdk_event_copy(event.nativeEvent()));
> +    gtk_main_do_event(inputEvent.get());

I think we shouldn't copy the event, gtk_main_do_event() doesn't modify the event.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-60
> -struct _WebKitWebViewBasePrivate {
> -    OwnPtr<PageClientImpl> pageClient;
> -    RefPtr<WebPageProxy> pageProxy;
> -    gboolean isPageActive;
> -    GtkIMContext* imContext;
> -    GtkClickCounter clickCounter;
> -    CString tooltipText;
> -};
> -

I prefer to keep private structures as private and add specific private API when needed, it makes easier to know who and why is using the private members outside the object.

[Martin Robinson]

(In reply to comment #24)

> I prefer to keep private structures as private and add specific private API when needed, it makes easier to know who and why is using the private members outside the object.

I suggested this because it's a pattern we commonly use in WebKit1.

[Carlos Garcia Campos]

(In reply to comment #22)
> I still though have one concern in calling gtk_main_do_event(). Consider a usecase, where key_press gets handled by web process but not key_release. Since we are calling gtk_main_do_event(), we again get key_press & key_release callbacks to WebKitWebViewBase widget. With our flag for handled status, we will pass key_release to parent widget, but key_press gets propagated again to web process. Wouldn't that be an issue?

No, with gtk_main_do_event() we only process the event that was not handled by the we process, if key press is handled by not key release, doneWithKeyEvent will be called with handled=true only for the key press event, so we will call gtk_main_do_event() only for the key release. I don't think we need the enum, since doneWithKeyEvent will be called for every key press/release event, the boolean is enough, events are handled synchronously by gtk+.

[Ravi Phaneendra Kasibhatla]

Created attachment 102258 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 5

Addressed the concerns & comments of carlos & martin.
As per explanation given by Carlos, since gtk_main_do_event is sync & gets invoked only for event which is being passed gtk_main_do_event, reverting to boolean flag. 
As per understanding with Martin & Carlos now keeping the WebKitWebViewPrivate in .cpp only and exposing a new API to set the flag from PageClientImpl.
As per discussion with Carlos & Martin, NativeWebKeyboardEvent::nativeEvent now returns a non-const GdkEvent. Removing the casting & event copy from PageClientImpl.

[WebKit Review Bot]

Comment on attachment 102258 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 5

Clearing flags on attachment: 102258

Committed r91937: <http://trac.webkit.org/changeset/91937>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.