[webkit-reviews] review granted: [Bug 39475] [chromium] if keydown is prevented, don't update the IME and clear the IME state : [Attachment 56678] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Tony Chang (Google)
<tony at chromium.org>'s request for review:
Bug 39475: [chromium] if keydown is prevented, don't update the IME and clear
the IME state
https://bugs.webkit.org/show_bug.cgi?id=39475

Attachment 56678: Patch
https://bugs.webkit.org/attachment.cgi?id=56678&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
r=me with the added FIXME. Given the total inconsistency of IME
preventDefault-handling on Mac browsers and that all Windows browsers seem to
properly handle preventDefault in that case, I think it's safe to experiment in
the direction of canceling. Also, this is an improvement over the bugs we found
from r59735, so I'm OK with committing it and trying it out on the dev channel.


WebKit/chromium/src/WebViewImpl.cpp:1193
 +	if ((command == WebCompositionCommandDiscard) ||
m_suppressNextKeypressEvent) {
Please add a FIXME that we should really try to cancel individual keydowns
instead of discarding the entire composition.

(In reply to comment #2)
> This is similar to the original patch, but in addition to ignoring the event,
we clear out any existing IME state.

For context, the original patch is http://trac.webkit.org/changeset/59735. This
is a better solution than r59735, but it's still not the ideal behavior.
Ideally, preventDefault on a keydown would only cancel that key press, not the
entire composition. For now, I'm OK with trying this and seeing if it meets
Wave's use case.

(In reply to comment #3)
> How does this behavior compare to other WebKit ports?  Most noteably apple
Mac and Apple Win?

preventDefault of keyDown is a no-op on Apple Mac. As best I can tell, Apple
Win actually properly cancels preventDefaulted keydowns. Although, I only
tested a few IMEs. There may be Windows IMEs that don't properly
preventDefault.

It's not clear to me whether the original behavior (preventDefault is a no-op)
or the new behavior (discard the composition) is better. Neither one meets the
use-case of excluding certain types of characters (e.g. non-numbers), although
I'm not really sure there's much use-case for that when you are
mid-composition. We should make sure to get actively solicit feedback from the
relevant developers (e.g. Wave) and be ready to roll the whole thing back
before the next beta/stable release. I'm not sure if there's a use-case for
just summarily disallowing any text entry in an editable area. If there is,
then the new behavior at least affords that.