WebHTMLView beep at every keydown for Chinese/Japanese/Korean Input Method after r228260. <rdar://problem/38092985>
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.
Created attachment 336972 [details] Fixes the bug
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).
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. 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.
(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.
(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:]?
> 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.
Comment on attachment 336972 [details] Fixes the bug Also, official r+, and you can consider the comments.
(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!
Committed r230173: <https://trac.webkit.org/changeset/230173>
Tracking the testing aspect of it in https://bugs.webkit.org/show_bug.cgi?id=184244.