[Webkit-unassigned] [Bug 25119] IME modifies the DOM before keydown

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 18 01:32:26 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=25119


ap at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #30435|1                           |0
        is obsolete|                            |




------- Comment #20 from ap at webkit.org  2009-05-18 01:32 PDT -------
(From update of attachment 30435)
> +        Fire the keydown event before inserting text into the DOM on IME input
<...>
> +        On Mac, we need to send the key event to the IME before firing keydown to
> +        know that it's an IME input (so we can set the keycode to 229). In order
> +        to fire the keydown after the DOM is modified we:

This probably should say "before", not "after".

> +        There's no automated way to test IME input, so I've added a manual test.

We have some tests for input methods in editing/input and
platform/mac/editing/input, I think that an automated test can and should be
added for this change.

>  #if PLATFORM(MAC)
>          // We only have this need to store keypress command info on the Mac.
>          Vector<KeypressCommand>& keypressCommands() { return m_keypressCommands; }
> +
> +        // Currently, only Mac Safari uses this. Eventually GTK, and Chromium Linux/Win
> +        // probably want to use this as well though. To ensure keydown fires before the
> +        // DOM is modified on IME key events.

There is no need to say that only Mac uses the code under a PLATFORM(MAC)
guard. Also, as this becomes a part of normal text processing, it's not
accurate to say that this code only ensures keydown firing at correct time -
that's its original inspiration that can be discovered via svn history, but not
the full set of its effects.

>  #if PLATFORM(MAC)        
>          Vector<KeypressCommand> m_keypressCommands;
> +        // Currently, only Mac Safari uses this. Eventually GTK, and Chromium Linux/Win
> +        // probably want to use this as well though. To ensure keydown fires before the
> +        // DOM is modified on IME key events.
> +        OwnPtr<IMECommand> m_imeCommand;
>  #endif

Ditto.

But here is also my major comment about this patch. We are already storing
input method commands in the same object, and in a much more versatile way
(Vector<KeypressCommand> m_keypressCommands). Why do do need to store another
copy? The new IMECommand structure doesn't even adequately describe what
commands can be sent back to us as a result of text processing.

Since Cocoa text bindings work in tandem with input methods, you may want to
check out a nice description of those at
<http://www.hcs.harvard.edu/~jrus/Site/cocoa-text.html>, especially an example
binding they give for ^H.

Another consideration is that this approach breaks expectations for input
methods that use two-way communication with the editing view. For example,
after an IM sends setMarkedText:selectedRange:, it has every right to expect
that hasMarkedText will return true.

> +void Editor::applyIMECommand(IMECommand* command) {

Brace goes to next line (and the comment about not using "IME" of course still
applies).

> +struct IMECommand {
> +    IMECommand(const String& confirmedText)
> +    : text(confirmedText)
> +    , isConfirmationCommand(true) {}

Initializers should be indented 4 spaces more to the right, and we usually put
braces each on its own line (although the latter doesn't seem to be mentioned
in coding style guidelines).


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list