Bug 63081 – [GTK] [WK2] Fix for getting editor client commands.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Lukasz Slachciak]

In WebKit2 GTK keyboard event type was always set as KeyDown for command list. Additionally removed incorrect ASSERT causing build break on the WebKit2 Gtk debug build.
Please check attached patch.

[Lukasz Slachciak]

Created attachment 98017 [details]
Keyboard event patch 1

[Lukasz Slachciak]

Martin, I subscribed you because git says me that you are author of original code. Please take a look on this simple patch.

[Martin Robinson]

Comment on attachment 98017 [details]
Keyboard event patch 1

Just removing the ASSERT is incorrect here. What type of events are passed to this function? If it's only KeyDown and KeyUp we need a comment explaining why a native  KeyUp event is equivalent to a DOM keypress event. The ASSERT should be updated, since the following line only handles two types of events.

[Lukasz Slachciak]

As my checks show only key down are passed. Trying to find why key up are not passed too. I'll investigate it more and update ASSERT.

[Lukasz Slachciak]

After some investigation I get why GTK port tries to interpret commands only for KeyDown event.
This is because whole command interpretation is done at UIProcess site. But e.g in Qt port it is done at WebProcess site.

But let\s start from the beginning:
exemplary execution log is following:

(I'm putting "d" with keyboard in google search box)


webkitWebViewBaseKeyPressEvent
 WebPageProxy::handleKeyboardEvent: KeyDown
   WebEditorClient::handleKeyboardEvent: keydown
     WebEditorClient::getEditorCommandsForKeyEvent: keydown (message sync)
       WebPageProxy::getEditorCommandsForKeyEvent - commandsList
         PageClientImpl::getEditorCommandsForKeyEvent: KeyDown
		
  WebEditorClient::handleKeyboardEvent: keypress
    WebEditorClient::getEditorCommandsForKeyEvent: keypress (message sync)
      WebPageProxy::getEditorCommandsForKeyEvent - commandsList
	PageClientImpl::getEditorCommandsForKeyEvent: KeyDown
	  WebEditorClient::handleKeyboardEvent inserting text d

  WebPageProxy::didReceiveEvent: KeyDown

webkitWebViewBaseKeyReleaseEvent
  WebPageProxy::handleKeyboardEvent: KeyUp
  WebPageProxy::didReceiveEvent: KeyUp


As you can see for original KeyPress event, two events are generated by  WebPageProxy::handleKeyboardEvent: KeyDown
In fact it is EventHandler::keyEvent who do this. For KeyDown event it generates two events: keydown and keypress which are handled by WebEditorClient::handleKeyboardEvent.
 
When WebEditorClient sends sync message to UI process (Messages::WebPageProxy::GetEditorCommandsForKeyEvent()) it expects that he will get those commands for current event. In the first communication it is not problem. WebPageProxy takes first event from m_keyEventQueue (KeyDown) and calls PageClientImpl::getEditorCommandsForKeyEvent.
But in the second case, after artificially genereated keypress event by EventHandler::keyEvent WebPageProxy doesn't know about it. He will take first event from the queue m_keyEventQueue (KeyDown).

In short words:
1. UIProcess gets KeyDown
2. WebProces handles calling EventHandler in WebCore
3. WebCore generates two event keydown and key press
4. Both events are handled by WebProcess
5. For each event WebProcess asks to interpret commands UIProcess.
6. WebProcess for command interpretation uses current event stored in m_keyEventQueue (which is KeyDown in both cases) AND THIS IS A PROBLEM
7. UIProcess gets KeyUp
8. For KeyUp event EventHandler::keyEvent says that he will not handle it.  


As for the patch I can add ASSERT checking if events are 
KeyDown or KeyPress
but as I described logic above KeyPress will never be passed to PageClientImpl::getEditorCommandsForKeyEvent.

[Martin Robinson]

(In reply to comment #5)
> As for the patch I can add ASSERT checking if events are 
> KeyDown or KeyPress
> but as I described logic above KeyPress will never be passed to PageClientImpl::getEditorCommandsForKeyEvent.

So it looks like this code is buggy. I imagine that we need to pass the actual DOM event through to the UIProcess for it to work properly.

[Lukasz Slachciak]

I agree.
I'm thinking about adding new parameter to WebPageProxy::getEditorCommandsForKeyEvent(Vector<WTF::String>& commandsList)

Let's say:
WebPageProxy::getEditorCommandsForKeyEvent(const KeyEvent& keyEvent, Vector<WTF::String>& commandsList)

 WebEditorClient::getEditorCommandsForKeyEvent will send sync message:

Messages::WebPageProxy::GetEditorCommandsForKeyEvent(event),                                              Messages::WebPageProxy::GetEditorCommandsForKeyEvent::Reply(pendingEditorCommands),
key event will be received by                                  
WebPageProxy::getEditorCommandsForKeyEvent
and it will call
PageClientImpl::getEditorCommandsForKeyEvent

I'll prepare patch for all of these.

[Lukasz Slachciak]

I was thinking about it and tried some different implementations.

I ended up with conclusion:
Why we really need to push this KeyboardEvent from WebProcess to UIProcess (in sync, blocking way)?

What if we could interpret this event just in WebEditorClient::getEditorCommandsForKeyEvent.
As I see in code Qt port also make this interpretation at WebProcess site (see WebPage::handleEditingKeyboardEvent)

Advantages:
- correct KeyboardEvents generated at WebCore site will be processed (not KeyDown NativeWebKeyboardEvent stored in the m_keyEventQueue)
- no need to push KeyboardEvent between processes (and write KeyboardEvent coder/encoder for IPC)
- no communication overhead

[Martin Robinson]

(In reply to comment #8)
> I ended up with conclusion:
> Why we really need to push this KeyboardEvent from WebProcess to UIProcess (in sync, blocking way)?

We want to avoid instantiating a GtkWidget in the WebProcess.

[Lukasz Slachciak]

Thx Martin for comment.

I think that I have finally found optimal solution.
I'll pass from WebProcess to UIProcess eventType. In fact this is what we are missing. We still need to base on the NativeWebKeyboardEvent but for the type which is passed to KeyBindingTranslator we should use KeyboardEvent type obtained from WebProcess.

[Lukasz Slachciak]

Created attachment 99132 [details]
KeyboardEvent type patch

This patch implements passing KeyboardEvent type from WebProcess to UIProcess

[Carlos Garcia Campos]

*** Bug 62802 has been marked as a duplicate of this bug. ***

[Martin Robinson]

*** Bug 64666 has been marked as a duplicate of this bug. ***

[Martin Robinson]

Comment on attachment 99132 [details]
KeyboardEvent type patch

Looks good!

[WebKit Review Bot]

Comment on attachment 99132 [details]
KeyboardEvent type patch

Clearing flags on attachment: 99132

Committed r91169: <http://trac.webkit.org/changeset/91169>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

*** Bug 64667 has been marked as a duplicate of this bug. ***