Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(234)

Issue 149755: This change list improves IME support on Linux. Many corner cases that were n...

Created:
11 years, 5 months ago by james.su
Modified:
6 years, 6 months ago
Reviewers:
Hironori Bono
CC:
chromium-reviews_googlegroups.com, darin (slow to review), brettw
Visibility:
Public.

Description

According to Evan Stade's suggestion, input method related code has been moved into a new class RenderWidgetHostViewGtkIMContext. This change list improves IME support on Linux. Many corner cases that were not handled in original code are addressed, for example the input method in password box case. The most important change in this CL is the change to key event processing flow. In old code, a key event will first be sent to webkit then dispatched to IME for filtering. With this CL, a key event will first be dispatched to IME for filtering, then how to send the event to webkit is decided by the filtering result. This CL tries to emulate the keyboard input behavior on Windows as much as possible. For example, if a keydown event is filtered by IME, then its virtual key code will be changed to VK_PROCESSKEY(0xE5) to prevent webkit from processing it again. This behavior can workaround bug 16281. To test this CL, you may cut and save following html code into a file and open it in chrome. ------------------8<----cut here----->8--------------------- <html><head> <meta http-equiv="content-type" content="text/html; charset=utf-8"> <script> function keyEventHandler(event) { var props = [ "type", "charCode", "keyCode", "altKey", "ctrlKey", "shiftKey", "metaKey" ]; var info = document.getElementById('info'); var s = ''; for (var i in props) { s += props[i] + ':' + event[props[i]] + ' '; } info.value += s + '\n'; } function textEventHandler(event) { info.value += "type:" + event['type'] + " data:" + event['data'] + '\n'; } function passwordChangeEventHandler(event) { var input2 = document.getElementById('input2'); info.value += "password:" + input2.value + '\n'; } function onLoad() { var input = document.getElementById('input'); input.addEventListener('keydown', keyEventHandler, false); input.addEventListener('keyup', keyEventHandler, false); input.addEventListener('keypress', keyEventHandler, false); input.addEventListener('textInput', textEventHandler, false); var input2 = document.getElementById('input2'); input2.addEventListener('change', passwordChangeEventHandler, false); } </script> </head><body onload="onLoad()"> <input id="input" size="20"> <input id="input2" type="password" size="20"> <p> <textarea id="info" rows="40" cols="150"></textarea> </p></body></html> ------------------8<----cut here----->8--------------------- This CL was confirmed to fix following issues: BUG=16281 "arrow keys and backspace/delete keys move/delete two characters at a time when xim immodule is used" BUG=16282 "Disable IMEs in a password input" BUG=16596 "fcitx (chinese input method) not working in ubuntu 9.04" BUG=16659 "Crash near RenderWidgetHostViewGtk::IMEUpdateStatus" BUG=16699 "Can't move cursor to omnibox and use input method if cursor is currently in a text input box in web page." BUG=16796 "Input method issue: When inputting text in a text box, if there is a composition string then pressing Backspace or Delete will cause the composition string be cleared and the text box refuses to accept any further input. All tests assume above html code is used. TEST=Start scim with "scim -d" and start chrome with GTK_IM_MODULE=xim and XMODIFIERS=@im=SCIM. Type something in input box, eg. "hello", then press backspace, to see if only one character is deleted. TEST=Move cursor to password input box, press ctrl-space to see if input method is not activated. Switch keyboard layout to Canadian-French then type a'[{' key and an 'a' key, then press enter, to see if a Latin character "U+00E2" is inputted in password box. TEST=Install fcitx with "sudo apt-get install fcitx" (assume you are using Ubuntu/Debian). Open a terminal, export XMODIFIERS=@im=fcitx and GTK_IM_MODULE=xim then run fcitx, then start chrome. Move cursor into an input box, press ctrl-space and input something, eg. "nihao" then press space. Check if some Chinese characters are inputted. TEST=Start chrome with GTK_IM_MODULE=scim. Move cursor into a text input box then move into a password box, chrome should not crash. TEST=Move cursor into a text input box, then click omnibox, and see if the text input box has lost focus. Press ctrl-space to activate input method, then type something, and see of the input goes into omnibox. TEST=Move cursor into a text input box and enable a Chinese Pinyin input method, then type something, eg. "dajiahao", make sure a composition string is displayed in the text input box, then press backspace and see if there is still a composition string and backspace is handled by input method.

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 6

Patch Set 3 : '' #

Total comments: 14

Patch Set 4 : '' #

Patch Set 5 : '' #

Total comments: 6

Patch Set 6 : '' #

Total comments: 13

Patch Set 7 : '' #

Total comments: 2

Patch Set 8 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+615 lines, -179 lines) Patch
M chrome/browser/renderer_host/render_widget_host_view_gtk.h View 1 2 3 4 5 6 7 3 chunks +8 lines, -25 lines 0 comments Download
M chrome/browser/renderer_host/render_widget_host_view_gtk.cc View 1 2 3 4 5 6 7 9 chunks +585 lines, -154 lines 0 comments Download
M webkit/api/src/gtk/WebInputEventFactory.cpp View 1 2 3 4 5 6 7 3 chunks +22 lines, -0 lines 0 comments Download

Messages

Total messages: 27 (0 generated)
james.su_gmail.com
Hi, My CL is finally ready for review. Please help review and test it. Thanks ...
11 years, 5 months ago (2009-07-16 16:04:19 UTC) #1
Evan Stade
Overall this looks swell, thanks a lot (although hbono knows much more than me about ...
11 years, 5 months ago (2009-07-17 00:24:27 UTC) #2
james.su_gmail.com
Thanks for your feedback. Regards James Su On Fri, Jul 17, 2009 at 8:24 AM, ...
11 years, 5 months ago (2009-07-17 00:34:50 UTC) #3
james.su_gmail.com
On 2009/07/17 00:24:27, Evan Stade wrote: > Overall this looks swell, thanks a lot (although ...
11 years, 5 months ago (2009-07-17 00:35:31 UTC) #4
Evan Stade
I was thinking of moving the data members as well (a la WebDragDest in TabContentsViewGtk) ...
11 years, 5 months ago (2009-07-17 01:03:48 UTC) #5
james.su_gmail.com
Ok, I'll make this change asap. Is it ok to put the new class in ...
11 years, 5 months ago (2009-07-17 01:09:51 UTC) #6
Evan Stade
That depends on how long it is. I would say it is probably ok.
11 years, 5 months ago (2009-07-17 01:16:43 UTC) #7
Hironori Bono
Thank you for your change. Because this change doesn't have any useful "TEST=" lines, I'm ...
11 years, 5 months ago (2009-07-17 02:29:50 UTC) #8
james.su_gmail.com
Thanks your feedback. On Fri, Jul 17, 2009 at 10:29 AM, <hbono@chromium.org> wrote: > Thank ...
11 years, 5 months ago (2009-07-17 03:38:44 UTC) #9
Hironori Bono
By the way, is it possible to use Rietveld to add your reply so that ...
11 years, 5 months ago (2009-07-17 05:32:50 UTC) #10
james.su_gmail.com
http://codereview.chromium.org/149755/diff/1002/11 File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right): http://codereview.chromium.org/149755/diff/1002/11#newcode189 Line 189: RenderWidgetHost *host = host_view->GetRenderWidgetHost(); On 2009/07/17 02:29:50, hbono ...
11 years, 5 months ago (2009-07-17 08:44:14 UTC) #11
james.su_gmail.com
Changes have been made according to Evan's suggestion. Test cases have been added. Please help ...
11 years, 5 months ago (2009-07-17 09:33:07 UTC) #12
james.su_gmail.com
A bug was fixed, which may cause crash when pressing Esc on popup view. Regards ...
11 years, 5 months ago (2009-07-17 09:52:47 UTC) #13
Hironori Bono
Thank you for your update. Even though I think the overall design is good, I ...
11 years, 5 months ago (2009-07-17 11:01:53 UTC) #14
james.su_gmail.com
Thanks for you feedback. I'll update my CL asap. Regards James Su http://codereview.chromium.org/149755/diff/2001/1013 File chrome/browser/renderer_host/render_widget_host_view_gtk.cc ...
11 years, 5 months ago (2009-07-17 13:01:59 UTC) #15
Evan Stade
I think this may also fix bugs 15959 and 15964
11 years, 5 months ago (2009-07-18 01:06:20 UTC) #16
james.su_gmail.com
Hi, I just did some refactory according to your suggestion. But for dead key issue, ...
11 years, 5 months ago (2009-07-18 13:01:51 UTC) #17
Evan Stade
Thanks for the refactor, with the following little changes the style LGTM (still need LGTM ...
11 years, 5 months ago (2009-07-20 18:29:17 UTC) #18
james.su_gmail.com
Thanks for your feedback. I'll update my CL accordingly. Though I'm not sure if bug ...
11 years, 5 months ago (2009-07-21 01:01:30 UTC) #19
james.su_gmail.com
Thanks for your feedback. I'll update my CL accordingly. Though I'm not sure if bug ...
11 years, 5 months ago (2009-07-21 01:01:43 UTC) #20
james.su_gmail.com
Hi, Just updated this CL according to Evan's suggestion. And I can't reproduce bug 15959 ...
11 years, 5 months ago (2009-07-21 01:24:25 UTC) #21
Hironori Bono
Thank you for your update and sorry for my slow responses. I would like to ...
11 years, 5 months ago (2009-07-21 03:07:33 UTC) #22
james.su_gmail.com
Thanks for your feedback, please see below my comments. I'll update my CL accordingly. Regards ...
11 years, 5 months ago (2009-07-21 03:37:42 UTC) #23
james.su_gmail.com
CL has been updated, please help review. James Su
11 years, 5 months ago (2009-07-21 04:15:46 UTC) #24
Hironori Bono
LGTM. This change works fine as far as I tested. Thank you again for your ...
11 years, 5 months ago (2009-07-21 07:47:04 UTC) #25
james.su_gmail.com
Hi, Thanks for your review. CL has been updated according to your latest comment. Please ...
11 years, 5 months ago (2009-07-21 08:05:58 UTC) #26
Evan Stade
11 years, 5 months ago (2009-07-21 19:52:14 UTC) #27
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21203

thanks for the patch! let the bug closing commence :)

Powered by Google App Engine
This is Rietveld 408576698