[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