184231 2018-04-01 23:51:52 -0700 REGRESSION(r228260): WebHTMLView beeps at every keydown for Chinese/Japanese/Korean Input Method 2018-04-04 09:04:04 -0700 1 1 1 Unclassified WebKit UI Events WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 184244 1 rniwa rniwa ap cdumez darin dbates jiewen_tan oldest_to_newest 1411013 0 rniwa 2018-04-01 23:51:52 -0700 WebHTMLView beep at every keydown for Chinese/Japanese/Korean Input Method after r228260. <rdar://problem/38092985> 1411014 1 rniwa 2018-04-01 23:52:26 -0700 The problem here is that EventHandler::internalKeyEvent sets setDefaultHandled when the key event is handled by input method. Because we clear m_defaultHandled flag before dispatching the event after r228260, EventHandler::keyEvent ends up returning false and resulting in the beep. Arguably, this is a misuse of m_defaultHandled flag. 1411016 2 336972 rniwa 2018-04-02 00:41:00 -0700 Created attachment 336972 Fixes the bug 1411017 3 rniwa 2018-04-02 00:42:11 -0700 It's really upsetting that there isn't s a way to write a test for this. But the problem here is that there doesn't seem to be a way of detecting this case inside a layout test, and in API tests, triggering the right input method is virtually impossible (we can't require runners of API tests to always have Chinese Pinyin keyboard available for example). 1411089 4 336972 ap 2018-04-02 09:21:58 -0700 Comment on attachment 336972 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=336972&action=review > Source/WebCore/ChangeLog:13 > + Unfortunately, no new tests since there is no facility to detect this case in layout tests, and we can't > + easily emulate or trigger a real input method in API tests. What aspects of an input method need to be emulated here? We certainly can't detect a beep, but any input method functions can be triggered via textInputController. The code change looks fine to me, however I don't understand the motivation for changing this behavior in r228260. The ChangeLog says "Clears m_defaultHandled so a value left over from a previous dispatch doesn't affect the next dispatch", but it doesn't explain why that's desirable, and doesn't add a regression test for the change. 1411201 5 rniwa 2018-04-02 12:52:26 -0700 (In reply to Alexey Proskuryakov from comment #4) > Comment on attachment 336972 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336972&action=review > > > Source/WebCore/ChangeLog:13 > > + Unfortunately, no new tests since there is no facility to detect this case in layout tests, and we can't > > + easily emulate or trigger a real input method in API tests. > > What aspects of an input method need to be emulated here? We certainly can't > detect a beep, but any input method functions can be triggered via > textInputController. That the event gets processed by input methods. Basically, we need handleInputMethodKeydown to set defaultHandled, and then we'd need a way of detecting the return value of EventHandler::internalKeyEvent. > The code change looks fine to me, however I don't understand the motivation > for changing this behavior in r228260. The ChangeLog says "Clears > m_defaultHandled so > a value left over from a previous dispatch doesn't affect the next > dispatch", but it doesn't explain why that's desirable, and doesn't add a > regression test for the change. Yeah, another option is to revert that aspect of the change. For now, I'm making this very targeted fix for the very egregious issue of beeping every time anyone times in CJK. 1411205 6 jiewen_tan 2018-04-02 12:55:13 -0700 (In reply to Ryosuke Niwa from comment #3) > It's really upsetting that there isn't s a way to write a test for this. But > the problem here is that there doesn't seem to be a way of detecting this > case inside a layout test, and in API tests, triggering the right input > method is virtually impossible (we can't require runners of API tests to > always have Chinese Pinyin keyboard available for example). For testing, could we somehow fake a KeyDown Event in either JS code or native code that mimic the one that could be fired by Pinyin input in an API test, and then use a swizzler to capture [NSResponder keyDown:]? 1411206 7 ap 2018-04-02 12:55:50 -0700 > Basically, we need handleInputMethodKeydown to set defaultHandled, and then we'd need a way of detecting the return value of EventHandler::internalKeyEvent. I see. There is some limited support for posing JS code as an input method, but not a lot, and I don't remember much about it from the top of my head. So probably not very testable indeed. 1411207 8 336972 ap 2018-04-02 12:56:14 -0700 Comment on attachment 336972 Fixes the bug Also, official r+, and you can consider the comments. 1411208 9 rniwa 2018-04-02 12:57:53 -0700 (In reply to Alexey Proskuryakov from comment #7) > > Basically, we need handleInputMethodKeydown to set defaultHandled, and then we'd need a way of detecting the return value of EventHandler::internalKeyEvent. > > I see. There is some limited support for posing JS code as an input method, > but not a lot, and I don't remember much about it from the top of my head. > So probably not very testable indeed. Yeah, I feel really bad landing this patch without a test given the lack of testing is probably exactly why this regression got introduced. But I have to think about it more to figure out a way of testing, and we probably shouldn't block landing this patch on that. Thanks for the review! 1411209 10 rniwa 2018-04-02 12:59:44 -0700 Committed r230173: <https://trac.webkit.org/changeset/230173> 1411212 11 rniwa 2018-04-02 13:02:11 -0700 Tracking the testing aspect of it in https://bugs.webkit.org/show_bug.cgi?id=184244. 336972 2018-04-02 00:41:00 -0700 2018-04-02 12:56:14 -0700 Fixes the bug bug-184231-20180402004058.patch text/plain 2352 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzMDE0OSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDE4LTA0LTAyICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIFJFR1JFU1NJT04ocjIyODI2MCk6 V2ViSFRNTFZpZXcgYmVlcHMgYXQgZXZlcnkga2V5ZG93biBmb3IgQ2hpbmVzZS9KYXBhbmVzZS9L b3JlYW4gSW5wdXQgTWV0aG9kCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xODQyMzEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBUaGUgYnVnIHdhcyBjYXVzZWQgYnkgRXZlbnRIYW5kbGVyOjppbnRlcm5hbEtl eUV2ZW50IGNhbGxpbmcgc2V0RGVmYXVsdEhhbmRsZWQgYW5kIGV4cGVjdGluZyBpdCB0byBzdGF5 IHRydWUKKyAgICAgICAgYWZ0ZXIgZGlzcGF0Y2hpbmcgdGhlIGV2ZW50IGV2ZW4gdGhvdWdoIG1f ZGVmYXVsdEhhbmRsZWQgaXMgYWx3YXlzIGNsZWFyZWQgYWZ0ZXIgcjIyODI2MC4gVGhpcyByZXN1 bHRzIGluCisgICAgICAgIEV2ZW50SGFuZGxlcjo6aW50ZXJuYWxLZXlFdmVudCByZXR1cm5pbmcg ZmFsc2UsIGFuZCByZXN1bHRpbmcgaW4gYSBiZWVwLgorCisgICAgICAgIFVuZm9ydHVuYXRlbHks IG5vIG5ldyB0ZXN0cyBzaW5jZSB0aGVyZSBpcyBubyBmYWNpbGl0eSB0byBkZXRlY3QgdGhpcyBj YXNlIGluIGxheW91dCB0ZXN0cywgYW5kIHdlIGNhbid0CisgICAgICAgIGVhc2lseSBlbXVsYXRl IG9yIHRyaWdnZXIgYSByZWFsIGlucHV0IG1ldGhvZCBpbiBBUEkgdGVzdHMuCisKKyAgICAgICAg KiBwYWdlL0V2ZW50SGFuZGxlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpFdmVudEhhbmRsZXI6 OmludGVybmFsS2V5RXZlbnQpOgorCiAyMDE4LTA0LTAxICBZdXN1a2UgU3V6dWtpICA8dXRhdGFu ZS50ZWFAZ21haWwuY29tPgogCiAgICAgICAgIFVzZSBXVEY6OkxvY2sgaW5zdGVhZCBvZiBHTXV0 ZXgKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BhZ2UvRXZlbnRIYW5kbGVyLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViQ29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAJKHJldmlzaW9uIDIzMDEy NCkKKysrIFNvdXJjZS9XZWJDb3JlL3BhZ2UvRXZlbnRIYW5kbGVyLmNwcAkod29ya2luZyBjb3B5 KQpAQCAtMzI3MiwxMyArMzI3MiwxNSBAQCBib29sIEV2ZW50SGFuZGxlcjo6aW50ZXJuYWxLZXlF dmVudChjb25zCiAgICAgICAgIGtleWRvd24tPnN0b3BQcm9wYWdhdGlvbigpOwogCiAgICAgZWxl bWVudC0+ZGlzcGF0Y2hFdmVudChrZXlkb3duKTsKKyAgICBpZiAoaGFuZGxlZEJ5SW5wdXRNZXRo b2QpCisgICAgICAgIHJldHVybiB0cnVlOwogCiAgICAgLy8gSWYgZnJhbWUgY2hhbmdlZCBhcyBh IHJlc3VsdCBvZiBrZXlkb3duIGRpc3BhdGNoLCB0aGVuIHJldHVybiBlYXJseSB0byBhdm9pZCBz ZW5kaW5nIGEgc3Vic2VxdWVudCBrZXlwcmVzcyBtZXNzYWdlIHRvIHRoZSBuZXcgZnJhbWUuCiAg ICAgYm9vbCBjaGFuZ2VkRm9jdXNlZEZyYW1lID0gbV9mcmFtZS5wYWdlKCkgJiYgJm1fZnJhbWUg IT0gJm1fZnJhbWUucGFnZSgpLT5mb2N1c0NvbnRyb2xsZXIoKS5mb2N1c2VkT3JNYWluRnJhbWUo KTsKICAgICBib29sIGtleWRvd25SZXN1bHQgPSBrZXlkb3duLT5kZWZhdWx0SGFuZGxlZCgpIHx8 IGtleWRvd24tPmRlZmF1bHRQcmV2ZW50ZWQoKSB8fCBjaGFuZ2VkRm9jdXNlZEZyYW1lOwotICAg IGlmIChoYW5kbGVkQnlJbnB1dE1ldGhvZCB8fCAoa2V5ZG93blJlc3VsdCAmJiAhYmFja3dhcmRD b21wYXRpYmlsaXR5TW9kZSkpCisgICAgaWYgKGtleWRvd25SZXN1bHQgJiYgIWJhY2t3YXJkQ29t cGF0aWJpbGl0eU1vZGUpCiAgICAgICAgIHJldHVybiBrZXlkb3duUmVzdWx0OwotICAgIAorCiAg ICAgLy8gRm9jdXMgbWF5IGhhdmUgY2hhbmdlZCBkdXJpbmcga2V5ZG93biBoYW5kbGluZywgc28g cmVmZXRjaCBlbGVtZW50LgogICAgIC8vIEJ1dCBpZiB3ZSBhcmUgZGlzcGF0Y2hpbmcgYSBmYWtl IGJhY2t3YXJkIGNvbXBhdGliaWxpdHkga2V5cHJlc3MsIHRoZW4gd2UgcHJldGVuZCB0aGF0IHRo ZSBrZXlwcmVzcyBoYXBwZW5lZCBvbiB0aGUgb3JpZ2luYWwgZWxlbWVudC4KICAgICBpZiAoIWtl eWRvd25SZXN1bHQpIHsK