Accept keyword by pressing space.

Accept keyword by pressing space.

This CL disables implicit keyword accepting completely. The user can only
accept a keyword by pressing Tab or space explicitly while the keyword hint
UI is visible.

BUG=60972
TEST=interactive_ui_tests --gtest_filter=AutocompleteEditViewTest.AcceptKeywordBySpace

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72003

[Peter Kasting, 2011-01-14 00:43:31]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
I think that rather than trying to detect this case after a change, you should
just detect it when the space key is pressed, similar to how we detect
up/down/tab/control/etc. when they're pressed and take action immediately.

I think this may allow you to get rid of the concept of PASTING/PASTED entirely.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
This addition seems a little odd to me.  The AutocompleteEdit already knows when
there's a hint visible: |is_keyword_hint_| is true.  Right?  Why does it need to
ask the controller?

[James Su, 2011-01-14 01:03:53]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 00:43:31, Peter Kasting wrote:
> I think that rather than trying to detect this case after a change, you should
> just detect it when the space key is pressed, similar to how we detect
> up/down/tab/control/etc. when they're pressed and take action immediately.
> 
> I think this may allow you to get rid of the concept of PASTING/PASTED
entirely.

The difference between Tab and Space is that Tab doesn't insert anything into
the buffer but space does. And things become very complicated when involving an
IME. For example an IME can change the behavior of space to anything else, it
may insert an ideographic space or even nothing. Or if there is a candidate
list, space may be used for choosing the first candidate.
And detecting space key event requires to change all AutocompleteEditView
implementations, which would also much more complicated than adding
PASTING/PASTED.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
On 2011/01/14 00:43:31, Peter Kasting wrote:
> This addition seems a little odd to me.  The AutocompleteEdit already knows
when
> there's a hint visible: |is_keyword_hint_| is true.  Right?  Why does it need
to
> ask the controller?

They keyword hint UI may not be visible if the browser window is too small,
accepting keyword in such case may surprise the user.

[Peter Kasting, 2011-01-14 01:20:01]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 01:03:54, James Su wrote:
> On 2011/01/14 00:43:31, Peter Kasting wrote:
> > I think that rather than trying to detect this case after a change, you
should
> > just detect it when the space key is pressed, similar to how we detect
> > up/down/tab/control/etc. when they're pressed and take action immediately.
> 
> The difference between Tab and Space is that Tab doesn't insert anything into
> the buffer but space does.

This is part of why I suggested changing the underlying representation to be a
(keyword, input string) pair.  But even without making that change it's easy
enough to modify the |user_text_|.  In other words, when space triggers keyword
mode, we should never send it to the edit at all, just modify our local
variables appropriately and rerun the query, which will completely change the
contents of the edit.

> And things become very complicated when involving an
> IME. For example an IME can change the behavior of space to anything else, it
> may insert an ideographic space or even nothing. Or if there is a candidate
> list, space may be used for choosing the first candidate.

We don't want space to toggle into keyword mode any time IME composition is
active.  Doesn't that handle all the cases?

> And detecting space key event requires to change all AutocompleteEditView
> implementations, which would also much more complicated than adding
> PASTING/PASTED.

I wasn't trying to optimize the total size of this patch.  I was merely
commenting that once you make this change, PASTING/PASTED become unnecessary.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
On 2011/01/14 01:03:54, James Su wrote:
> On 2011/01/14 00:43:31, Peter Kasting wrote:
> > This addition seems a little odd to me.  The AutocompleteEdit already knows
> when
> > there's a hint visible: |is_keyword_hint_| is true.  Right?  Why does it
need
> to
> > ask the controller?
> 
> They keyword hint UI may not be visible if the browser window is too small,
> accepting keyword in such case may surprise the user.

The hint is just a hint.  It is important that space accept the keyword even
when the hint is not visible, just as tab can accept keywords regardless of
whether the hint is visible.

[James Su, 2011-01-14 02:02:46]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 01:20:01, Peter Kasting wrote:
> On 2011/01/14 01:03:54, James Su wrote:
> > On 2011/01/14 00:43:31, Peter Kasting wrote:
> > > I think that rather than trying to detect this case after a change, you
> should
> > > just detect it when the space key is pressed, similar to how we detect
> > > up/down/tab/control/etc. when they're pressed and take action immediately.
> > 
> > The difference between Tab and Space is that Tab doesn't insert anything
into
> > the buffer but space does.
> 
> This is part of why I suggested changing the underlying representation to be a
> (keyword, input string) pair.  But even without making that change it's easy
> enough to modify the |user_text_|.  In other words, when space triggers
keyword
> mode, we should never send it to the edit at all, just modify our local
> variables appropriately and rerun the query, which will completely change the
> contents of the edit.
The problem is, the IME will handle the space key first, then it may insert
arbitrary content into the edit box, and even if the IME inserts anything into
the edit box, it may still return an unhandled space key to us.

> 
> > And things become very complicated when involving an
> > IME. For example an IME can change the behavior of space to anything else,
it
> > may insert an ideographic space or even nothing. Or if there is a candidate
> > list, space may be used for choosing the first candidate.
> 
> We don't want space to toggle into keyword mode any time IME composition is
> active.  Doesn't that handle all the cases?
An IME can handle key events without composition text. Actually most popular
Chinese IMEs on Windows don't use composition text at all.

> 
> > And detecting space key event requires to change all AutocompleteEditView
> > implementations, which would also much more complicated than adding
> > PASTING/PASTED.
> 
> I wasn't trying to optimize the total size of this patch.  I was merely
> commenting that once you make this change, PASTING/PASTED become unnecessary.

I'm just worry about the complexity regarding to IME support and platform
difference. Three platforms handle keyboard and IME event in totally different
ways, it's very hard to get things right on all three platforms.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
On 2011/01/14 01:20:01, Peter Kasting wrote:
> On 2011/01/14 01:03:54, James Su wrote:
> > On 2011/01/14 00:43:31, Peter Kasting wrote:
> > > This addition seems a little odd to me.  The AutocompleteEdit already
knows
> > when
> > > there's a hint visible: |is_keyword_hint_| is true.  Right?  Why does it
> need
> > to
> > > ask the controller?
> > 
> > They keyword hint UI may not be visible if the browser window is too small,
> > accepting keyword in such case may surprise the user.
> 
> The hint is just a hint.  It is important that space accept the keyword even
> when the hint is not visible, just as tab can accept keywords regardless of
> whether the hint is visible.
Existing code doesn't accept keyword by Tab key if the hint is not visible. I
can change this behavior, if you think it's not correct.

[Peter Kasting, 2011-01-14 02:30:36]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 02:02:46, James Su wrote:
> The problem is, the IME will handle the space key first, then it may insert
> arbitrary content into the edit box, and even if the IME inserts anything into
> the edit box, it may still return an unhandled space key to us.

It seems like I don't understand how IMEs work well enough.  How is it that we
can get away with all the other keychecks we do when receiving WM_KEYDOWN?  I am
surprised that we'd have no opportunity to intercept a keydown event before the
IME processes it.  There must be some way.

> > > And detecting space key event requires to change all AutocompleteEditView
> > > implementations, which would also much more complicated than adding
> > > PASTING/PASTED.
> > 
> > I wasn't trying to optimize the total size of this patch.  I was merely
> > commenting that once you make this change, PASTING/PASTED become
unnecessary.
> 
> I'm just worry about the complexity regarding to IME support and platform
> difference. Three platforms handle keyboard and IME event in totally different
> ways, it's very hard to get things right on all three platforms.

That's true, but we're already in that situation.

There are practical problems with trying to detect space being pressed
after-the-fact, but I'm more concerned about the conceptual issue.  Conceptually
we really want to treat the space keypress like an accelerator, not as a
character, like we do when we try and detect that the edit has gained a space
character and react to it.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
On 2011/01/14 02:02:46, James Su wrote:
> Existing code doesn't accept keyword by Tab key if the hint is not visible. I
> can change this behavior, if you think it's not correct.

Yeah, that seems like a bug.

[James Su, 2011-01-14 03:26:22]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 02:30:36, Peter Kasting wrote:
> On 2011/01/14 02:02:46, James Su wrote:
> > The problem is, the IME will handle the space key first, then it may insert
> > arbitrary content into the edit box, and even if the IME inserts anything
into
> > the edit box, it may still return an unhandled space key to us.
> 
> It seems like I don't understand how IMEs work well enough.  How is it that we
> can get away with all the other keychecks we do when receiving WM_KEYDOWN?  I
am
> surprised that we'd have no opportunity to intercept a keydown event before
the
> IME processes it.  There must be some way.
There is no way on Windows to intercept a key event before the IME. The
situation is even worse on Linux, that the most popular input method intercepts
all events before the whole application, so when the input method is in action,
the application can not receive any events.

> 
> > > > And detecting space key event requires to change all
AutocompleteEditView
> > > > implementations, which would also much more complicated than adding
> > > > PASTING/PASTED.
> > > 
> > > I wasn't trying to optimize the total size of this patch.  I was merely
> > > commenting that once you make this change, PASTING/PASTED become
> unnecessary.
> > 
> > I'm just worry about the complexity regarding to IME support and platform
> > difference. Three platforms handle keyboard and IME event in totally
different
> > ways, it's very hard to get things right on all three platforms.
> 
> That's true, but we're already in that situation.
The situation is much better for keys that don't generate any content (like Tab,
arrow keys, etc.). Because we can simply do nothing if the input method consumes
them.

> 
> There are practical problems with trying to detect space being pressed
> after-the-fact, but I'm more concerned about the conceptual issue. 
Conceptually
> we really want to treat the space keypress like an accelerator, not as a
> character, like we do when we try and detect that the edit has gained a space
> character and react to it.

We can treat keys like Tab as accelerators, because they don't generate content.
But we can not do it for Space, because it generates content, so it IS a
character.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
On 2011/01/14 02:30:36, Peter Kasting wrote:
> On 2011/01/14 02:02:46, James Su wrote:
> > Existing code doesn't accept keyword by Tab key if the hint is not visible.
I
> > can change this behavior, if you think it's not correct.
> 
> Yeah, that seems like a bug.

According to the original code, it's on purpose rather than a bug. Maybe we need
to confirm it with UX designer.

[Peter Kasting, 2011-01-14 17:31:33]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 03:26:22, James Su wrote:
> There is no way on Windows to intercept a key event before the IME.

I've been reading a few MSDN docs and I don't think this is correct.

First, it sounds as if when the IME processes VK_SPACE and does something
special with it, the WM_KEYDOWN and WM_KEYUP messages that are passed on to us
are changed to say VK_PROCESSKEY instead of VK_SPACE.  That sounds as if just
checking VK_SPACE will do what we want since it won't occur when the IME handles
it specially.  It would be nice to test this.

Furthermore, it's possible to retrieve the original keycode in a VK_PROCESSKEY
case with ImmGetVirtualKey(), and it's also possible to set our own input
context so we get first crack at all IME messages before we hand them to the
"real" IME.  I'm not sure either of those is terribly useful, though.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
On 2011/01/14 03:26:22, James Su wrote:
> On 2011/01/14 02:30:36, Peter Kasting wrote:
> > On 2011/01/14 02:02:46, James Su wrote:
> > > Existing code doesn't accept keyword by Tab key if the hint is not
visible.
> I
> > > can change this behavior, if you think it's not correct.
> > 
> > Yeah, that seems like a bug.
> 
> According to the original code, it's on purpose rather than a bug. Maybe we
need
> to confirm it with UX designer.

I am the UX designer.  The original code was wrong.

[James Su, 2011-01-14 18:04:12]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 17:31:33, Peter Kasting wrote:
> On 2011/01/14 03:26:22, James Su wrote:
> > There is no way on Windows to intercept a key event before the IME.
> 
> I've been reading a few MSDN docs and I don't think this is correct.
> 
> First, it sounds as if when the IME processes VK_SPACE and does something
> special with it, the WM_KEYDOWN and WM_KEYUP messages that are passed on to us
> are changed to say VK_PROCESSKEY instead of VK_SPACE.  That sounds as if just
> checking VK_SPACE will do what we want since it won't occur when the IME
handles
> it specially.  It would be nice to test this.
Yes exactly. But if the user uses an IME and enables "full width" mode, then the
IME will handle the space key and insert an ideographic space instead. An IME
may even handle the space key then insert a normal half width space character. I
do think we need to accept keyword in such cases. But the IME may not always
generate an whitespace character when handling the space key, so we still need
to check the content generated by the space key to see whether it's a whitespace
character or not.

So again, the key point here is: the space key IS a character, which may
generate arbitrary content when using different IME and/or keyboard layout. We
cannot simply ignore the content.

> 
> Furthermore, it's possible to retrieve the original keycode in a VK_PROCESSKEY
> case with ImmGetVirtualKey(), and it's also possible to set our own input
> context so we get first crack at all IME messages before we hand them to the
> "real" IME.  I'm not sure either of those is terribly useful, though.
As I said above, no matter what we do, we always need to check the content
generated by a space key event. And please take Linux and Mac into account as
well, which have totally different mechanism for handling keyboard events.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.h:103: virtual bool
IsKeywordHintVisible() const = 0;
On 2011/01/14 17:31:33, Peter Kasting wrote:
> On 2011/01/14 03:26:22, James Su wrote:
> > On 2011/01/14 02:30:36, Peter Kasting wrote:
> > > On 2011/01/14 02:02:46, James Su wrote:
> > > > Existing code doesn't accept keyword by Tab key if the hint is not
> visible.
> > I
> > > > can change this behavior, if you think it's not correct.
> > > 
> > > Yeah, that seems like a bug.
> > 
> > According to the original code, it's on purpose rather than a bug. Maybe we
> need
> > to confirm it with UX designer.
> 
> I am the UX designer.  The original code was wrong.
Ok, I'll remove those code. It even makes the code simpler :)

[James Su, 2011-01-18 20:31:59]

ping.

[Peter Kasting, 2011-01-18 21:41:18]

On Tue, Jan 18, 2011 at 12:31 PM, <suzhe@chromium.org> wrote:

> ping.


Have patience, I'm out sick.

I wrote a response on Friday but it's not appearing here, so maybe it's
still a draft on my work machine.  I'll check when I next make it in.

http://codereview.chromium.org/6252003/
>

[James Su, 2011-01-18 21:53:29]

On 2011/01/18 21:41:18, Peter Kasting wrote:
> On Tue, Jan 18, 2011 at 12:31 PM, <mailto:suzhe@chromium.org> wrote:
> 
> > ping.
> 
> 
> Have patience, I'm out sick.
Ok. Take care.

> 
> I wrote a response on Friday but it's not appearing here, so maybe it's
> still a draft on my work machine.  I'll check when I next make it in.
> 
> http://codereview.chromium.org/6252003/
> >

[Peter Kasting, 2011-01-19 19:24:42]

Here's the draft comment I never sent.

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/14 18:04:12, James Su wrote:
> Yes exactly. But if the user uses an IME and enables "full width" mode, then
the
> IME will handle the space key and insert an ideographic space instead. An IME
> may even handle the space key then insert a normal half width space character.
I
> do think we need to accept keyword in such cases. But the IME may not always
> generate an whitespace character when handling the space key, so we still need
> to check the content generated by the space key to see whether it's a
whitespace
> character or not.

It seems like acting directly on VK_SPACE might be correct, but insufficient to
catch everything we care about.  Is that accurate?

If so, then what about checking WM_CHAR or WM_IME_CHAR for a space character? 
Maybe checking WM_IME_CHAR catches the other cases?

What about cases where the user doesn't press space (and space only) directly
but a space is inserted?  Maybe it's one of those Chinese IMEs that inserts
whitespace during composition or maybe we had a composition string and then
deleted most of it or I don't know.  Do we need a two-phase check for this where
we only act on a space character when we also just saw a WM_KEYDOWN or
VK_PROCESSKEY and ImmGetVirtualKey() returned VK_SPACE?

> And please take Linux and Mac into account as
> well, which have totally different mechanism for handling keyboard events.

I want to understand my options on Windows and decide what the best course is
there before moving on to worry about Mac and Linux.

[James Su, 2011-01-19 19:57:15]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/19 19:24:42, Peter Kasting wrote:
> On 2011/01/14 18:04:12, James Su wrote:
> > Yes exactly. But if the user uses an IME and enables "full width" mode, then
> the
> > IME will handle the space key and insert an ideographic space instead. An
IME
> > may even handle the space key then insert a normal half width space
character.
> I
> > do think we need to accept keyword in such cases. But the IME may not always
> > generate an whitespace character when handling the space key, so we still
need
> > to check the content generated by the space key to see whether it's a
> whitespace
> > character or not.
> 
> It seems like acting directly on VK_SPACE might be correct, but insufficient
to
> catch everything we care about.  Is that accurate?
One possible case is that an IME may use a different key to insert whitespace
character, and I think we still need to trigger keyword mode in this case.

> 
> If so, then what about checking WM_CHAR or WM_IME_CHAR for a space character? 
> Maybe checking WM_IME_CHAR catches the other cases?
An IME may also use WM_IME_COMPOSITION to insert the whitespace character. I'm
actually not sure about WM_IME_CHAR.

> 
> What about cases where the user doesn't press space (and space only) directly
> but a space is inserted?  Maybe it's one of those Chinese IMEs that inserts
> whitespace during composition or maybe we had a composition string and then
> deleted most of it or I don't know.  Do we need a two-phase check for this
where
> we only act on a space character when we also just saw a WM_KEYDOWN or
> VK_PROCESSKEY and ImmGetVirtualKey() returned VK_SPACE?
I'm actually don't want to limit it to VK_SPACE. As I said above, an IME can use
any key to insert whitespace character, and we should definitely treat it as
normal space key and trigger keyword mode.
And besides the key event issue, for some IMEs, especially Korean and
Traditional Chinese, when the user press the space key, the IME may confirm the
current composition text and then insert a whitespace character (either half or
full width) immediately. Some IMEs may combine the whitespace character with the
current composition text and confirm them together. We need to trigger keyword
mode for such cases as well, otherwise those users may never trigger a keyword
in their native language by space.

> 
> > And please take Linux and Mac into account as
> > well, which have totally different mechanism for handling keyboard events.
> 
> I want to understand my options on Windows and decide what the best course is
> there before moving on to worry about Mac and Linux.

[Peter Kasting, 2011-01-19 20:07:05]

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete_edit.cc:669:
MaybeAcceptKeywordBySpace(new_user_text))
On 2011/01/19 19:57:15, James Su wrote:
> One possible case is that an IME may use a different key to insert whitespace
> character, and I think we still need to trigger keyword mode in this case.

We should not trigger keyword mode for this case.  The whole point is to treat
space as an accelerator.  If I wanted keyword mode to trigger on some other
method of inserting whitespace here, I'd want it to trigger when pasting in a
string with a space too.

> > If so, then what about checking WM_CHAR or WM_IME_CHAR for a space
character? 
> > Maybe checking WM_IME_CHAR catches the other cases?
> An IME may also use WM_IME_COMPOSITION to insert the whitespace character. I'm
> actually not sure about WM_IME_CHAR.

Supposedly we get WM_IME_CHAR any time the IME has synthesized a character from
one or more original keypresses.

> I'm actually don't want to limit it to VK_SPACE. As I said above, an IME can
use
> any key to insert whitespace character, and we should definitely treat it as
> normal space key and trigger keyword mode.

We should definitely _not_ do that.

> And besides the key event issue, for some IMEs, especially Korean and
> Traditional Chinese, when the user press the space key, the IME may confirm
the
> current composition text and then insert a whitespace character (either half
or
> full width) immediately. Some IMEs may combine the whitespace character with
the
> current composition text and confirm them together. We need to trigger keyword
> mode for such cases as well, otherwise those users may never trigger a keyword
> in their native language by space.

This is why I'm trying to get us to a solution that involves looking at the
original keypress, not at what the IME inserts.

[James Su, 2011-01-19 20:49:05]

On 2011/01/19 20:07:05, Peter Kasting wrote:
>
http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
> File chrome/browser/autocomplete/autocomplete_edit.cc (right):
> 
>
http://codereview.chromium.org/6252003/diff/1/chrome/browser/autocomplete/aut...
> chrome/browser/autocomplete/autocomplete_edit.cc:669:
> MaybeAcceptKeywordBySpace(new_user_text))
> On 2011/01/19 19:57:15, James Su wrote:
> > One possible case is that an IME may use a different key to insert
whitespace
> > character, and I think we still need to trigger keyword mode in this case.
> 
> We should not trigger keyword mode for this case.  The whole point is to treat
> space as an accelerator.  If I wanted keyword mode to trigger on some other
> method of inserting whitespace here, I'd want it to trigger when pasting in a
> string with a space too.
Why can't the user (or the IME) remap a different key to space? I'd prefer to
treat keyword mode trigger as an action, which can be activated by a key event
which inserts a whitespace character, rather than treating the space key itself
as an accelerator. Pasting is a totally different action, so it shouldn't
trigger keyword mode.
Mac and Linux/Gtk actually use this "action" model.

> 
> > > If so, then what about checking WM_CHAR or WM_IME_CHAR for a space
> character? 
> > > Maybe checking WM_IME_CHAR catches the other cases?
> > An IME may also use WM_IME_COMPOSITION to insert the whitespace character.
I'm
> > actually not sure about WM_IME_CHAR.
> 
> Supposedly we get WM_IME_CHAR any time the IME has synthesized a character
from
> one or more original keypresses.
I believe it's free for an IME to choose the message it want.

> 
> > I'm actually don't want to limit it to VK_SPACE. As I said above, an IME can
> use
> > any key to insert whitespace character, and we should definitely treat it as
> > normal space key and trigger keyword mode.
> 
> We should definitely _not_ do that.
> 
> > And besides the key event issue, for some IMEs, especially Korean and
> > Traditional Chinese, when the user press the space key, the IME may confirm
> the
> > current composition text and then insert a whitespace character (either half
> or
> > full width) immediately. Some IMEs may combine the whitespace character with
> the
> > current composition text and confirm them together. We need to trigger
keyword
> > mode for such cases as well, otherwise those users may never trigger a
keyword
> > in their native language by space.
> 
> This is why I'm trying to get us to a solution that involves looking at the
> original keypress, not at what the IME inserts.
Then, we effectively prevent those IME users from using this feature. For most
of them, they actually don't care about (actually don't understand) the
difference between IME composition mode and normal input mode.
So, we shouldn't treat them differently.

[Peter Kasting, 2011-01-19 21:03:51]

On 2011/01/19 20:49:05, James Su wrote:
> Why can't the user (or the IME) remap a different key to space?

We don't let the user remap a different key to <tab>.  Why is this different?

> I'd prefer to
> treat keyword mode trigger as an action, which can be activated by a key event
> which inserts a whitespace character, rather than treating the space key
itself
> as an accelerator. Pasting is a totally different action, so it shouldn't
> trigger keyword mode.

But then you have to rethink scenarios like "select the post-keyword text and
hit space to replace it all with a space", which in your model should probably
trigger keyword mode.

I'm also not sure why, if any sequence of keys which winds up inserting a space
should trigger keyword mode, you consider pasting to be "a different action", at
least in the case of pasting a space character after the keyword.  Shouldn't
that trigger keyword mode too in the world you describe?

At the end of the day I'm concerned about two things:
* Users should never trigger keyword mode while using an IME in a way that
interrupts their attempt to input/compose some string.
* Users should never accidentally trigger keyword mode while trying to modify
the edit contents if the edit just happens to transiently look like a keyword.

Treating space as an accelerator accomplishes these and is also parallel to how
the rest of our code already works.  Trying to trigger on actions that insert a
space character makes me nervous about both and also makes me worry about
additional complexity and future bugs as we hijack the normal
OnAfterPossibleChange() process to go back in time and modify the user's
actions.

If you are convinced you can avoid all the above issues I could maybe be swayed
to go this way, but it makes me uncomfortable.

[James Su, 2011-01-19 22:42:28]

On 2011/01/19 21:03:51, Peter Kasting wrote:
> On 2011/01/19 20:49:05, James Su wrote:
> > Why can't the user (or the IME) remap a different key to space?
> 
> We don't let the user remap a different key to <tab>.  Why is this different?
This is actually not true, at least on Mac. On Linux I restricted it to Tab key
explicitly to follow Windows' behavior, though I don't think it's correct.
Actually both Mac and Linux/Gtk use the "action" model. For example, on Mac when
Tab key is pressed, a command named "insertTab" will be executed, we trigger
keyword mode by handling this command. It's free for the user to remap a
different key to "insertTab" command at system level, then that key will act
exactly like the real Tab key, though it generates a different key code. If the
user pastes a tab character into the edit box, command "paste" will be executed
instead of "insertTab", so that the application can distinguish these two
actions. Linux/Gtk uses a similar model, although it uses a different set of
action names.
So we actually should allow the user to remap Tab key. We already support it on
Mac, and very easy to support it on Linux by deleting some restriction code. But
I'm not sure if it's feasible on Windows.
Mac and Linux/Gtk have distinct actions for keys like Tab, Enter, Backspace,
Delete, arrows, etc. but not for space key, so we need to check the result
content.

> 
> > I'd prefer to
> > treat keyword mode trigger as an action, which can be activated by a key
event
> > which inserts a whitespace character, rather than treating the space key
> itself
> > as an accelerator. Pasting is a totally different action, so it shouldn't
> > trigger keyword mode.
> 
> But then you have to rethink scenarios like "select the post-keyword text and
> hit space to replace it all with a space", which in your model should probably
> trigger keyword mode.
This CL doesn't trigger keyword mode in this scenario.

> 
> I'm also not sure why, if any sequence of keys which winds up inserting a
space
> should trigger keyword mode, you consider pasting to be "a different action",
at
> least in the case of pasting a space character after the keyword.  Shouldn't
> that trigger keyword mode too in the world you describe?
As I described above, pasting is a different action, so we should treat it
differently. And we do want to avoid triggering keyword mode in such scenario.
The only scenario this CL doesn't avoid is drag-n-drop a single space character.
Though it's easy to handle, I don't think it's worth, as this scenario is very
rare.

> 
> At the end of the day I'm concerned about two things:
> * Users should never trigger keyword mode while using an IME in a way that
> interrupts their attempt to input/compose some string.
> * Users should never accidentally trigger keyword mode while trying to modify
> the edit contents if the edit just happens to transiently look like a keyword.
This CL achieves these two goals exactly, no more no less, in a platform
independent way.

> 
> Treating space as an accelerator accomplishes these and is also parallel to
how
> the rest of our code already works.  Trying to trigger on actions that insert
a
> space character makes me nervous about both and also makes me worry about
> additional complexity and future bugs as we hijack the normal
> OnAfterPossibleChange() process to go back in time and modify the user's
> actions.
Avoiding additional complexity and future bugs is the most important reason for
me to choose this platform independent solution, so that we don't need to deal
with three different implementations and endless compatibility issues with
variety of input methods.

> 
> If you are convinced you can avoid all the above issues I could maybe be
swayed
> to go this way, but it makes me uncomfortable.

[Peter Kasting, 2011-01-20 00:04:22]

OK, you convinced me.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:305: (paste_state_ ==
REPLACED_ALL), keyword_is_selected, keyword_is_selected);
Let's go ahead and leave the check here as "paste_state_ != NONE" so that we
prevent autocompletion immediately after any type of paste.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:828: if (paste_state_ != NONE
|| !is_keyword_hint_ || keyword_.empty() ||
Nit: This file puts parens around boolean subconditions.

With one change elsewhere*, we can condense this whole function to a single
conditional:

  return (paste_state_ == NONE) && is_keyword_hint_ && !keyword_.empty() &&
      inline_autocomplete_text_.empty() && (user_text_ == keyword_) &&
      (new_user_text_.length() == user_text_.length() + 1) &&
      !new_user_text_.compare(0, user_text_.length(), user_text_) &&
      (new_user_text_.find_first_of(L" XYZ", user_text_.length()) !=
          std::wstring::npos) && AcceptKeyword();

...where "XYZ" is replaced by the escapes for the various space characters you
want, e.g. U+3000.

This purposefully avoids using IsWhitespace() because characters like \t, \r,
\n, etc. shouldn't be treated as triggering keyword mode.

*Namely, make AcceptKeyword() return a bool (always true) and document it as
returning true for caller convenience.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.h:330: NONE,           // Most
recent edit was not a paste that replaced all text.
Nit: These might make more sense in the order NONE, PASTING, REPLACING_ALL,
PASTED, REPLACED_ALL?

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.h:391: // Accepts current keyword
if the user only typed a whitespace at the end of
Nit: whitespace -> space

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1629:
model_->AcceptKeyword();
Nit: If you change AcceptKeyword() to return true, you can condense this to
"handled = model_->AcceptKeyword();".

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:865: ((new_sel.cpMin
!= new_sel.cpMax) ||
Nit: I suggest using a similar addition to this in the GTK code rather than
adding an empty() function.

[James Su, 2011-01-20 06:49:31]

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:305: (paste_state_ ==
REPLACED_ALL), keyword_is_selected, keyword_is_selected);
On 2011/01/20 00:04:22, Peter Kasting wrote:
> Let's go ahead and leave the check here as "paste_state_ != NONE" so that we
> prevent autocompletion immediately after any type of paste.

Done.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:828: if (paste_state_ != NONE
|| !is_keyword_hint_ || keyword_.empty() ||
On 2011/01/20 00:04:22, Peter Kasting wrote:
> Nit: This file puts parens around boolean subconditions.
> 
> With one change elsewhere*, we can condense this whole function to a single
> conditional:
> 
>   return (paste_state_ == NONE) && is_keyword_hint_ && !keyword_.empty() &&
>       inline_autocomplete_text_.empty() && (user_text_ == keyword_) &&
>       (new_user_text_.length() == user_text_.length() + 1) &&
>       !new_user_text_.compare(0, user_text_.length(), user_text_) &&
>       (new_user_text_.find_first_of(L" XYZ", user_text_.length()) !=
>           std::wstring::npos) && AcceptKeyword();
> 
> ...where "XYZ" is replaced by the escapes for the various space characters you
> want, e.g. U+3000.
> 
> This purposefully avoids using IsWhitespace() because characters like \t, \r,
> \n, etc. shouldn't be treated as triggering keyword mode.
> 
> *Namely, make AcceptKeyword() return a bool (always true) and document it as
> returning true for caller convenience.

Done with slightly different logic, because:
1. the |user_text_| may not be same as the |keyword_|, eg. "www.google.com" may
trigger keyword "google.com"
2. the keyword may contain space characters in the middle (issue 6867), so we
need to check the last character explicitly.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.h (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.h:330: NONE,           // Most
recent edit was not a paste that replaced all text.
On 2011/01/20 00:04:22, Peter Kasting wrote:
> Nit: These might make more sense in the order NONE, PASTING, REPLACING_ALL,
> PASTED, REPLACED_ALL?

Done.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.h:391: // Accepts current keyword
if the user only typed a whitespace at the end of
On 2011/01/20 00:04:22, Peter Kasting wrote:
> Nit: whitespace -> space

Done.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1629:
model_->AcceptKeyword();
On 2011/01/20 00:04:22, Peter Kasting wrote:
> Nit: If you change AcceptKeyword() to return true, you can condense this to
> "handled = model_->AcceptKeyword();".

Done.

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:865: ((new_sel.cpMin
!= new_sel.cpMax) ||
On 2011/01/20 00:04:22, Peter Kasting wrote:
> Nit: I suggest using a similar addition to this in the GTK code rather than
> adding an empty() function.

Done.

[Peter Kasting, 2011-01-20 18:49:23]

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/20001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:828: if (paste_state_ != NONE
|| !is_keyword_hint_ || keyword_.empty() ||
On 2011/01/20 06:49:31, James Su wrote:
> 2. the keyword may contain space characters in the middle (issue 6867), so we
> need to check the last character explicitly.

That's why my find_first_of() call had a second argument.  You must have missed
that.  Oh well, no big deal.

http://codereview.chromium.org/6252003/diff/35001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:796: (type ==
AutocompleteMatch::SEARCH_SUGGEST));
Are you _really_ sure we want all of these to trigger keyword mode?

Is there some ICU function we can use here instead of writing our own?

[James Su, 2011-01-20 19:22:14]

http://codereview.chromium.org/6252003/diff/35001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6252003/diff/35001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:796: case 0x0020:  // Space
On 2011/01/20 18:49:23, Peter Kasting wrote:
> Are you _really_ sure we want all of these to trigger keyword mode?
> 
> Is there some ICU function we can use here instead of writing our own?

I'm actually not sure.
AFAIK, besides 0x0020, many keyboard layouts or input methods can input 0x00A0
and 0x3000. I'm not sure about others.
But we probably should disallow 0x00A0 and 0x202F, as they are "no break".
So I think may be we can just allow 0x0020 and 0x3000 for now and add others
later when necessary.
What do you think?

[Peter Kasting, 2011-01-20 19:33:37]

On 2011/01/20 19:22:14, James Su wrote:
> So I think may be we can just allow 0x0020 and 0x3000 for now and add others
> later when necessary.
> What do you think?

OK.

In that case, you can save some code by moving back to the find_first_of("
\x3000", user_text_.length()) call.

[James Su, 2011-01-20 19:56:35]

On 2011/01/20 19:33:37, Peter Kasting wrote:
> On 2011/01/20 19:22:14, James Su wrote:
> > So I think may be we can just allow 0x0020 and 0x3000 for now and add others
> > later when necessary.
> > What do you think?
> 
> OK.
> 
> In that case, you can save some code by moving back to the find_first_of("
> \x3000", user_text_.length()) call.
I'd prefer to keep existing style, which is more readable for me and easier for
future maintenance.

[Peter Kasting, 2011-01-20 20:36:59]

LGTM then