[webkit-reviews] review denied: [Bug 28733] [GTK] GtkIMContext filtering interferes with DOM key events : [Attachment 39634] Updated patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 21 23:12:42 PDT 2009
Xan Lopez <xan.lopez at gmail.com> has denied Martin Robinson
<martin.james.robinson at gmail.com>'s request for review:
Bug 28733: [GTK] GtkIMContext filtering interferes with DOM key events
https://bugs.webkit.org/show_bug.cgi?id=28733
Attachment 39634: Updated patch
https://bugs.webkit.org/attachment.cgi?id=39634&action=review
------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
> From 577ffcebd4918a23912543b9b401e4beabb90e72 Mon Sep 17 00:00:00 2001
> From: Martin Robinson <martin.james.robinson at gmail.com>
> 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 @@
> +2009-09-15 Martin Robinson <martin.james.robinson at gmail.com>
> +
> + 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 <cfleizach at apple.com>
>
> 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 @@
> +2009-09-15 Martin Robinson <martin.james.robinson at gmail.com>
> +
> + 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 <ggaren at apple.com>
>
> 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 @@
> +2009-09-15 Martin Robinson <martin.james.robinson at gmail.com>
> +
> + 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 <gustavo.noronha at collabora.co.uk>
>
> 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.
More information about the webkit-reviews
mailing list