Bug 56020 – WebKit2: Pressing Tab in Web Inspector's console does not cycle through completion options

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Adam Roben (:aroben)]

To reproduce:

1. Go to http://webkit.org/
2. Open the Inspector
3. In the Console, type "location." without the quotes
4. When the completion suggestion appears, press Tab

A Tab is inserted, rather than the next completion suggestion appearing. This does not happen on Mac, or in WebKit1.

[Adam Roben (:aroben)]

<rdar://problem/9108368>

[Jeff Miller]

This is a general problem with any WK2 web view that's not hosted inside a Safari window (e.g. the inspector).

Safari needs to call TranslateMessage() if the web view does not handle the key message. Unlike WK1, WK2 handles Windows messages asynchronously (since they need to be sent to the web process), and the WKPageUIClient has a didNotHandleKeyEvent callback that gets invoked when the web view doesn't handle the key message.

However, Safari only installs a WKPageUIClient on web views that are hosted inside a Safari window.  For web views like the inspector, it calls TranslateMessage() immediately, which in this case is causing a tab character to be generated.

I think we need a global didNotHandleKeyEvent callback into the client application for pages that don't have a WKPageUIClient installed so that Safari can defer calling TranslateMessage() until it knows whether the key message is handled.

[Jeff Miller]

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

[Jeff Miller]

Created attachment 89074 [details]
Patch

[Jeff Miller]

After discussing this with Sam, we decided that it's fine for now if the WebPageProxy calls TranslateMessage() in this case, rather than coming up with a mechanism to communicate this back to the application outside of the PageUIClient.  The patch I attached does this.

[Adam Roben (:aroben)]

Comment on attachment 89074 [details]
Patch

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

Presumably we should modify MiniBrowser not to call TranslateMessage, too.

Is there a way to make a test for this? Maybe TestWebKitAPI could see if a WM_CHAR event gets put into the message queue.

> Source/WebKit2/UIProcess/WebUIClient.h:67
> +    bool canNotHandleKeyEvent() const;

Boy, this name is confusing! I'd suggest hasDidNotHandleKeyEventCallback or similar. implementsDidNotHandleKeyEvent perhaps?

[Jeff Miller]

(In reply to comment #6)
> (From update of attachment 89074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89074&action=review
> 
> Presumably we should modify MiniBrowser not to call TranslateMessage, too.

Will do.

> 
> Is there a way to make a test for this? Maybe TestWebKitAPI could see if a WM_CHAR event gets put into the message queue.

On IRC, Adam elaborated:

"I was imagining that you'd set up a WKView with no UI client, post a WM_KEYDOWN event, and wait to see if a WM_CHAR event shows up in the message queue (without calling ::TranslateMessage yourself) in TestWebKitAPI. For bonus points you could repeat the test but with a UI client that implementes didNotHandleKeyEvent and ensure that no WM_CHAR appears."
> 
> > Source/WebKit2/UIProcess/WebUIClient.h:67
> > +    bool canNotHandleKeyEvent() const;
> 
> Boy, this name is confusing! I'd suggest hasDidNotHandleKeyEventCallback or similar. implementsDidNotHandleKeyEvent perhaps?

I was trying to follow the idiom set by WebUIClient::canRunBeforeUnloadConfirmPanel() and WebUIClient::runBeforeUnloadConfirmPanel(), but it's tough with this function name.  I'll go with implementsDidNotHandleKeyEvent().

[Jeff Miller]

Created attachment 89228 [details]
Patch

[Jeff Miller]

Created attachment 89235 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 89235 [details]
Patch

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

> Tools/ChangeLog:12
> +        WebKit2: Pressing Tab in Web Inspector's console does not cycle through completion options
> +        https://bugs.webkit.org/show_bug.cgi?id=56020
> +
> +        Don't call TranslateMessage() in the MiniBrowser or TestWebKitAPI for key messages destined for a WebKit2 view,
> +        since WebKit will do this for us.  If we didn't do this, TranslateMessage() would be called twice,
> +        which would generate two characters for every keypress (for example).
> +        
> +        Add new WebKit2/TranslateMessageGeneratesWMChar test to test expected TranslateMessage() behavior.

I think it's worth adding a comment about why we didn't bother to modify WebKitTestRunner.

> Tools/MiniBrowser/win/main.cpp:44
> +static bool shouldTranslateMessage(MSG msg)

Can use a const MSG&.

> Tools/MiniBrowser/win/main.cpp:48
> +    if ((msg.message == WM_KEYDOWN || msg.message == WM_SYSKEYDOWN || msg.message == WM_KEYUP || msg.message == WM_SYSKEYUP)) {

Making this an early return would be better.

> Tools/MiniBrowser/win/main.cpp:55
> +        TCHAR className[256];
> +        if (!::GetClassName(msg.hwnd, className, ARRAYSIZE(className)))
> +            return true;
> +
> +        // Don't call TranslateMessage() on key events destined for a WebKit2 view, WebKit will do this if it doesn't handle the message.
> +        // It would be nice to use some API here instead of hard-coding the window class name.
> +        return _tcscmp(className, _T("WebKit2WebViewWindowClass"));

In new code we've been avoiding TCHAR and using wchar_t directly. I think that would be good to do here, too.

> Tools/TestWebKitAPI/PlatformUtilities.h:41
> +bool shouldTranslateMessage(MSG);

Could pass a const MSG&.

> Tools/TestWebKitAPI/win/PlatformUtilitiesWin.cpp:36
> +#include <tchar.h>

In

> Tools/TestWebKitAPI/win/PlatformUtilitiesWin.cpp:76
> +bool shouldTranslateMessage(MSG msg)
> +{
> +    // Only these four messages are actually translated by ::TranslateMessage or ::TranslateAccelerator.
> +    // It's useless (though harmless) to call those functions for other messages, so we always allow other messages to be translated.
> +    if ((msg.message == WM_KEYDOWN || msg.message == WM_SYSKEYDOWN || msg.message == WM_KEYUP || msg.message == WM_SYSKEYUP)) {
> +        TCHAR className[256];
> +        if (!::GetClassName(msg.hwnd, className, ARRAYSIZE(className)))
> +            return true;
> +
> +        // Don't call TranslateMessage() on key events destined for a WebKit2 view, WebKit will do this if it doesn't handle the message.
> +        // It would be nice to use some API here instead of hard-coding the window class name.
> +        return _tcscmp(className, _T("WebKit2WebViewWindowClass"));
> +    }
> +    
> +    return true;
> +}

Same comments here as for MiniBrowser.

[Jeff Miller]

Committed r83633: <http://trac.webkit.org/changeset/83633>