[iOS] The completion handler in -handleKeyWebEvent:withCompletionHandler: is sometimes never called

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/60539389>

[Wenson Hsieh]

Created attachment 404213 [details]
Patch

[Devin Rousso]

Comment on attachment 404213 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1068
> +    if (!_page || !_page->hasQueuedKeyEvent())

Is it possible for `_keyWebEventHandler` to be set without `_page->hasQueuedKeyEvent()` (or vice versa)?  I feel like this should be an `ASSERT` inside the `if` below to ensure that the state is synchronized.

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:376
> +        auto keyDownEvent = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyDown timeStamp:CFAbsoluteTimeGetCurrent() characters:@"a" charactersIgnoringModifiers:@"a" modifiers:0 isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:0 isTabKey:NO]);

NIT: shouldn't `keyCode` be `65`?

[Devin Rousso]

Comment on attachment 404213 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:389
> +        auto keyUpEvent = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyUp timeStamp:CFAbsoluteTimeGetCurrent() characters:@"a" charactersIgnoringModifiers:@"a" modifiers:0 isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:0 isTabKey:NO]);

Ditto (:376)

[Devin Rousso]

Comment on attachment 404213 [details]
Patch

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

> Source/WebKit/ChangeLog:14
> +        This can happen in several ways. For instance, if the web process is swapped or terminates in the middle of

Can you also add a test for the PSON case?

[Wenson Hsieh]

Comment on attachment 404213 [details]
Patch

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

>> Source/WebKit/ChangeLog:14
>> +        This can happen in several ways. For instance, if the web process is swapped or terminates in the middle of
> 
> Can you also add a test for the PSON case?

It was more difficult to craft a test case for this, which is why I tested using a crashed web process instead (which exercises the same codepath). I’ll try to come up with a test that involves PSON...

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1068
>> +    if (!_page || !_page->hasQueuedKeyEvent())
> 
> Is it possible for `_keyWebEventHandler` to be set without `_page->hasQueuedKeyEvent()` (or vice versa)?  I feel like this should be an `ASSERT` inside the `if` below to ensure that the state is synchronized.

I think it’s possible to have queued key events without a completion handler, in the case where the user resigns first responder (e.g. by focusing the unified field) and we bail out of the queued event handler early.

However, I think having a key event handler stashed away should always mean that we have a queued key event, so I’ll add an assertion to enforce this.

>> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:376
>> +        auto keyDownEvent = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyDown timeStamp:CFAbsoluteTimeGetCurrent() characters:@"a" charactersIgnoringModifiers:@"a" modifiers:0 isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:0 isTabKey:NO]);
> 
> NIT: shouldn't `keyCode` be `65`?

👍🏻

>> Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:389
>> +        auto keyUpEvent = adoptNS([[WebEvent alloc] initWithKeyEventType:WebEventKeyUp timeStamp:CFAbsoluteTimeGetCurrent() characters:@"a" charactersIgnoringModifiers:@"a" modifiers:0 isRepeating:NO withFlags:0 withInputManagerHint:nil keyCode:0 isTabKey:NO]);
> 
> Ditto (:376)

👍🏻

(There’s a bunch more in this file, but I’ll clean them up in a separate patch)

[Wenson Hsieh]

Created attachment 404270 [details]
For EWS

[EWS]

Committed r264376: <https://trac.webkit.org/changeset/264376>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404270 [details].