<rdar://problem/60539389>
Created attachment 404213 [details] Patch
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`?
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)
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?
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)
Created attachment 404270 [details] For EWS
Committed r264376: <https://trac.webkit.org/changeset/264376> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404270 [details].