Bug 39475 - [chromium] if keydown is prevented, don't update the IME and clear the IME state
Summary: [chromium] if keydown is prevented, don't update the IME and clear the IME state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-21 00:18 PDT by Tony Chang
Modified: 2010-05-23 18:14 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.05 KB, patch)
2010-05-21 00:18 PDT, Tony Chang
ojan: review+
ojan: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-05-21 00:18:12 PDT
[chromium] if keydown is prevented, don't update the IME and clear the IME state
Comment 1 Tony Chang 2010-05-21 00:18:40 PDT
Created attachment 56678 [details]
Patch
Comment 2 Tony Chang 2010-05-21 00:19:44 PDT
This is similar to the original patch, but in addition to ignoring the event, we clear out any existing IME state.
Comment 3 Eric Seidel (no email) 2010-05-21 12:10:24 PDT
How does this behavior compare to other WebKit ports?  Most noteably apple Mac and Apple Win?
Comment 4 Ojan Vafai 2010-05-21 17:52:25 PDT
Comment on attachment 56678 [details]
Patch

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.
Comment 5 Tony Chang 2010-05-23 18:14:52 PDT
Committed r60054: <http://trac.webkit.org/changeset/60054>