Bug 28733 – [GTK] GtkIMContext filtering interferes with DOM key events

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The way that key presses are filtered by gtk_im_context_filter_keypress is blocking some DOM key events. Furthermore, since keystrokes are filtered before a key is released, the contents of the editor field are modified at the wrong stage (i.e. before the keydown event instead of right before the keyup event). This behavior can be observed at http://jparent.googlepages.com/IME.html .

Expected results (for entering 'abc' in either field):
down contents are: 
press contents are: 
up contents are: a
down contents are: a
press contents are: a
up contents are: ab
down contents are: ab
press contents are: ab
up contents are: abc

Actual results:
down contents are: a
up contents are: a
down contents are: ab
up contents are: ab
down contents are: abc
up contents are: abc

This issue is also causing problems with the Web Inspector. When the Inspector console finds a completion, the next keystroke will often be lost.

[Martin Robinson]

Created attachment 38597 [details]
Patch for this issue

I've added a patch which reorganizes EditorClientGtk to manage IME commits and preedits in two stages.
1. When a commit or preedit is detected it will be recorded -- they always happen during keydown.
2. When the next applicable keyup event happens these IME events will be recorded in the field.

[Xan Lopez]

Comment on attachment 38597 [details]
Patch for this issue

So, this looks fine in general to me, but I have some issues:

- How do you know the behavior the IM code should have? By inspection of other ports? Some spec? Just the webpage you have linked to? (Not that I don't believe we are broken, especially since the inspector is also failing; just wondering :))
- I'm kind of weary to touch this stuff a lot without having tests for it, it would be great to finish eventSender to enable them or add new ones.
- Your patch will break my stuff in https://bugs.webkit.org/show_bug.cgi?id=25526, which would be also great to finish :). It's also blocked on doing tests for it though.

A few small details:

> diff --git a/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp b/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
>  bool EditorClient::shouldBeginEditing(WebCore::Range*)
>  {
> +    g_free(pendingComposition);
> +    pendingComposition = 0;
> +    g_free(pendingPreedit);
> +    pendingPreedit = 0;
> +

This chunk is repeated three times, probably worth to do a small function for it.

>      notImplemented();
>      return true;
>  }
>  
>  bool EditorClient::shouldEndEditing(WebCore::Range*)
>  {
> +    g_free(pendingComposition);
> +    pendingComposition = 0;
> +    g_free(pendingPreedit);
> +    pendingPreedit = 0;
> +
>      notImplemented();

Wonder at what point it's silly to keep the notImplemented() in these functions; what should happen here?

> -
> -    if (keyEvent->type() == PlatformKeyboardEvent::RawKeyDown) {
> -        // WebKit doesn't have enough information about mode to decide how commands that just insert text if executed via Editor should be treated,
> -        // so we leave it upon WebCore to either handle them immediately (e.g. Tab that changes focus) or let a keypress event be generated
> -        // (e.g. Tab that inserts a Tab character, or Enter).
> -        return !command.isTextInsertion() && command.execute(evt);
> +    const gchar* editorCommandString = interpretEditorCommandKeyEvent(event);
> +    if (editorCommandString)
> +    {

Brace in wrong line.

All that being said, I think I would like someone else to look at this before r+ the patch, I'm not so familiar with all the details in this.

[Martin Robinson]

I agree that tests should come along with this patch.

The source of problems in GTK+ with IME events is that the "System (Simple)" input method (and perhaps others) treats non-IME input like IME input. That means that when you press 'a' that keypress is filtered by the IMContext and an IME committed event happens soon after.

As far as the behavior of normal key events, the expected behavior comes from both FireFox (Linux) and WebKit (Mac). Here are a couple pertinent links on the topic as well:

- https://bugs.webkit.org/show_bug.cgi?id=25119
- https://lists.webkit.org/pipermail/webkit-dev/2007-December/002992.html

[Xan Lopez]

Comment on attachment 38597 [details]
Patch for this issue

Per conversations on IRC this patch needs to be updated, removing it from the queue.

[Martin Robinson]

Created attachment 39634 [details]
Updated patch

I've attached an updated patch which builds upon the recent EventSender implementation to enable a bunch of tests. One failing test (fast/events/js-keyboard-event-creation.html) would pass if it were not for the same bug which is causing fast/html/text-field-input-types.html to fail.

[Xan Lopez]

Comment on attachment 39634 [details]
Updated patch

> From 577ffcebd4918a23912543b9b401e4beabb90e72 Mon Sep 17 00:00:00 2001
> From: Martin Robinson <[e-mail]>
> Date: Tue, 15 Sep 2009 21:38:25 -0700
> Subject: [PATCH] Ensure that even when GTKSimpleIMContext filters non-IME keystrokes, keyboard events are fired properly.
> 
> ---
>  LayoutTests/ChangeLog                         |   11 ++
>  LayoutTests/platform/gtk/Skipped              |    8 -
>  WebCore/ChangeLog                             |   13 ++
>  WebCore/platform/gtk/KeyEventGtk.cpp          |    4 +
>  WebKit/gtk/ChangeLog                          |   20 +++
>  WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp |  209 +++++++++++++++----------
>  6 files changed, 171 insertions(+), 94 deletions(-)
> 
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index b57082c..f314fe6 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,14 @@
> [phone]  Martin Robinson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK] GtkIMContext filtering interferes with DOM key events
> +        https://bugs.webkit.org/show_bug.cgi?id=28733
> +
> +        Enable tests which are now passing because of key event fixes.
> +
> +        * platform/gtk/Skipped:
> +
>  2009-09-15  Chris Fleizach  <[e-mail]>
>  
>          Layout test fix. 
> diff --git a/LayoutTests/platform/gtk/Skipped b/LayoutTests/platform/gtk/Skipped
> index 8db2ec5..e8a796c 100644
> --- a/LayoutTests/platform/gtk/Skipped
> +++ b/LayoutTests/platform/gtk/Skipped
> @@ -1605,12 +1605,6 @@ fast/events/dblclick-addEventListener.html
>  fast/events/drag-in-frames.html
>  fast/events/frame-tab-focus.html
>  fast/events/js-keyboard-event-creation.html
> -fast/events/key-events-in-input-button.html
> -fast/events/key-events-in-input-text.html
> -fast/events/keydown-keypress-focus-change.html
> -fast/events/keydown-keypress-preventDefault.html
> -fast/events/keypress-focus-change.html
> -fast/events/keypress-insert-tab.html
>  fast/events/mouse-click-events.html
>  fast/events/mouseclick-target-and-positioning.html
>  fast/events/mouseup-from-button2.html
> @@ -1629,7 +1623,6 @@ fast/events/pointer-events-2.html
>  fast/events/popup-blocking-click-in-iframe.html
>  fast/events/right-click-focus.html
>  fast/events/scrollbar-double-click.html
> -fast/events/special-key-events-in-input-text.html
>  fast/events/stop-load-in-unload-handler-using-document-write.html
>  fast/events/stop-load-in-unload-handler-using-window-stop.html
>  fast/events/tabindex-focus-blur-all.html
> @@ -5582,7 +5575,6 @@ editing/pasteboard/files-during-page-drags.html
>  editing/selection/extend-selection-after-double-click.html
>  fast/events/drag-to-navigate.html
>  fast/events/prevent-drag-to-navigate.html
> -fast/events/keydown-function-keys.html
>  fast/forms/slider-delete-while-dragging-thumb.html
>  fast/events/tab-focus-anchor.html
>  http/tests/local/drag-over-remote-content.html
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 4b19fb1..dfca55a 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,16 @@
> [phone]  Martin Robinson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK] GtkIMContext filtering interferes with DOM key events
> +        https://bugs.webkit.org/show_bug.cgi?id=28733
> +
> +        Give GDK_Backspace key events the proper text properties.
> +
> +        * platform/gtk/KeyEventGtk.cpp:
> +        (WebCore::keyIdentifierForGdkKeyCode):
> +        (WebCore::singleCharacterString):
> +
>  2009-09-15  Geoffrey Garen  <[e-mail]>
>  
>          Reviewed by Sam Weinig.
> diff --git a/WebCore/platform/gtk/KeyEventGtk.cpp b/WebCore/platform/gtk/KeyEventGtk.cpp
> index 5875547..4186c2f 100644
> --- a/WebCore/platform/gtk/KeyEventGtk.cpp
> +++ b/WebCore/platform/gtk/KeyEventGtk.cpp
> @@ -136,6 +136,8 @@ static String keyIdentifierForGdkKeyCode(guint keyCode)
>              // Standard says that DEL becomes U+007F.
>          case GDK_Delete:
>              return "U+007F";
> +        case GDK_BackSpace:
> +            return "U+0008";
>          case GDK_ISO_Left_Tab:
>          case GDK_3270_BackTab:
>          case GDK_Tab:
> @@ -503,6 +505,8 @@ static String singleCharacterString(guint val)
>          case GDK_KP_Enter:
>          case GDK_Return:
>              return String("\r");
> +        case GDK_BackSpace:
> +            return String("\x8");
>          default:
>              gunichar c = gdk_keyval_to_unicode(val);
>              glong nwc;
> diff --git a/WebKit/gtk/ChangeLog b/WebKit/gtk/ChangeLog
> index ea3c0c2..863bc45 100644
> --- a/WebKit/gtk/ChangeLog
> +++ b/WebKit/gtk/ChangeLog
> @@ -1,3 +1,23 @@
> [phone]  Martin Robinson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK] GtkIMContext filtering interferes with DOM key events
> +        https://bugs.webkit.org/show_bug.cgi?id=28733
> +
> +        Ensure that even when GTKSimpleIMContext filters non-IME keystrokes,
> +        keyboard events are fired properly.
> +
> +        * WebCoreSupport/EditorClientGtk.cpp:
> +        (WebKit::imContextCommitted):
> +        (WebKit::imContextPreeditChanged):
> +        (WebKit::EditorClient::shouldBeginEditing):
> +        (WebKit::EditorClient::shouldEndEditing):
> +        (WebKit::interpretEditorCommandKeyEvent):
> +        (WebKit::handleCaretBrowsingKeyboardEvent):
> +        (WebKit::EditorClient::handleKeyboardEvent):
> +        (WebKit::EditorClient::handleInputMethodKeydown):
> +
>  2009-09-14  Gustavo Noronha Silva  <[e-mail]>
>  
>          Reviewed by Xan Lopez and Jan Alonzo.
> diff --git a/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp b/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
> index 71a7c1a..04f6d4d 100644
> --- a/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
> +++ b/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
> @@ -49,39 +49,22 @@ using namespace WebCore;
>  
>  namespace WebKit {
>  
> +static gchar* pendingComposition = 0;
> +static gchar* pendingPreedit = 0;
> +
>  static void imContextCommitted(GtkIMContext* context, const gchar* str, EditorClient* client)
>  {
> -    Frame* targetFrame = core(client->m_webView)->focusController()->focusedOrMainFrame();
> -
> -    if (!targetFrame || !targetFrame->editor()->canEdit())
> -        return;
> -
> -    Editor* editor = targetFrame->editor();
> -
> -    String commitString = String::fromUTF8(str);
> -    editor->confirmComposition(commitString);
> +    // This signal will fire during a keydown event. We want to the contents of the

s/We want to the/We want the/ ?

> +    // field to change right before the keyup event, so we wait until then to actually
> +    // commit this composition. 
> +    pendingComposition = g_strdup(str);

Since you seem assume pendingComposition must be always clear at this point I guess it would be good to add an ASSERT(!pendingComposition).

>  }
>  
>  static void imContextPreeditChanged(GtkIMContext* context, EditorClient* client)
>  {
> -    Frame* frame = core(client->m_webView)->focusController()->focusedOrMainFrame();
> -    Editor* editor = frame->editor();
> -
> -    gchar* preedit = NULL;
> -    gint cursorPos = 0;
>      // We ignore the provided PangoAttrList for now.
> -    gtk_im_context_get_preedit_string(context, &preedit, NULL, &cursorPos);
> -    String preeditString = String::fromUTF8(preedit);
> -    g_free(preedit);
> -
> -    // setComposition() will replace the user selection if passed an empty
> -    // preedit. We don't want this to happen.
> -    if (preeditString.isEmpty() && !editor->hasComposition())
> -        return;
> -
> -    Vector<CompositionUnderline> underlines;
> -    underlines.append(CompositionUnderline(0, preeditString.length(), Color(0, 0, 0), false));
> -    editor->setComposition(preeditString, underlines, cursorPos, 0);
> +    gint cursorPos = 0;
> +    gtk_im_context_get_preedit_string(context, &pendingPreedit, NULL, &cursorPos);

cursorPos can be just NULL here, the functions seem to check the value before assigning.

And another ASSERT for pendingPreedit? :)

>  }
>  
>  void EditorClient::setInputMethodState(bool active)
> @@ -136,12 +119,22 @@ int EditorClient::spellCheckerDocumentTag()
>  
>  bool EditorClient::shouldBeginEditing(WebCore::Range*)
>  {
> +    g_free(pendingComposition);
> +    pendingComposition = 0;
> +    g_free(pendingPreedit);
> +    pendingPreedit = 0;
> +

As I said in a previous comment, I think it's worth to put this in a 'clearPendingVariables()' function or something, since it's repeated three/four times.

>      notImplemented();
>      return true;
>  }
>  
>  bool EditorClient::shouldEndEditing(WebCore::Range*)
>  {
> +    g_free(pendingComposition);
> +    pendingComposition = 0;
> +    g_free(pendingPreedit);
> +    pendingPreedit = 0;
> +
>      notImplemented();
>      return true;
>  }
> @@ -421,7 +414,7 @@ static const KeyPressEntry keyPressEntries[] = {
>      { '\r',   AltKey | ShiftKey,  "InsertNewline"                               },
>  };
>  
> -static const char* interpretKeyEvent(const KeyboardEvent* evt)
> +static const char* interpretEditorCommandKeyEvent(const KeyboardEvent* evt)
>  {
>      ASSERT(evt->type() == eventNames().keydownEvent || evt->type() == eventNames().keypressEvent);
>  
> @@ -456,74 +449,120 @@ static const char* interpretKeyEvent(const KeyboardEvent* evt)
>      return mapKey ? keyPressCommandsMap->get(mapKey) : 0;
>  }
>  
> -static bool handleEditingKeyboardEvent(KeyboardEvent* evt)
> +static bool handleCaretBrowsingKeyboardEvent(Frame* frame, const PlatformKeyboardEvent* keyEvent)
> +{
> +    switch (keyEvent->windowsVirtualKeyCode()) {
> +        case VK_LEFT:
> +            frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> +                    SelectionController::LEFT,
> +                    keyEvent->ctrlKey() ? WordGranularity : CharacterGranularity,
> +                    true);
> +            return true;
> +        case VK_RIGHT:
> +            frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> +                    SelectionController::RIGHT,
> +                    keyEvent->ctrlKey() ? WordGranularity : CharacterGranularity,
> +                    true);
> +            return true;
> +        case VK_UP:
> +            frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> +                    SelectionController::BACKWARD,
> +                    keyEvent->ctrlKey() ? ParagraphGranularity : LineGranularity,
> +                    true);
> +            return true;
> +        case VK_DOWN:
> +            frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> +                    SelectionController::FORWARD,
> +                    keyEvent->ctrlKey() ? ParagraphGranularity : LineGranularity,
> +                    true);
> +            return true;
> +        default:
> +            return false; // Not a caret browswing keystroke, so continue processing.
> +    }
> +}
> +
> +void EditorClient::handleKeyboardEvent(KeyboardEvent* event)
>  {
> -    Node* node = evt->target()->toNode();
> +    Node* node = event->target()->toNode();
>      ASSERT(node);
>      Frame* frame = node->document()->frame();
>      ASSERT(frame);
>  
> -    const PlatformKeyboardEvent* keyEvent = evt->keyEvent();
> -    if (!keyEvent)
> -        return false;
> +    const PlatformKeyboardEvent* platformEvent = event->keyEvent();
> +    if (!platformEvent)
> +        return;
>  
>      bool caretBrowsing = frame->settings()->caretBrowsingEnabled();
> -    if (caretBrowsing) {
> -        switch (keyEvent->windowsVirtualKeyCode()) {
> -            case VK_LEFT:
> -                frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> -                        SelectionController::LEFT,
> -                        keyEvent->ctrlKey() ? WordGranularity : CharacterGranularity,
> -                        true);
> -                return true;
> -            case VK_RIGHT:
> -                frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> -                        SelectionController::RIGHT,
> -                        keyEvent->ctrlKey() ? WordGranularity : CharacterGranularity,
> -                        true);
> -                return true;
> -            case VK_UP:
> -                frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> -                        SelectionController::BACKWARD,
> -                        keyEvent->ctrlKey() ? ParagraphGranularity : LineGranularity,
> -                        true);
> -                return true;
> -            case VK_DOWN:
> -                frame->selection()->modify(keyEvent->shiftKey() ? SelectionController::EXTEND : SelectionController::MOVE,
> -                        SelectionController::FORWARD,
> -                        keyEvent->ctrlKey() ? ParagraphGranularity : LineGranularity,
> -                        true);
> -                return true;
> -        }
> +    if (caretBrowsing && handleCaretBrowsingKeyboardEvent(frame, platformEvent)) {
> +        // This was a caret browsing key event, so prevent it from bubbling up to the DOM.
> +        event->setDefaultHandled();
> +        return;
>      }
>  
> -    Editor::Command command = frame->editor()->command(interpretKeyEvent(evt));
> +    // Don't allow editor commands or text insertion for nodes that cannot edit.
> +    if (!frame->editor()->canEdit())
> +        return;
>  
> -    if (keyEvent->type() == PlatformKeyboardEvent::RawKeyDown) {
> -        // WebKit doesn't have enough information about mode to decide how commands that just insert text if executed via Editor should be treated,
> -        // so we leave it upon WebCore to either handle them immediately (e.g. Tab that changes focus) or let a keypress event be generated
> -        // (e.g. Tab that inserts a Tab character, or Enter).
> -        return !command.isTextInsertion() && command.execute(evt);
> +    const gchar* editorCommandString = interpretEditorCommandKeyEvent(event);
> +    if (editorCommandString)
> +    {

The brace is wrong.

> +        Editor::Command command = frame->editor()->command(editorCommandString);
> +
> +        // On editor commands from key down events, we only want to let the event bubble up to
> +        // the DOM if it inserts text. If it doesn't insert text (e.g. Tab that changes focus)
> +        // we just want WebKit to handle it immediately without a DOM event.
> +        if (platformEvent->type() == PlatformKeyboardEvent::RawKeyDown) {
> +            if (!command.isTextInsertion() && command.execute(event))
> +                event->setDefaultHandled();
> +
> +            return;
> +        } else if (command.execute(event)) {
> +            event->setDefaultHandled();
> +            return;
> +        }
>      }
>  
> -    if (command.execute(evt))
> -        return true;
> +    // This is just a normal text insertion, so wait to execute the insertion
> +    // until a keyup event happens. This will ensure that the insertion will not
> +    // be reflected in the contents of the field until the keyup DOM event.
> +    if (event->type() != eventNames().keydownEvent) {
> +
> +        if (pendingComposition) {
> +            String compositionString = String::fromUTF8(pendingComposition);
> +            frame->editor()->confirmComposition(compositionString);
> +
> +            g_free(pendingComposition);
> +            pendingComposition = 0;
> +            g_free(pendingPreedit);
> +            pendingPreedit = 0;
> +
> +        } else if (pendingPreedit) {
> +            String preeditString = String::fromUTF8(pendingPreedit);
> +
> +            // Don't use an empty preedit as it will destroy the current
> +            // selection, even if the composition is cancelled or fails later on.
> +            if (!preeditString.isEmpty()) {
> +                Vector<CompositionUnderline> underlines;
> +                underlines.append(CompositionUnderline(0, preeditString.length(), Color(0, 0, 0), false));
> +                frame->editor()->setComposition(preeditString, underlines, 0, 0);
> +            }
>  
> -    // Don't insert null or control characters as they can result in unexpected behaviour
> -    if (evt->charCode() < ' ')
> -        return false;
> +            g_free(pendingPreedit);
> +            pendingPreedit = 0;
>  
> -    // Don't insert anything if a modifier is pressed
> -    if (keyEvent->ctrlKey() || keyEvent->altKey())
> -        return false;
> +        } else {
> +            // Don't insert null or control characters as they can result in unexpected behaviour
> +            if (event->charCode() < ' ')
> +                return;
>  
> -    return frame->editor()->insertText(evt->keyEvent()->text(), evt);
> -}
> +            // Don't insert anything if a modifier is pressed
> +            if (platformEvent->ctrlKey() || platformEvent->altKey())
> +                return;
>  
> -void EditorClient::handleKeyboardEvent(KeyboardEvent* event)
> -{
> -    if (handleEditingKeyboardEvent(event))
> -        event->setDefaultHandled();
> +            if (frame->editor()->insertText(platformEvent->text(), event))
> +                event->setDefaultHandled();
> +        }
> +    }
>  }
>  
>  void EditorClient::handleInputMethodKeydown(KeyboardEvent* event)
> @@ -533,9 +572,7 @@ void EditorClient::handleInputMethodKeydown(KeyboardEvent* event)
>          return;
>  
>      WebKitWebViewPrivate* priv = m_webView->priv;
> -    // TODO: Dispatch IE-compatible text input events for IM events.
> -    if (gtk_im_context_filter_keypress(priv->imContext, event->keyEvent()->gdkEventKey()))
> -        event->setDefaultHandled();
> +    gtk_im_context_filter_keypress(priv->imContext, event->keyEvent()->gdkEventKey());
>  }
>  
>  EditorClient::EditorClient(WebKitWebView* webView)
> -- 
> 1.6.0.4
> 

Looks good to me, but marking r- to fix those small details.

[Martin Robinson]

Created attachment 39977 [details]
Updated patch (rev2)

Thank you for your comments. I have updated the patch with the changes you suggested.

[Martin Robinson]

Created attachment 40008 [details]
Updated patch (rev3)

Updated to use ASSERT a little more sanely.

[Xan Lopez]

Comment on attachment 40008 [details]
Updated patch (rev3)

r=me!

[WebKit Commit Bot]

Comment on attachment 40008 [details]
Updated patch (rev3)

Rejecting patch 40008 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/gtk/Skipped
Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469

[Eric Seidel (no email)]

Comment on attachment 40008 [details]
Updated patch (rev3)

Sorry, commit-queue bug, not your fault! :(  bug 28316.

[WebKit Commit Bot]

Comment on attachment 40008 [details]
Updated patch (rev3)

Clearing flags on attachment: 40008

Committed r48697: <http://trac.webkit.org/changeset/48697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

I reverted this patch and I'm reopening the bug, since it broke keyboard event handling notification to GTK+ pretty massively :/

The thing is, the changes in EditorClientGtk.cpp to not do anything on KeyDown make us report back to GTK+ that key_press was unhandled, *and*, EditorClient is never called by WebCore for KeyUp events, so that code is effectively dead and doing nothing, which is obviously broken.

We need to figure out this more carefully.

[Xan Lopez]

(In reply to comment #14)
> I reverted this patch and I'm reopening the bug, since it broke keyboard event
> handling notification to GTK+ pretty massively :/
> 
> The thing is, the changes in EditorClientGtk.cpp to not do anything on KeyDown
> make us report back to GTK+ that key_press was unhandled, *and*, EditorClient
> is never called by WebCore for KeyUp events, so that code is effectively dead
> and doing nothing, which is obviously broken.
> 
> We need to figure out this more carefully.

So, disregard the last part, EditorClient is called for KeyUp events (fuckup with gdb :)), but the problem is that we never mark the event as handled when finalizing a pending composition coming from keyDown. So if we can fix this I suppose the patch would be OK, since reporting event not handled for press is OK since we actually only insert during KeyUp.

[Martin Robinson]

Created attachment 40392 [details]
Updated patch (rev4)

The latest patch fixes an issue where keystrokes from compositions did not mark the event as handled by the default event handler. I've also added a test to verify this fix.

[Xan Lopez]

Comment on attachment 40392 [details]
Updated patch (rev4)

Looks great to me, let's try this again! r=me

[WebKit Commit Bot]

Comment on attachment 40392 [details]
Updated patch (rev4)

Clearing flags on attachment: 40392

Committed r48964: <http://trac.webkit.org/changeset/48964>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.