Summary: | REGRESSION: IME key events different in nightly | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Davidson <mdavids> | ||||||||
Component: | Text | Assignee: | Adele Peterson <adele> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | adele, ap, darin, kevino | ||||||||
Priority: | P1 | Keywords: | GoogleBug, InRadar, Regression | ||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
URL: | http://gmail.com | ||||||||||
Attachments: |
|
Description
Michael Davidson
2006-09-15 09:22:21 PDT
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). 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). radar 4823129 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? That sounds good to me. 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. (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. (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. (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. 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. (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. 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.
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! Created attachment 13499 [details]
patch that doesn't break autorepeat
ok- this patch fixes the autorepeat problem.
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. Could this be tested with a custom NSInputManager (see bug 5305)? 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.
Comment on attachment 13499 [details]
patch that doesn't break autorepeat
Darin reviewed a newer version of this.
Committed revision 20030. |