|
|
Created:
8 years, 2 months ago by Seigo Nonaka Modified:
8 years, 2 months ago CC:
chromium-reviews, tfarina, James Su Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix omnibox suggestion: restore selection on WM_IME_ENDCOMPOSITION
BUG=154379
TEST=Manually done one Win XP, Win 7, Win 8 including jp, kr, cn
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161515
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Address comments #Patch Set 4 : Restore selection #
Total comments: 2
Patch Set 5 : Address comments #
Messages
Total messages: 16 (0 generated)
lgtm http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:755: bool prevent_inline_auto_completion = nit: I think this can just be prevent_inline_autocompletion. Also in the comments, autocompletion can be one word. http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:759: // on-screen keyboards on Windows 8 insert a character and then CUAS I think the CUAS acronym isn't widely known. Consider writing "CUAS (Cicero Unaware Application Support)". http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:760: // translates it into IMM32 protocol like WM_IME_COMPOSITION. As a result, "into an IMM32 protocol message" http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right): http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.h:515: // workaround for preventing inline auto completion for Chinese and Japanese nits: "used in a workaround" and "autocompletion"
falken: Thank you for your review! http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:755: bool prevent_inline_auto_completion = On 2012/10/10 09:51:33, falken wrote: > nit: I think this can just be prevent_inline_autocompletion. Also in the > comments, autocompletion can be one word. Done. http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:759: // on-screen keyboards on Windows 8 insert a character and then CUAS On 2012/10/10 09:51:33, falken wrote: > I think the CUAS acronym isn't widely known. Consider writing "CUAS (Cicero > Unaware Application Support)". Done. http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:760: // translates it into IMM32 protocol like WM_IME_COMPOSITION. As a result, On 2012/10/10 09:51:33, falken wrote: > "into an IMM32 protocol message" Done. http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right): http://codereview.chromium.org/11093044/diff/2001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.h:515: // workaround for preventing inline auto completion for Chinese and Japanese On 2012/10/10 09:51:33, falken wrote: > nits: "used in a workaround" and "autocompletion" Done.
lgtm with a nit. http://codereview.chromium.org/11093044/diff/7001/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11093044/diff/7001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1426: in_on_ime_composition_ = true; AutoReset<bool> auto_reset_in_on_ime_composition(&in_on_ime_composition_, true);
Yukawa: Thank you for your review. Adding Peter Kasting as a Omnibox experts. Peter: Could you take a look? Thank you. http://codereview.chromium.org/11093044/diff/7001/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11093044/diff/7001/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1426: in_on_ime_composition_ = true; On 2012/10/11 01:02:10, Yohei Yukawa wrote: > AutoReset<bool> auto_reset_in_on_ime_composition(&in_on_ime_composition_, true); Done.
Can we just fix the bug? I can help fix if you just figure out what causes the cursor/selection change after we come back with inline autocompletion. This ought to be pretty easy to find, just set breakpoints. If CUAS is the problem, can we set some compatibility bits in our manifest to flip its state?
Wait, are the comments saying that it's not that we get a message that clears the selection, but that the SetSel() call actually fails? If so, can we detect the failure? What's the window of time in which things fail, can we update the selection after that? Is this documented anywhere?
>Can we just fix the bug? Actually, we don't have complete fix now, but we are now working for making better workaround. >If CUAS is the problem, can we set some compatibility bits in our manifest to flip its state? Unfortunately we can't, CUAS can not be disabled on Vista and later. > Wait, are the comments saying that it's not that we get a message that clears the selection, but that the SetSel() call actually fails? > If so, can we detect the failure? Yes for the comment, and no we can't for the detection. SetSel returns expected value but it somehow fails update the actual state. We tested it by calling GetSel just after calling SetSel, and the result of GetSel is what we passed to SetSel. So we haven't succeeded to detect the failure of SetSel pragmatically yet. > What's the window of time in which things fail, can we update the selection after that? > Is this documented anywhere? Not documented as far as I searched. Anyway we will try to find any better workarounds for some days with breakpoints. Thank you for your comments! On 2012/10/11 05:26:19, Peter Kasting wrote: > Wait, are the comments saying that it's not that we get a message that clears > the selection, but that the SetSel() call actually fails? If so, can we detect > the failure? What's the window of time in which things fail, can we update the > selection after that? Is this documented anywhere?
On 2012/10/11 06:38:13, Seigo Nonaka wrote: > > Wait, are the comments saying that it's not that we get a message that clears > the selection, but that the SetSel() call actually fails? > > If so, can we detect the failure? > Yes for the comment, and no we can't for the detection. > SetSel returns expected value but it somehow fails update the actual state. > We tested it by calling GetSel just after calling SetSel, and the result of > GetSel is what we passed to SetSel. > So we haven't succeeded to detect the failure of SetSel pragmatically yet. Update: It turned out that WM_IME_ENDCOMPOSITION, which OmniboxViewWin hasn't handled so far, clears the selection. In other words, just after WM_IME_COMPOSITION is handled, the selection is fine but subsequent WM_IME_ENDCOMPOSITION breaks the selection state. We also found that this behavior of WM_IME_ENDCOMPOSITION is specific to RichEdit 4.1. The current stable (M22) does not have this issue because it is still using RichEdit 2.0. As far as we've tested, following code should work at least for Japanese. Currently we are testing Chinese/Korean languages. LRESULT OmniboxViewWin::OnImeEndComposition(UINT message, WPARAM wparam, LPARAM lparam) { // Unlike RichEdit 2.0, RichEdit 4.1 clears the selection when it receives // WM_IME_ENDCOMPOSITION. Here we need to restore the selection manually. LONG start = 0; LONG end = 0; GetSel(start, end); LRESULT result = DefWindowProc(message, wparam, lparam); SetSel(start, end); return result; } Peter, do you have any concern about adding above new WM_IME_ENDCOMPOSITION handler into OmniboxViewWin? If it is OK, we will update this change set after we finish remaining manual testing. Thanks again for your suggestion. It actually helped us to reveal the root cause.
The general idea of handling WM_IME_ENDCOMPOSITION like this sounds good to me. I wouldn't comment about RichEdit 2.0, just talk about 4.1. Also, use a CHARRANGE instead of two LONGs. My one worry is, are there any scenarios where clearing the selection on "end composition" is actually correct? Like maybe somehow the IME itself selected some text temporarily and now needs to reset that? This is vague because I don't understand the problem space well enough to know if this is even a possibility.
On 2012/10/11 18:46:20, Peter Kasting wrote: > The general idea of handling WM_IME_ENDCOMPOSITION like this sounds good to me. > > I wouldn't comment about RichEdit 2.0, just talk about 4.1. Also, use a > CHARRANGE instead of two LONGs. Got it. > My one worry is, are there any scenarios where clearing the selection on "end > composition" is actually correct? Like maybe somehow the IME itself selected > some text temporarily and now needs to reset that? This is vague because I > don't understand the problem space well enough to know if this is even a > possibility. Good point. I just checked again this scenario with above patch against MSIME-Ja and it worked well. Even when the IME specifies a highlighted text range, it is cleared in WM_IME_COMPOSITION handler and our selection manipulation in WM_IME_ENDCOMPOSITION worked as expected. From my knowledge of IME development (I'm also working on developing Google Japanese IME), IME cannot select any characters during composition. It can only specify the display attribute. I also confirmed this by monitoring the selection range returned by GetSel. The returned value is always the 0-length cursor position even when IME specifies a highlighted text range. So my gut feeling is that this code is safe. Anyway we will check major IMEs manually.
Update patch set. I confirmed that this patch works fine on Win XP SP3, Win 7 SP1, Win 8 desktop for MSIME including Japanese, Chinese and Korean. Peter: Could you please take a look again?
LGTM Once this ships, try to seek out feedback from people using weird non-Microsoft IMEs and the like if possible. http://codereview.chromium.org/11093044/diff/19001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11093044/diff/19001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1424: // Unlike RichEdit 2.0, RichEdit 4.1 clears the selection when it receives Nit: Slightly better: // The edit control auto-clears the selection on WM_IME_ENDCOMPOSITION, which // means any inline autocompletion we were showing will no longer be selected, // and therefore no longer replaced by further user typing. To avoid this we // manually restore the original selection after the edit handles the message.
Thank you for your kindly comment! http://codereview.chromium.org/11093044/diff/19001/chrome/browser/ui/views/om... File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right): http://codereview.chromium.org/11093044/diff/19001/chrome/browser/ui/views/om... chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1424: // Unlike RichEdit 2.0, RichEdit 4.1 clears the selection when it receives Updated, thanks. On 2012/10/12 03:29:46, Peter Kasting wrote: > Nit: Slightly better: > > // The edit control auto-clears the selection on WM_IME_ENDCOMPOSITION, which > // means any inline autocompletion we were showing will no longer be selected, > // and therefore no longer replaced by further user typing. To avoid this we > // manually restore the original selection after the edit handles the message.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nona@chromium.org/11093044/14004
Change committed as 161515 |