Bug 204694 – [Win] Retrieve all following WM_CHAR events at the beginning of processing WM_KEYDOWN event

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

[Fujii Hironori]

[Win] Retrieve all following WM_CHAR events at the beginning of processing WM_KEYDOWN event

In Windows ports, WM_KEYDOWN dispatches keydown event, and
WM_CHAR dispatches keypress event. If a keydown event is canceled
by calling preventDefault, the following corresponding keypress
events shouldn't be dispatched.

WebKit1 implements it by removing WM_CHAR events if the keydown
event is consumed. However, WebKit2 can't do so because WebKit2
processes key events asynchronously. Thus, retrieve all following
WM_CHAR events, and dispatch them after processing the keydown
and it is not consumed.

In addition to that, retrieving following WM_CHAR events are
needed to fix Bug 204672 because the events are needed for 'key'
property of keydown KeyboardEvent for dead key combination.

Gecko and Chromium also implements 'key' property in the same approach.

Bug 204672 – [Win] 'key' property of keydown KeyboardEvent doesn't have the diacritic for alphabet key following a dead key
Bug 200558 – [Win][WebKit2] Can't prevent input events by canceling keydown events

[Fujii Hironori]

Created attachment 384481 [details]
Patch

[Fujii Hironori]

Created attachment 384492 [details]
Patch

[Ross Kirsling]

Comment on attachment 384492 [details]
Patch

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

> Source/WebKit/ChangeLog:25
> +        Test: Covered by existing fast/events/inputText-never-fired-on-keydown-cancel.html and fast/events/keydown-keypress-preventDefault.html

Shouldn't there be some update to TestExpectations then?

[Fujii Hironori]

Comment on attachment 384492 [details]
Patch

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

>> Source/WebKit/ChangeLog:25
>> +        Test: Covered by existing fast/events/inputText-never-fired-on-keydown-cancel.html and fast/events/keydown-keypress-preventDefault.html
> 
> Shouldn't there be some update to TestExpectations then?

No. These tests are failing only for WinCairo WebKit2.

[Fujii Hironori]

Comment on attachment 384492 [details]
Patch

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

>>> Source/WebKit/ChangeLog:25
>>> +        Test: Covered by existing fast/events/inputText-never-fired-on-keydown-cancel.html and fast/events/keydown-keypress-preventDefault.html
>> 
>> Shouldn't there be some update to TestExpectations then?
> 
> No. These tests are failing only for WinCairo WebKit2.

I mean these test aren’t marked as Failure.

[Ross Kirsling]

Comment on attachment 384492 [details]
Patch

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

r=me but let's change the naming somehow.

>>>> Source/WebKit/ChangeLog:25
>>>> +        Test: Covered by existing fast/events/inputText-never-fired-on-keydown-cancel.html and fast/events/keydown-keypress-preventDefault.html
>>> 
>>> Shouldn't there be some update to TestExpectations then?
>> 
>> No. These tests are failing only for WinCairo WebKit2.
> 
> I mean these test aren’t marked as Failure.

I see. We sure do need a test bot, hehe...

> Source/WebKit/Shared/NativeWebKeyboardEvent.h:110
> +    Vector<MSG> m_followingCharEvents;

I think "pending" might be a better word.

(followingCharEvents kind of reads like isFollowingCharEvents with the "is" dropped. "subsequent" might also work if you really want to say "comes after" and not "yet to be processed".)

[Fujii Hironori]

Created attachment 384589 [details]
Patch for landing

Thank you for the review.

* Renamed following → pending

[Fujii Hironori]

Comment on attachment 384589 [details]
Patch for landing

Clearing flags on attachment: 384589

Committed r252976: <https://trac.webkit.org/changeset/252976>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57548463>

[Fujii Hironori]

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