Bug 193452

Summary: [iOS] Make Windows virtual key code computation match Mac
Product: WebKit Reporter: Daniel Bates <dbates>
Component: PlatformAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: ews, megan_gardner, rniwa, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar, PlatformOnly
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: iOS 12   
Bug Depends on:    
Bug Blocks: 190571    
Attachments:
Description Flags
Patch and layout tests
none
To Land none

Description Daniel Bates 2019-01-15 09:59:04 PST
Make Window virtual key code computation match Mac.
Comment 1 Daniel Bates 2019-01-15 10:13:32 PST
Created attachment 359173 [details]
Patch and layout tests
Comment 2 Daniel Bates 2019-01-15 10:13:52 PST
Although the test will fail (or may even timeout) when using a build of WebKit with the public SDK I am curious to see what the results are. I will amend the patch before landing as appropriate to ensure that this patch does not cause test failures.
Comment 3 Build Bot 2019-01-15 10:15:50 PST
Attachment 359173 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:427:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Daniel Bates 2019-01-25 14:47:51 PST
(In reply to Daniel Bates from comment #2)
> Although the test will fail (or may even timeout) when using a build of
> WebKit with the public SDK I am curious to see what the results are. I will
> amend the patch before landing as appropriate to ensure that this patch does
> not cause test failures.

It didn't fail. I'm a bit surprised, but it could be that the test isn't comprehensive enough (say, testing function keys and arrow keys, et cetera). I do not suspect the test passing represents a bug in the patch. It's hard to keep all the bugs I've fixed and the side effects of each bug in my head. I'm happy the test passes. We'll see if we can keep the test green and avoid skipping it with the public SDK in a future iteration  where I make it more comprehensive.
Comment 5 Ryosuke Niwa 2019-01-25 14:59:52 PST
Comment on attachment 359173 [details]
Patch and layout tests

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

> LayoutTests/fast/events/ios/resources/key-tester.js:39
> +    constructor(key, modifiers = []) {

Put { on new line to be consistent?

> LayoutTests/fast/events/ios/resources/key-tester.js:77
> +        for (let j = i, k = 0; j; j = Math.floor(j / 2), ++k) {

Can we use more describe variable names than i and j?
It's a bit confusing as to what's happening here.

> LayoutTests/fast/events/ios/resources/key-tester.js:95
> +let disallowedKeyCommands = [

Use const?

> LayoutTests/fast/events/ios/resources/key-tester.js:115
> +let tests = [];

Ditto.

> LayoutTests/fast/events/ios/resources/key-tester.js:181
> +

Maybe remove this blank line?

> LayoutTests/fast/events/ios/resources/key-tester.js:201
> +    numberOfKeyUpsRemaining = currentTest.modifiers.length + 1 /* non-modifier key */;

Instead of adding a comment, why don't we declare a local variable named nonModiferKey = ~?

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:425
> +    // Check that this is the type of event that has a keyCode.

Can we just extract the switch below as a helper function like eventHasKeyCode(~) and remove this comment?
This function is quite comment heavy.

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:434
> +    // With the exception of keypad comma, the following corresponds to the criterion for UIKeyModifierNumericPad.

criterion to decide what?
Comment 6 Ryosuke Niwa 2019-01-25 15:03:47 PST
Comment on attachment 359173 [details]
Patch and layout tests

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

> LayoutTests/fast/events/ios/resources/key-tester.js:166
> +    for (let propertyName of ["type", "key", "code", "keyIdentifier", "keyCode", "charCode", "keyCode", "which", "altKey", "ctrlKey", "metaKey", "shiftKey"])

Why not also dump location / keyLocation?
Comment 7 Daniel Bates 2019-01-25 15:56:09 PST
Comment on attachment 359173 [details]
Patch and layout tests

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

>> LayoutTests/fast/events/ios/resources/key-tester.js:39
>> +    constructor(key, modifiers = []) {
> 
> Put { on new line to be consistent?

OK.

>> LayoutTests/fast/events/ios/resources/key-tester.js:77
>> +        for (let j = i, k = 0; j; j = Math.floor(j / 2), ++k) {
> 
> Can we use more describe variable names than i and j?
> It's a bit confusing as to what's happening here.

Changed this code to read:

// For each ordinal 1, 2, ... numberOfNonEmptyPermutations we look at its binary representation
// and generate a permutation that consists of the entries in anArray at the indices where there
// is a one bit in the binary representation. For example, suppose anArray = ["metaKey", "altKey", "ctrlKey"].
// To generate the 5th permutation we look at the binary representation of i = 5 => 0b101. And
// compute the permutation to be [ anArray[0], anArray[2] ] = [ "metaKey", "ctrlKey" ] because
// the 0th and 2nd bits are ones in the binary representation.
for (let i = 1; i <= numberOfNonEmptyPermutations; ++i) {
    let temp = [];
    for (let bitmask = i, j = 0; bitmask; bitmask = Math.floor(bitmask / 2), ++j) {
        if (bitmask % 2)
            temp.push(anArray[j]);
    }
    result.push(temp);
}

>> LayoutTests/fast/events/ios/resources/key-tester.js:95
>> +let disallowedKeyCommands = [
> 
> Use const?

OK.

>> LayoutTests/fast/events/ios/resources/key-tester.js:115
>> +let tests = [];
> 
> Ditto.

OK.

>> LayoutTests/fast/events/ios/resources/key-tester.js:166
>> +    for (let propertyName of ["type", "key", "code", "keyIdentifier", "keyCode", "charCode", "keyCode", "which", "altKey", "ctrlKey", "metaKey", "shiftKey"])
> 
> Why not also dump location / keyLocation?

OK.

>> LayoutTests/fast/events/ios/resources/key-tester.js:181
>> +
> 
> Maybe remove this blank line?

Will fix.

>> LayoutTests/fast/events/ios/resources/key-tester.js:201
>> +    numberOfKeyUpsRemaining = currentTest.modifiers.length + 1 /* non-modifier key */;
> 
> Instead of adding a comment, why don't we declare a local variable named nonModiferKey = ~?

Will change this code to read:

const numberOfNonModifierKeys = 1;
numberOfKeyUpsRemaining = currentTest.modifiers.length + numberOfNonModifierKeys;

>> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:425
>> +    // Check that this is the type of event that has a keyCode.
> 
> Can we just extract the switch below as a helper function like eventHasKeyCode(~) and remove this comment?
> This function is quite comment heavy.

I'll consider this depending on whether this code can be re-used in other places. I think it can! I may defer doing this cleanup until I find the second call site and choose to do it in its own patch.

>> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:434
>> +    // With the exception of keypad comma, the following corresponds to the criterion for UIKeyModifierNumericPad.
> 
> criterion to decide what?

The criterion to decide if an event is from the numeric keypad. UIKit |= event.modifierFlags with UIKeyModifierNumericPad if the event's key code corresponds to one of the key codes in the below switch block or is a keypad comma.
Comment 8 Daniel Bates 2019-01-25 16:39:56 PST
Created attachment 360188 [details]
To Land
Comment 9 Daniel Bates 2019-01-28 15:00:51 PST
Committed r240604: <https://trac.webkit.org/changeset/240604>
Comment 10 Radar WebKit Bug Importer 2019-01-28 15:01:50 PST
<rdar://problem/47612503>
Comment 11 Daniel Bates 2019-01-28 15:08:39 PST
(In reply to Daniel Bates from comment #7)
> >> LayoutTests/fast/events/ios/resources/key-tester.js:95
> >> +let disallowedKeyCommands = [
> > 
> > Use const?
> 
> OK.
> 
> >> LayoutTests/fast/events/ios/resources/key-tester.js:115
> >> +let tests = [];
> > 
> > Ditto.
> 
> OK.

Oops! I said I would make these changes, but I forgot. When I have a moment, I will look to land a follow up fix.