Bug 10871 – REGRESSION: IME key events different in nightly

• Component: Text
• Status: RESOLVED FIXED

[Michael Davidson]

When typing into a textarea using an IME, current Safari gives only keyup events (not keydown or keypress) when the IME is active. Nightly webkit builds give normal key events for all keys pressed.

This is a problem for Gmail. In the compose textarea, for example, it performs an autocomplete when the enter key is pressed. In current Safari it can detect when the enter key is pressed in order to commit an IME character because it doesn't get a keydown. In the nightly build, it's impossible to detect this enter from the following enter, which should trigger a the autocomplete.

To see this, set the input method to "Katakana" and create a contact in Gmail with the first character of the name "カ" (obtained by pressing k-a-enter). With Safari on 10.4, if you type this character in the compose box, the enter key will bring up the autocomplete filtered by the first character as expected. In nightly webkit, however, the first enter key will actually do the autocompletion, which is the problem.

I have set up a page at http://bantha.org/~mdavids/ime.html to demonstrate the problem. When you type k-a-enter-enter with current Safari on 10.4, these are the key events you get:

down keycode: 75 which: 75 length: 0
press keycode: 107 which: 107 length: 0
up keycode: 75 which: 75 length: 0
up keycode: 65 which: 65 length: 0
up keycode: 13 which: 13 length: 1    // first enter, commits the character
down keycode: 13 which: 13 length: 1  // second enter, commits the autocomplete
press keycode: 13 which: 13 length: 1
up keycode: 13 which: 13 length: 1


With nightly webkit, this is what you get:

down keycode: 75 which: 75 length: 0
press keycode: 107 which: 107 length: 0
up keycode: 75 which: 75 length: 1
down keycode: 65 which: 65 length: 1
press keycode: 97 which: 97 length: 1
up keycode: 65 which: 65 length: 1
down keycode: 13 which: 13 length: 1     // first enter
press keycode: 13 which: 13 length: 1
up keycode: 13 which: 13 length: 1
down keycode: 13 which: 13 length: 1     // second enter
press keycode: 13 which: 13 length: 1
up keycode: 13 which: 13 length: 1

There is no way to distinguish the first and second enter, which is the problem.

The behavior doesn't have to revert to current Safari's behavior, but Gmail needs a way to distinguish enter in IME from regular enter. Any difference in the key events can be exploited, but there has to be some difference.

[Alexey Proskuryakov]

When verifying this, I saw that the behavior was different even before pressing Enter - in stock Safari, a suggestion appeared right after I typed k-a.

See also: bug 10010 (not all IMs use Enter as a special key to confirm the input area, some pass it forward).

[Michael Davidson]

Indeed, stock Safari isn't perfect, but at least it has information that can be used to fix the behavior. Current trunk WebKit can't be worked around (that I can see).

[Stephanie Lewis]

radar 4823129

[Alexey Proskuryakov]

I looked into this some more, and I start thinking that we should try to match Firefox behavior, with keyup/keydown being always sent, and keypress only being sent if IME didn't handle the event. Ideally, we should also send DOM3 textInput event (which would let you support Character Palette, Ink etc.)

What do you think about this?

[Michael Davidson]

That sounds good to me. 

[Alexey Proskuryakov]

See also: bug 10010.

[Alexey Proskuryakov]

To clarify on the model a bit more, and to stir further discussion:
1) keydown/keyup are the lowest level keyboard events which map directly to physical keypresses.
2) keypress is sent when an input method doesn't handle the event (i.e. when the IM has returned a "not handled" status, or a plain keyboard layout is being used). This is also when form submission etc takes place. Canceling the event cancels default processing.
3) I'm not really sure when textInput is supposed to be sent. I guess it's only meaningful for final text, i.e., typing into inline hole shouldn't dispatch this event, but confirming it should do so. Typing with a plain keyboard or entering text via Character Palette should probably dispatch it, as well. Pasting from clipboard probably shouldn't.

Looks like canceling textInput cannot prevent the text from being inserted - after all, DOM already has the inline hole content, correct? Canceling keypress prevents typing with a plain keyboard and default actions.

See also: <http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onkeypress.asp> (same for up and down). Seems to be quite different from what Firefox does.

[Darin Adler]

(In reply to comment #7)
> 2) keypress is sent when an input method doesn't handle the event (i.e. when
> the IM has returned a "not handled" status, or a plain keyboard layout is being
> used). This is also when form submission etc takes place. Canceling the event
> cancels default processing.

This is quite difficult to implement given the way we work with AppKit. In AppKit, a single interpretKeyEvents: method both sends events to input methods and dispatches commands. So if we want to interpret the key event before sending a keypress, so we can tell if the input method handles it or not, then we will also be dispatching commands like cut, motion with arrow keys, and the delete keys. That means there won't be a keypress for delete or arrow key presses, and I think there should be. This is going to be tricky to get right.

> 3) I'm not really sure when textInput is supposed to be sent. I guess it's only
> meaningful for final text, i.e., typing into inline hole shouldn't dispatch
> this event, but confirming it should do so. Typing with a plain keyboard or
> entering text via Character Palette should probably dispatch it, as well.
> Pasting from clipboard probably shouldn't.

That's right. My patch attached to bug 10010 implements the rule you mention here.

The DOM Level 3 specification says that pasting should dispatch it if the pasted text is plain text, but I think that's a bad idea. Whether text is styled or not should not affect which events are dispatched!

> Looks like canceling textInput cannot prevent the text from being inserted -
> after all, DOM already has the inline hole content, correct? Canceling keypress
> prevents typing with a plain keyboard and default actions.

In my opinion preventing default behavior on a textInput event should cause the content from the inline hole to be discarded.

[Alexey Proskuryakov]

(In reply to comment #8)
> That means there won't be a keypress for delete or arrow key
> presses, and I think there should be. This is going to be tricky to get right.

In Firefox, delete and arrow keypresses are also sent only if they are not handled by the IM. I guess this is a side effect of Carbon not treating those as commands?

But the behavior still sounds desirable to me.

> In my opinion preventing default behavior on a textInput event should cause the
> content from the inline hole to be discarded.

OK.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> > That means there won't be a keypress for delete or arrow key
> > presses, and I think there should be. This is going to be tricky to get right.
> 
> In Firefox, delete and arrow keypresses are also sent only if they are not
> handled by the IM. I guess this is a side effect of Carbon not treating those
> as commands?
> 
> But the behavior still sounds desirable to me.

Yes, I think it's probably required.

And yes, the AppKit issues are AppKit-specific and presumably do not arise in the Carbon equivalent.

[Adele Peterson]

Created attachment 13494 [details]
initial patch

This is my first cut at a fix for this bug and for bug #12677.

I tried to implement Alexey's suggestions, but I may be missing something about how this works.  So hopefully a few of you will try this out, and we can work out the kinks.

Basically, this patch will only dispatch a keypress event if the input method has not handled the event.

Here are the results from Michael's testcase, after typing "k-a-enter-enter" using Katakana:
(http://bantha.org/~mdavids/ime.html)

down keycode: 75 which: 75 length: 0
up keycode: 75 which: 75 length: 1
// 'k'

down keycode: 65 which: 65 length: 1
up keycode: 65 which: 65 length: 1
// 'a'

down keycode: 13 which: 13 length: 1
press keycode: 13 which: 13 length: 1
up keycode: 13 which: 13 length: 1
// 'enter' that confirms the marked text

down keycode: 13 which: 13 length: 1
press keycode: 13 which: 13 length: 1
// 'enter that submits the form

There are a few differences from our old behavior that I'd like to discuss.

1) Now we will fire a keydown *and* a keyup for marked text that is set by the input method.  I think this follows Alexey's proposal that keydown/keyup should correspond to the physical keyboard events.

2)  For the input methods that I tested with, Katakana and 2-set Korean, it appears that instead of calling "unmarkText" for that first enter key, they call "insertText" with the actual committed character.  This means that we must dispatch a keypress event.  The keyCode is still 13 since that comes from the physical key pressed, not the text that's inserted.  So this may be hard to detect from Gmail...

3) The last 'enter' that submits the form doesn't get a keyup.  This isn't a new effect from my patch, but I thought I should mention it here to clear up any confusion.  This happens because the keypress causes a blur to occur.  Since the input is no longer focused, the keyup event gets dispatched to the body instead.

[Darin Adler]

(In reply to comment #11)
> down keycode: 13 which: 13 length: 1
> press keycode: 13 which: 13 length: 1
> up keycode: 13 which: 13 length: 1
> // 'enter' that confirms the marked text

This seems clearly wrong. For the Katakana input method a return does not result in form submission and thus should not result in a keypress event.

That's different for the Korean input method.

[Adele Peterson]

Created attachment 13497 [details]
new patch

new patch.  Darin and I talked about how if input methods call insert text, then we need to detect that, and let the insert text happen immediately.

With this new change,  here's the output from the testcase:

down keycode: 75 which: 75 length: 0
up keycode: 75 which: 75 length: 1
// 'k'
down keycode: 65 which: 65 length: 1
up keycode: 65 which: 65 length: 1
// 'a'
down keycode: 13 which: 13 length: 1
up keycode: 13 which: 13 length: 1
// 'enter' that confirms the text
down keycode: 13 which: 13 length: 1
press keycode: 13 which: 13 length: 1
// 'enter' that submits the form

this seems like the right results.

[Adele Peterson]

ha! I just found a bug with this while editing in Bugzilla.  If you hold down the delete key, triggering an autorepeat event, instead of deleting, the browser will go back in history a few times.

Oops!

[Adele Peterson]

Created attachment 13499 [details]
patch that doesn't break autorepeat

ok- this patch fixes the autorepeat problem.

[Adele Peterson]

It would be nice to be able to test this with DumpRenderTree.  Not quite sure how that would work though.  The machine running the tests would have to have input methods installed, and we'd have to have a way to switch to an input method through javascript, using the layoutTestController.

[Alexey Proskuryakov]

Could this be tested with a custom NSInputManager (see bug 5305)?

[Darin Adler]

Comment on attachment 13499 [details]
patch that doesn't break autorepeat

I would like to see handleKeyPress divided into two separate calls rather than a single one with a boolean. In my mind, one of the calls is "allow input methods to handle key"; because of AppKit that one has a side effect of determining what the action is, but that's not the purpose of the call, rather an unfortunate side effect. Another call is "do the usual key press handling". I don't know what names to use but I think it's better to not use one call for both.

The second call could be the one to use the "saved action".

The name keypressSelectorString is Macintosh-specific. If it's really going to be Mac-specific, then maybe it should be in #if PLATFORM(MAC) and be a SEL. On the other hand, if it's going to be platform-independent then it should be called "command" rather than selector. I don't think the names "saved action" and "keypressSelectorString" and "keypressText" are named in a way that makes it clear they're connected; if the keypressSelectorString and keypressText were instead a struct named SavedAction or something along those lines it would be clearer. I think terminology could be important here for people understanding this, especially people on platforms that don't have the strange quirk Mac does where we have to do both things! We could treat this need to cache the command on the keyboard event as a Mac-specific thing -- that might make things easier for people on other platforms who don't need this although it would make KeyboardEvent.h uglier.

This breaks non-Mac platforms, right, because of the new parameter to the EditorClient function?

+        event->setKeypressSelectorString(String((char*)aSelector));

The above code is incorrect. You need to either use NSStringFromSelector or sel_getName. I think the above code might even fail on Leopard, although it will work on Tiger.

+        // The only way we know if its from an input method is to check hasMarkedText.  There might be a better way to do this.

I think "there might be a better way" is confusing, even though it's true. I think the comment should say, "We assume it's from the input method if we have marked text." and an on another line "FIXME: In theory that could be wrong for some input methods, so we should look for another way to determine if the call is from the input method."

The substance of the code looks great.

I'd like to see some new layout tests -- is there no way to activate an input method from a layout test? We should consider how to make this part of regression tests.

review- specifically because of the "cast selector to char*" issue. If you resolve that it's OK to land without further review, but please consider my other comments too.

[Adele Peterson]

Comment on attachment 13499 [details]
patch that doesn't break autorepeat

Darin reviewed a newer version of this.

[Adele Peterson]

Committed revision 20030.