The way IME handling works now it's quite complicated to deal with misbehaving input methods. It's also tightly integrated into the WebKit1 WebCoreSupport infrastructure. This bug tracks decoupling the logic from WebKit1 and fixing a few remaining bugs.
Created attachment 138547 [details] Patch
Created attachment 139043 [details] Patch
Carlos pointed out a memory leak and some small style mistakes. I've fixed those and updated the patch.
Comment on attachment 139043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139043&action=review I'm going to dream of compositions and preedits, oh yeah. I only have a couple concerns over code flow I'd like you to double check for sanity. I love how it's taken lots of complexity into a couple objects, though, I can see how this will help WebKit2 big time. > Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:66 > + ASSERT(!m_widget); Should we also ASSERT that the widget is not realized? As with this other ASSERT it seems pretty much impossible given we are only doing this on the instance init, but I guess that'd make the sanity checking complete. > Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:135 > + if (!m_composingTextCurrently && !m_preeditChanged && m_confirmedComposition.length() == 1) { > + bool result = sendSimpleKeyEvent(event, m_confirmedComposition); > + if (!m_confirmedComposition.isEmpty()) { > + m_composingTextCurrently = false; > + m_confirmedComposition = String(); > + } I could not figure out in which case this condition will be false, is it a side effect? > Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:162 > + // If this keystroke was not filtered, but resulted in an updated composition > + // or preedit, we send those before the key event. The following event sequence > + // copied from the Chromium source code illustrates the reasoning: This all sounds logical to me, but the way the code is structured it looks like we can fall in here even if filtered is true (since inside that if there are no unconditional returns). Does this still hold in that case or did I miss something?
Comment on attachment 139043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139043&action=review >> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:66 >> + ASSERT(!m_widget); > > Should we also ASSERT that the widget is not realized? As with this other ASSERT it seems pretty much impossible given we are only doing this on the instance init, but I guess that'd make the sanity checking complete. This is a good idea. I'll add the ASSERT. >> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:135 >> + } > > I could not figure out in which case this condition will be false, is it a side effect? I think you're right, so I'll remove this check. >> Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp:162 >> + // copied from the Chromium source code illustrates the reasoning: > > This all sounds logical to me, but the way the code is structured it looks like we can fall in here even if filtered is true (since inside that if there are no unconditional returns). Does this still hold in that case or did I miss something? It's a bit unclear, but we want keyup events that were filtered to be processed here as well. I can try to make this clearer by changing the comment below from: // We also treat key release events the same way, as the IME code in // EditorClient doesn't get to look at them, as for key press events. to // We also treat filtered key release events the same way, as the IME code in // EditorClient doesn't get to look at them, as for key press events. I can also try to simplify the cases above to be like: if (filtered && !m_composingTextCurrently && !m_preeditChanged && m_confirmedComposition.length() == 1) { ... } if (filtered && event->type == GDK_KEY_PRESS) { ... } if (event->type == GDK_KEY_RELEASE && lastFilteredKeyPressCodeWithNoResults == event->keyval) return true; // Now we should only have unfiltered key events that produced results and filtered key up events.
Committed r116114: <http://trac.webkit.org/changeset/116114>
Comment on attachment 139043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139043&action=review > Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445 > + ASSERT(platformEvent->string().isNull()); It broke the GTK debug build: ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp: In member function ‘bool WebKit::EditorClient::handleInputMethodKeyboardEvent(WebCore::KeyboardEvent*)’: ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445: error: ‘const class WebCore::PlatformKeyboardEvent’ has no member named ‘string’
(In reply to comment #7) > (From update of attachment 139043 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139043&action=review > > > Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445 > > + ASSERT(platformEvent->string().isNull()); > > It broke the GTK debug build: > > ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp: In member function ‘bool WebKit::EditorClient::handleInputMethodKeyboardEvent(WebCore::KeyboardEvent*)’: > ../../Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:445: error: ‘const class WebCore::PlatformKeyboardEvent’ has no member named ‘string’ Thanks for letting me know. I fixed the GTK+ build here: http://trac.webkit.org/changeset/116135