Bug 129812 – [Mac] Don't perform a round-trip through WebProcess before interpreting key events

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

WebKit2 currently sends key events to WebProcess, which immediately bounces them back for -interpretKeyEvents:. This is unnecessarily complicated. Also, this means that keyboard and mouse events are not handled as part of the same event stream, which is not great.

We capture all commands that result from key interpretation for later execution anyway, so it's doesn't matter when we do this. Input method actions are not captured, but they happen before DOM key event dispatch.

[Alexey Proskuryakov]

Created attachment 226044 [details]
proposed patch

[Darin Adler]

Comment on attachment 226044 [details]
proposed patch

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

> Source/WebCore/dom/KeyboardEvent.cpp:105
> +#if USE(APPKIT)

USE(APPKIT) here but PLATFORM(COCOA) in the header?

> Source/WebKit2/Shared/NativeWebKeyboardEvent.h:63
> +    NativeWebKeyboardEvent(NSEvent *, BOOL handledByInputMethod, const Vector<WebCore::KeypressCommand>&);

Why not bool instead of BOOL?

> Source/WebKit2/Shared/WebEvent.h:223
> +#if USE(APPKIT)
> +    WebKeyboardEvent(Type, const String& text, const String& unmodifiedText, const String& keyIdentifier, int windowsVirtualKeyCode, int nativeVirtualKeyCode, int macCharCode, bool handledByInputMethod, const Vector<WebCore::KeypressCommand>&, bool isAutoRepeat, bool isKeypad, bool isSystemKey, Modifiers, double timestamp);
> +#else
>      WebKeyboardEvent(Type, const String& text, const String& unmodifiedText, const String& keyIdentifier, int windowsVirtualKeyCode, int nativeVirtualKeyCode, int macCharCode, bool isAutoRepeat, bool isKeypad, bool isSystemKey, Modifiers, double timestamp);
> +#endif

Should we do something with a structure instead of these ridiculously long argument lists?

> Source/WebKit2/Shared/WebKeyboardEvent.cpp:39
> +#if USE(APPKIT)
> +WebKeyboardEvent::WebKeyboardEvent(Type type, const String& text, const String& unmodifiedText, const String& keyIdentifier, int windowsVirtualKeyCode, int nativeVirtualKeyCode, int macCharCode, bool handledByInputMethod, const Vector<WebCore::KeypressCommand>& commands, bool isAutoRepeat, bool isKeypad, bool isSystemKey, Modifiers modifiers, double timestamp)

Missing blank line here.

> Source/WebKit2/Shared/WebKeyboardEvent.cpp:72
>  }
> +#endif

Missing blank line here.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:348
> +        _data->_page->setInitialFocus(direction == NSSelectingNext, keyboardEvent != nil, NativeWebKeyboardEvent(keyboardEvent, false, Vector<KeypressCommand>()));

This false/empty-vector stuff is a bit ugly. Should these be default argument values instead perhaps?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1310
> +    if ([[event charactersIgnoringModifiers] isEqualToString:@"\e"] && !([event modifierFlags] & NSDeviceIndependentModifierFlagsMask))
> +        return [super performKeyEquivalent:event];

Hard-coding a specific key string and modifiers seems a more fragile rule than what the old code had. Is there some way to handle this without that?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:240
>      bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*);

Seems like this should take a reference, not a pointer. Next time, maybe.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:213
> +    const Vector<KeypressCommand>& commands = event->keypressCommands();

I think this would read better with auto.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:219
> +    // Don't handle Esc while handling keydown event, we need to dispatch a keypress first.
> +    if (platformEvent->type() != PlatformEvent::Char && event->windowsVirtualKeyCode() == VK_ESCAPE && commands.size() == 1 && commandNameForSelectorName(commands[0].commandName) == "cancelOperation")
>          return false;

Really unfortunate to have this key hard-coded. Also seems strange to convert the selector name into a command name just to do == on it.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:231
> +    for (size_t i = 0; i < commands.size(); ++i) {
> +        if (frame->editor().command(commandNameForSelectorName(commands[i].commandName)).isTextInsertion())
> +            haveTextInsertionCommands = true;
> +    }

This should be a C++ for loop:

    for (auto& command : commands) {
        if (frame->editor().command(commandNameForSelectorName(command.commandName)).isTextInsertion())
            haveTextInsertionCommands = true;
    }

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:294
> +        handled = frame.editor().insertText(text, 0);

Can this be nullptr instead of 0?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:314
> +    handled = frame.editor().insertDictatedText(text, dictationAlternativeLocations, 0);

Can this be nullptr instead of 0?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:428
> +    handled = executeKeypressCommandsInternal(commands, 0);

Can this be nullptr instead of 0?

[Alexey Proskuryakov]

Comment on attachment 226044 [details]
proposed patch

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

Will address all other comments, and fix the builds.

>> Source/WebKit2/Shared/WebEvent.h:223
>> +#endif
> 
> Should we do something with a structure instead of these ridiculously long argument lists?

Maybe! I'm not sure if I fully understand the design here, why do we have events and event factories?

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:348
>> +        _data->_page->setInitialFocus(direction == NSSelectingNext, keyboardEvent != nil, NativeWebKeyboardEvent(keyboardEvent, false, Vector<KeypressCommand>()));
> 
> This false/empty-vector stuff is a bit ugly. Should these be default argument values instead perhaps?

I'd like to keep it ugly for now to maybe pass something more appropriate than an event. It's not clear why setInitialFocus needs a keyboard event.

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:1310
>> +        return [super performKeyEquivalent:event];
> 
> Hard-coding a specific key string and modifiers seems a more fragile rule than what the old code had. Is there some way to handle this without that?

I think that it's more accurate too - the Esc key is specifically what's passed to performKeyEquivalent: from AppKit as an exception, there is no generality here.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:240
>>      bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*);
> 
> Seems like this should take a reference, not a pointer. Next time, maybe.

Yes, there is a good amount of cleanup to do, especially once all platforms follow suit.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:213
>> +    const Vector<KeypressCommand>& commands = event->keypressCommands();
> 
> I think this would read better with auto.

I prefer to keep the type, I like to see what my variables actually are.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:219
>>          return false;
> 
> Really unfortunate to have this key hard-coded. Also seems strange to convert the selector name into a command name just to do == on it.

Again, I think that this is the correct way to handle the Esc key. An alternative would be to add a CancelOperation EditorCommand, and mark it as text insertion command, but that would have its own downsides:

1. That would be a hack, as it's not really a text insertion command, despite the need to dispatch keypress event.

2. We would dispatch key press for Cmd+period, and theoretically for any user defined key bindings that generate cancelOperation:, which would be wrong.

Calling commandNameForSelectorName is silly indeed. I did this because KeypressCommand.commandName is poorly defined, it may be either a selector or a command name, and commandNameForSelectorName normalizes that. This should be cleaned up separately.

[Alexey Proskuryakov]

Created attachment 226148 [details]
proposed patch

[Darin Adler]

Comment on attachment 226148 [details]
proposed patch

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

> Source/WebCore/dom/KeyboardEvent.h:102
> +    const Vector<KeypressCommand>& keypressCommands() const { return m_keypressCommands; }
> +
> +    // This is only needed for WebKit1 - with WebKit2, the commands are delivered from UI process.
>      Vector<KeypressCommand>& keypressCommands() { return m_keypressCommands; }

I just noticed the strange fact that the const overload for this is in a different paragraph from the non-const overload. You should fix that.

> Source/WebCore/platform/KeypressCommand.h:34
> +#if PLATFORM(COCOA)

Should this wrap the whole file instead of just the struct?

> Source/WebCore/platform/KeypressCommand.h:45
> +
> +

Extra blank space here.

[Alexey Proskuryakov]

Comment on attachment 226148 [details]
proposed patch

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

>> Source/WebCore/dom/KeyboardEvent.h:102
>>      Vector<KeypressCommand>& keypressCommands() { return m_keypressCommands; }
> 
> I just noticed the strange fact that the const overload for this is in a different paragraph from the non-const overload. You should fix that.

This is intentional - as a comment above states, the non-const override is only needed for ports that still interpret key events after creating them.

I should restate the comment in a clearer way.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r165356>.

[Jeff Miller]

This appears to have caused <rdar://problem/16387169>.

[Alexey Proskuryakov]

Yes, there were two or three regressions from this change, all known ones fixed already.