Fix omnibox suggestion: restore selection on WM_IME_ENDCOMPOSITION

Fix 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

[falken, 2012-10-10 09:51:33]

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"

[Seigo Nonaka, 2012-10-10 10:18:22]

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.

[Yohei Yukawa, 2012-10-11 01:02:10]

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);

[Seigo Nonaka, 2012-10-11 02:11:33]

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.

[Peter Kasting, 2012-10-11 05:17:54]

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?

[Peter Kasting, 2012-10-11 05:26:19]

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?

[Seigo Nonaka, 2012-10-11 06:38:13]

>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?

[Yohei Yukawa, 2012-10-11 15:10:30]

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.

[Peter Kasting, 2012-10-11 18:46:20]

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.

[Yohei Yukawa, 2012-10-11 22:16:07]

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.

[Seigo Nonaka, 2012-10-12 03:22:31]

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?

[Peter Kasting, 2012-10-12 03:29:46]

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.

[Seigo Nonaka, 2012-10-12 03:58:07]

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.

[commit-bot: I haz the power, 2012-10-12 03:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/11093044/14004

[commit-bot: I haz the power, 2012-10-12 06:29:43]

Change committed as 161515