Bug 190487

Summary: [iOS] WebKit should dispatch DOM events when a modifier key is pressed
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit2Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, megan_gardner, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar, PlatformOnly
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 190571, 191120, 191967    
Attachments:
Description Flags
Patch
none
Patch and test thorton: review+

Description Daniel Bates 2018-10-11 14:26:34 PDT
Part of <rdar://problem/44930119>.

Add support for recognizing WebEvents that represent changes to modifier flags (e.g. the Command key).
Comment 1 Daniel Bates 2018-10-11 16:54:39 PDT
Created attachment 352104 [details]
Patch
Comment 2 Daniel Bates 2018-10-30 16:40:16 PDT
Created attachment 353432 [details]
Patch and test

Updated patch a similar change to -[WebEvent initWithKeyEventType:timeStamp:characters:charactersIgnoringModifiers:modifiers:isRepeating:withFlags:withInputManagerHint:keyCode:isTabKey:] as I did for -[WebEvent initWithKeyEventType:timeStamp:characters:charactersIgnoringModifiers:modifiers:isRepeating:withFlags:keyCode:isTabKey:characterSet:].
Comment 3 Tim Horton 2018-11-01 16:32:17 PDT
Comment on attachment 353432 [details]
Patch and test

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

> Source/WebCore/platform/ios/WebEvent.h:59
> +// These enum values are copied directly from GSEvent for compatibility.

Really?

> Source/WebCore/platform/ios/WebEvent.h:87
> +// These enum values are copied directly from GSEvent for compatibility.

Don't look "copied directly" to me.

> Tools/ChangeLog:8
> +        Update the code for renamed enumerators. For WebKitTestRunner, removed comment that does not

Is this "enumerations" not "enumerators"?
Comment 4 Daniel Bates 2018-11-02 10:09:24 PDT
Comment on attachment 353432 [details]
Patch and test

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

>> Source/WebCore/platform/ios/WebEvent.h:59
>> +// These enum values are copied directly from GSEvent for compatibility.
> 
> Really?

Copied and renamed just like all the other enums in this file that have the same comment above them. I take it you would like the comment to be more precise. Before landing I will change this comment and all added comments to read: "These enum values correspond to the kGSEvent* enumerators for compatibility."
Comment 5 Daniel Bates 2018-11-02 10:13:40 PDT
(In reply to Daniel Bates from comment #4)
> Comment on attachment 353432 [details]
> Patch and test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353432&action=review
> 
> >> Source/WebCore/platform/ios/WebEvent.h:59
> >> +// These enum values are copied directly from GSEvent for compatibility.
> > 
> > Really?
> 
> Copied and renamed just like all the other enums in this file that have the
> same comment above them. I take it you would like the comment to be more
> precise. Before landing I will change this comment and all added comments to
> read: "These enum values correspond to the kGSEvent* enumerators for
> compatibility."

Will be even more precise and reference the framework that has these enumerators:

These enum values correspond to the GraphicsServices kGSEventFlagMask* enumerators for compatibility.

Will also change the comment "These enum values are copied directly from GSEvent for compatibility." above the WebEventCharacterSet enum to read "These enum values correspond to the GraphicsServices kGSCharacterSet* constants for compatibility." before landing.
Comment 6 Daniel Bates 2018-11-02 10:17:18 PDT
Committed r237738: <https://trac.webkit.org/changeset/237738>
Comment 7 Daniel Bates 2018-11-02 10:18:06 PDT
(In reply to Tim Horton from comment #3)
> > Tools/ChangeLog:8
> > +        Update the code for renamed enumerators. For WebKitTestRunner, removed comment that does not
> 
> Is this "enumerations" not "enumerators"?

For completeness, I fixed this in this ChangeLog and all other ChangeLogs in this patch.