Bug 201913 - [Win][WebKitTestRunner] Implement EventSenderProxy::keyDown
Summary: [Win][WebKitTestRunner] Implement EventSenderProxy::keyDown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-18 02:57 PDT by Fujii Hironori
Modified: 2019-09-23 20:55 PDT (History)
7 users (show)

See Also:


Attachments
WIP patch (4.88 KB, patch)
2019-09-18 02:57 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (5.56 KB, patch)
2019-09-19 03:12 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (5.86 KB, patch)
2019-09-19 19:24 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch for landing (6.52 KB, patch)
2019-09-23 20:52 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-09-18 02:57:28 PDT
[Win][WebKitTestRunner] Implement EventSenderProxy::keyDown
Comment 1 Fujii Hironori 2019-09-18 02:57:50 PDT
Created attachment 379030 [details]
WIP patch
Comment 2 Fujii Hironori 2019-09-19 00:13:46 PDT
This WIP patch sends key events, but tests doesn't pass
expectedly because of asynchronous message passing from UI
process to Web process.

For example, fast/dom/fragment-activation-focuses-target.html has the following code.

>  eventSender.keyDown("\t");
>  shouldBe("document.activeElement", "document.getElementById('fragment3')");

This code expects the focus was moved imediately after calling keyDown.
But, key event message sent by UI process is still queued in that time.

How do other WebKit2 ports do?
Comment 3 Fujii Hironori 2019-09-19 02:35:56 PDT
Comment 2 was completely wrong.
Web process processes messages while keyEvent (Bug 148326).
Comment 4 Fujii Hironori 2019-09-19 03:12:35 PDT
Created attachment 379116 [details]
Patch
Comment 5 Ross Kirsling 2019-09-19 11:03:25 PDT
Comment on attachment 379116 [details]
Patch

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

Seems reasonable based on DRT version.

> Tools/WebKitTestRunner/win/EventSenderProxyWin.cpp:230
> +        wchar_t buff[1];
> +        WKStringGetCharacters(keyRef, buff, _countof(buff));
> +        charCode = buff[0];

Is this better than the approach taken in WebKitBrowserWindow?
https://github.com/WebKit/webkit/blob/master/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp#L46-L47

> Tools/WebKitTestRunner/win/EventSenderProxyWin.cpp:258
> +    keyData |= (KF_UP | KF_REPEAT) << 16;

I don't think I understand this line. Does this correspond to the PostMessage in the DRT implementation? Why auto-repeat?
Comment 6 Fujii Hironori 2019-09-19 19:20:13 PDT
Comment on attachment 379116 [details]
Patch

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

>> Tools/WebKitTestRunner/win/EventSenderProxyWin.cpp:230
>> +        charCode = buff[0];
> 
> Is this better than the approach taken in WebKitBrowserWindow?
> https://github.com/WebKit/webkit/blob/master/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp#L46-L47

Only the first character is needed. DRT also checks only the first character.

>> Tools/WebKitTestRunner/win/EventSenderProxyWin.cpp:258
>> +    keyData |= (KF_UP | KF_REPEAT) << 16;
> 
> I don't think I understand this line. Does this correspond to the PostMessage in the DRT implementation? Why auto-repeat?

Good catch.
I removed the PostMessage(WM_CHAR) part because I couldn't
understand why it is needed and I'm too lazy to study the reason from
the commit history.

It came from Bug 11449.
https://github.com/WebKit/webkit/commit/ca3369df7ca41a2199f9d2187642989488a94e2c

fast/forms/select-type-ahead-non-latin.html is the test case.
I need the PostMessage. Will fix.
Comment 7 Fujii Hironori 2019-09-19 19:20:28 PDT
Comment on attachment 379116 [details]
Patch

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

>>> Tools/WebKitTestRunner/win/EventSenderProxyWin.cpp:258
>>> +    keyData |= (KF_UP | KF_REPEAT) << 16;
>> 
>> I don't think I understand this line. Does this correspond to the PostMessage in the DRT implementation? Why auto-repeat?
> 
> Good catch.
> I removed the PostMessage(WM_CHAR) part because I couldn't
> understand why it is needed and I'm too lazy to study the reason from
> the commit history.
> 
> It came from Bug 11449.
> https://github.com/WebKit/webkit/commit/ca3369df7ca41a2199f9d2187642989488a94e2c
> 
> fast/forms/select-type-ahead-non-latin.html is the test case.
> I need the PostMessage. Will fix.

You are right. DRT and my WTR implementations differ in this part.

I want to reuse existing EventSenderProxy::dispatchMessage in WTR if possible.
It calls TranslateMessage, this introduces the difference.
If I didn't turn those bits, TranslateMessage posts WM_CHAR even for WM_KEYUP.

According the spec, WM_KEYUP needs those bits.

https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-keyup

> 30	The previous key state. The value is always 1 for a WM_KEYUP message.
> 31	The transition state. The value is always 1 for a WM_KEYUP message.

DRT doesn't need the those bits becasuse it doesn't use TranslateMessage for WM_KEYUP.
Comment 8 Fujii Hironori 2019-09-19 19:24:41 PDT
Created attachment 379189 [details]
Patch
Comment 9 Ross Kirsling 2019-09-20 10:44:25 PDT
Comment on attachment 379189 [details]
Patch

r=me, though it would be helpful to add some of that explanation to the ChangeLog too.
Comment 10 Fujii Hironori 2019-09-23 20:52:56 PDT
Created attachment 379421 [details]
Patch for landing
Comment 11 Fujii Hironori 2019-09-23 20:54:41 PDT
Committed r250279: <https://trac.webkit.org/changeset/250279>
Comment 12 Radar WebKit Bug Importer 2019-09-23 20:55:24 PDT
<rdar://problem/55649032>