Bug 184231 – REGRESSION(r228260): WebHTMLView beeps at every keydown for Chinese/Japanese/Korean Input Method

• Component: UI Events
• Status: RESOLVED FIXED

[Ryosuke Niwa]

WebHTMLView beep at every keydown for Chinese/Japanese/Korean Input Method after r228260.

<rdar://problem/38092985>

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 336972 [details]
Fixes the bug

[Ryosuke Niwa]

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).

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

(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.

[Jiewen Tan]

(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:]?

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

Comment on attachment 336972 [details]
Fixes the bug

Also, official r+, and you can consider the comments.

[Ryosuke Niwa]

(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!

[Ryosuke Niwa]

Committed r230173: <https://trac.webkit.org/changeset/230173>

[Ryosuke Niwa]

Tracking the testing aspect of it in https://bugs.webkit.org/show_bug.cgi?id=184244.