Bug 204694

Summary: [Win] Retrieve all following WM_CHAR events at the beginning of processing WM_KEYDOWN event
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: WebKit Misc.Assignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, don.olmstead, pvollan, ross.kirsling, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 204672, 200558    
Attachments:
Description Flags
Patch
none
Patch
ross.kirsling: review+
Patch for landing none

Description Fujii Hironori 2019-11-28 23:42:23 PST
[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
Comment 1 Fujii Hironori 2019-11-28 23:55:31 PST
Created attachment 384481 [details]
Patch
Comment 2 Fujii Hironori 2019-11-29 02:26:52 PST
Created attachment 384492 [details]
Patch
Comment 3 Ross Kirsling 2019-11-29 13:05:28 PST
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?
Comment 4 Fujii Hironori 2019-11-29 14:32:52 PST
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.
Comment 5 Fujii Hironori 2019-11-29 14:34:30 PST
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.
Comment 6 Ross Kirsling 2019-11-29 15:40:24 PST
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".)
Comment 7 Fujii Hironori 2019-12-01 18:27:54 PST
Created attachment 384589 [details]
Patch for landing

Thank you for the review.

* Renamed following → pending
Comment 8 Fujii Hironori 2019-12-01 19:20:17 PST
Comment on attachment 384589 [details]
Patch for landing

Clearing flags on attachment: 384589

Committed r252976: <https://trac.webkit.org/changeset/252976>
Comment 9 Fujii Hironori 2019-12-01 19:20:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-12-01 19:21:22 PST
<rdar://problem/57548463>
Comment 11 Fujii Hironori 2019-12-01 19:21:41 PST
*** Bug 200558 has been marked as a duplicate of this bug. ***