Cursor jumps to end of omnibox when activating keyword mode via inserted space

Cursor jumps to end of omnibox when activating keyword mode via inserted space
    
When 'keyword' search mode is activated by inserting a space between keyword and search string, the old caret position should be retained.
    
The caret should be positioned at the start of the search string after stripping the keyword string from the user text.
    
Patch from Joe Thomas <mhx348@motorola.com>;.
    
BUG=142803
TEST=Browser unit test, steps as mentioned in the bug report

R=pkasting@chromium.org


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

[Peter Kasting, 2012-10-02 07:37:12]

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:887: view_->GetText() !=
DisplayTextFromUserText(user_text_)) {
Does this do the right thing in cases where we have (new or existing) inline
autocompletion in keyword mode?  Or when we're tabbing through the dropdown and
thus have temporary text?  I'm worried about those causing cases where the
display text does not match the user text and thus erroneously triggering this
block.

[Joe Thomas, 2012-10-02 18:39:23]

Thanks for the review. Please see my comments inline

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:887: view_->GetText() !=
DisplayTextFromUserText(user_text_)) {

On 2012/10/02 07:37:12, Peter Kasting wrote:
> Does this do the right thing in cases where we have (new or existing) inline
> autocompletion in keyword mode? 

When keyword search mode is activated, AFAIK there is no more inline
autocompletion. OmniboxEditModel::OnPopupDataChanged is always called with  NULL
value for 'text' parameter while 'keyword' search mode is active. And this sets
the existing inline_autocomplete_text_ to NULL in
OmniboxEditModel::OnPopupDataChanged.

> Or when we're tabbing through the dropdown and
> thus have temporary text?  

This case is handled in the block just above and it returns from there.

>I'm worried about those causing cases where the
> display text does not match the user text and thus erroneously triggering this
> block.

[Peter Kasting, 2012-10-02 18:42:35]

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:887: view_->GetText() !=
DisplayTextFromUserText(user_text_)) {
On 2012/10/02 18:39:23, Joe Thomas wrote:
> On 2012/10/02 07:37:12, Peter Kasting wrote:
> > Does this do the right thing in cases where we have (new or existing) inline
> > autocompletion in keyword mode? 
> 
> When keyword search mode is activated, AFAIK there is no more inline
> autocompletion. OmniboxEditModel::OnPopupDataChanged is always called with 
NULL
> value for 'text' parameter while 'keyword' search mode is active. And this
sets
> the existing inline_autocomplete_text_ to NULL in
> OmniboxEditModel::OnPopupDataChanged.

We can definitely inline autocomplete while in keyword search mode.  I just
tested now: tab-to-search google.com and type "test"; then after the page loads,
do the same thing, and "test" will inline autocomplete as soon as you type the
't'.

[Joe Thomas, 2012-10-02 18:57:24]

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:887: view_->GetText() !=
DisplayTextFromUserText(user_text_)) {
On 2012/10/02 18:42:35, Peter Kasting wrote:
> On 2012/10/02 18:39:23, Joe Thomas wrote:
> > On 2012/10/02 07:37:12, Peter Kasting wrote:
> > > Does this do the right thing in cases where we have (new or existing)
inline
> > > autocompletion in keyword mode? 
> > 
> > When keyword search mode is activated, AFAIK there is no more inline
> > autocompletion. OmniboxEditModel::OnPopupDataChanged is always called with 
> NULL
> > value for 'text' parameter while 'keyword' search mode is active. And this
> sets
> > the existing inline_autocomplete_text_ to NULL in
> > OmniboxEditModel::OnPopupDataChanged.
> 
> We can definitely inline autocomplete while in keyword search mode.  I just
> tested now: tab-to-search http://google.com and type "test"; then after the
page loads,
> do the same thing, and "test" will inline autocomplete as soon as you type the
> 't'.

Yes, you are right. The patch breaks that usecase. I will re-work on the patch.
I did not know that there is an inline autocompletion available for search
strings. Sorry for my ignorance.

[Joe Thomas, 2012-10-02 20:59:41]

Uploaded patch set 2

[Peter Kasting, 2012-10-02 21:52:02]

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:889:
view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,
This still feels kind of wrong to me.

Where does the existing code take care of changing the window text when we
switch into keyword mode?  It seems like that location, or else the code that
actually handles changing into keyword mode when inserting a space, ought to be
where we set the caret position.

[Joe Thomas, 2012-10-02 22:11:34]

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:889:
view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,
On 2012/10/02 21:52:02, Peter Kasting wrote:
> This still feels kind of wrong to me.
> 
> Where does the existing code take care of changing the window text when we
> switch into keyword mode?  

The existing code which changes the display text is 
view_->OnInlineAutocompleteTextMaybeChanged function call(which I moved to the
else..if condition below). DisplayTextFromUserText strips the "keyword" from
user_text if we are in keyword search mode and pass it to the view for
displaying.

>It seems like that location, or else the code that
> actually handles changing into keyword mode when inserting a space, ought to
be
> where we set the caret position.

[Peter Kasting, 2012-10-02 22:38:28]

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:889:
view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,
On 2012/10/02 22:11:34, Joe Thomas wrote:
> On 2012/10/02 21:52:02, Peter Kasting wrote:
> > This still feels kind of wrong to me.
> > 
> > Where does the existing code take care of changing the window text when we
> > switch into keyword mode?  
> 
> The existing code which changes the display text is 
> view_->OnInlineAutocompleteTextMaybeChanged function call(which I moved to the
> else..if condition below). DisplayTextFromUserText strips the "keyword" from
> user_text if we are in keyword search mode and pass it to the view for
> displaying.

This just doesn't seem like the right place to be touching this.

Perhaps it would be better to add code to the conditional above that checks if
the keyword state changed.  When a keyword state change occurs, we should
probably make the code at that point toggle the view's text into or out of
keyword mode, translating whatever the current cursor position is appropriately.
 Then the code down here can continue to run normally.

[Joe Thomas, 2012-10-03 01:59:47]

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:889:
view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,
On 2012/10/02 22:38:28, Peter Kasting wrote:
> On 2012/10/02 22:11:34, Joe Thomas wrote:
> > On 2012/10/02 21:52:02, Peter Kasting wrote:
> > > This still feels kind of wrong to me.
> > > 
> > > Where does the existing code take care of changing the window text when we
> > > switch into keyword mode?  
> > 
> > The existing code which changes the display text is 
> > view_->OnInlineAutocompleteTextMaybeChanged function call(which I moved to
the
> > else..if condition below). DisplayTextFromUserText strips the "keyword" from
> > user_text if we are in keyword search mode and pass it to the view for
> > displaying.
> 
> This just doesn't seem like the right place to be touching this.
> 
> Perhaps it would be better to add code to the conditional above that checks if
> the keyword state changed.  When a keyword state change occurs, we should
> probably make the code at that point toggle the view's text into or out of
> keyword mode, translating whatever the current cursor position is
appropriately.
>  Then the code down here can continue to run normally.

I doubt I completely understand your logic from your description. I am sorry for
that. I would like to explain the logic behind the current patch.

The complete code flow is as follows:

1. Whenever there is a change in the omnibox text(including space),
OmniboxEditModel gets a notification at OnAfterPossibleChange() which calls
view_->UpdatePopup()

2. This invokes AutocompleteController::Start from
OmniboxEditModel::StartAutocomplete. Once it is finished, we  get a call back at
OmniboxEditModel::OnResultChanged

3. OmniboxEditModel::OnResultChanged invokes OnPopupDataChanged(). Only at this
point, we will be able to decide "keyword" search mode is activated. And the
current patch modifies this function.

The reason for moving the existing condition to "else..if" is that if the 'newly
added' condition evaluates to true, then the existing condition will be a no-op
if kept separate(view_->OnInlineAutocompleteTextMaybeChanged() does a string
match and return early).

[Peter Kasting, 2012-10-03 03:01:01]

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:889:
view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,
Yes, I understood all that.

I've just spent another hour or so pondering this change.  I think it might work
correctly, though I think you can simplify your conditional to just "if
(keyword_state_changed && KeywordIsSelected())", and I think you can pass
"false" for the last arg of SetWindowTextAndCaretPos() and then remove the set
of |call_controller_onchanged| to false.  You also need some comments about what
you're doing and why (in particular, calling out why keyword mode changes that
come through here are ones where it's guaranteed correct to put the cursor at
the front of the replacement string).  Also, you should test the following kinds
of interactions exhaustively with the cursor at different positions at the
start, to make sure it's positioned correctly at the end:

(1) Keyword search is the default result, we arrow away and then hit esc to
return
(2) Keyword search is the default result, we arrow away, then shift delete the
result we arrow to (not: must be a deletable result, e.g. one from history); it
may be necessary to hit escape at this point to return to the keyword result

Note that in these cases, whether esc restores keyword search mode or not may
depend on how we entered keyword search mode and whether we've typed anything
since entering it.  Just make sure your patch doesn't change any of the
behaviors of the current code.

[Joe Thomas, 2012-10-03 05:41:12]

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/8001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:889:
view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text_), 0,
On 2012/10/03 03:01:01, Peter Kasting wrote:
> Yes, I understood all that.
> 
> I've just spent another hour or so pondering this change.  I think it might
work
> correctly, though I think you can simplify your conditional to just "if
> (keyword_state_changed && KeywordIsSelected())", and I think you can pass
> "false" for the last arg of SetWindowTextAndCaretPos() and then remove the set
> of |call_controller_onchanged| to false.  You also need some comments about
what
> you're doing and why (in particular, calling out why keyword mode changes that
> come through here are ones where it's guaranteed correct to put the cursor at
> the front of the replacement string).  Also, you should test the following
kinds
> of interactions exhaustively with the cursor at different positions at the
> start, to make sure it's positioned correctly at the end:
> 
> (1) Keyword search is the default result, we arrow away and then hit esc to
> return
> (2) Keyword search is the default result, we arrow away, then shift delete the
> result we arrow to (not: must be a deletable result, e.g. one from history);
it
> may be necessary to hit escape at this point to return to the keyword result
> 
> Note that in these cases, whether esc restores keyword search mode or not may
> depend on how we entered keyword search mode and whether we've typed anything
> since entering it.  Just make sure your patch doesn't change any of the
> behaviors of the current code.

Thanks for the detailed review. I will make the required changes and test it as
per your suggestions.

[Joe Thomas, 2012-10-03 07:59:59]

Uploaded Patch Set 3.

Review comments incorporated.

Done testing around omnibox and no regressions found. Also tested the 'keyword'
related usecases suggested in the review comments and no issues found.

[Peter Kasting, 2012-10-03 17:08:12]

LGTM with some suggestions

https://codereview.chromium.org/11034019/diff/15001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/15001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:892: // string to maintain the
old position, it needs to be set to 0.
Thanks for adding comments here.

I wonder if we could make them even clearer, something like:

  if (keyword_state_changed && KeywordIsSelected()) {
    // If we reach here, the user most likely entered keyword mode by inserting
    // a space between a keyword name and a search string (as pressing space or
    // tab after the keyword name alone would have been be handled in
    // MaybeAcceptKeywordBySpace() by calling AcceptKeyword(), which won't reach
    // here).  In this case, we don't want to call
    // OnInlineAutocompleteTextMaybeChanged() as normal, because that will
    // correctly change the text (to the search string alone) but move the caret
    // to the end of the string; instead we want the caret at the start of the
    // search string since that's where it was in the original input.  So we set
    // the text and caret position directly.
    //
    // It may also be possible to reach here if we're reverting from having
    // temporary text back to a default match that's a keyword search, but in
    // that case the RevertTemporaryText() call below will reset the caret or
    // selection correctly so the caret positioning we do here won't matter.
    ...

https://codereview.chromium.org/11034019/diff/15001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:893: if (keyword_state_changed
&& KeywordIsSelected())
Nit: {} needed on both arms, as one arm's body is >1 line

[Joe Thomas, 2012-10-03 18:04:58]

Uploaded patch set 4. 

This is my first patch. Could you please help me to send the patch to try server
and later submit it?

Thanks for the review.

https://codereview.chromium.org/11034019/diff/15001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11034019/diff/15001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:892: // string to maintain the
old position, it needs to be set to 0.
On 2012/10/03 17:08:12, Peter Kasting wrote:
> Thanks for adding comments here.
> 
> I wonder if we could make them even clearer, something like:
> 
>   if (keyword_state_changed && KeywordIsSelected()) {
>     // If we reach here, the user most likely entered keyword mode by
inserting
>     // a space between a keyword name and a search string (as pressing space
or
>     // tab after the keyword name alone would have been be handled in
>     // MaybeAcceptKeywordBySpace() by calling AcceptKeyword(), which won't
reach
>     // here).  In this case, we don't want to call
>     // OnInlineAutocompleteTextMaybeChanged() as normal, because that will
>     // correctly change the text (to the search string alone) but move the
caret
>     // to the end of the string; instead we want the caret at the start of the
>     // search string since that's where it was in the original input.  So we
set
>     // the text and caret position directly.
>     //
>     // It may also be possible to reach here if we're reverting from having
>     // temporary text back to a default match that's a keyword search, but in
>     // that case the RevertTemporaryText() call below will reset the caret or
>     // selection correctly so the caret positioning we do here won't matter.
>     ...

This looks a lot better than the comments that I added. Thank you so much.

https://codereview.chromium.org/11034019/diff/15001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:893: if (keyword_state_changed
&& KeywordIsSelected())
On 2012/10/03 17:08:12, Peter Kasting wrote:
> Nit: {} needed on both arms, as one arm's body is >1 line

Done.

[Peter Kasting, 2012-10-03 18:18:03]

On 2012/10/03 18:04:58, Joe Thomas wrote:
> This is my first patch. Could you please help me to send the patch to try
server
> and later submit it?

Actually this made me realize that we should probably try to test this.  I
suspect you can modify chrome/browser/ui/omnibox/omnibox_view_browsertest.cc to
add some cursor-position-checking in the AcceptKeywordBySpace() function.

[Joe Thomas, 2012-10-03 20:29:31]

Uploaded patch set 5.

Added testcase. But I could not run it locally on my Linux machine as it looks
like the testcase "OmniboxViewTest.AcceptKeywordBySpace" is disabled for
LINUX_OS

[Joe Thomas, 2012-10-04 16:35:54]

Tried to add some bots, but it showed RED immediately(I don't think it even
ran). Is it because I don't have enough permission? Where can I check the logs?

[Peter Kasting, 2012-10-04 19:35:58]

On 2012/10/04 16:35:54, Joe Thomas wrote:
> Tried to add some bots, but it showed RED immediately(I don't think it even
> ran). Is it because I don't have enough permission? Where can I check the
logs?

I suspect you don't have permission.  I added some bots for you.  (And am now
running the linux bot again to see if the first run failed due to flakiness.)

[Joe Thomas, 2012-10-04 20:53:27]

On 2012/10/04 19:35:58, Peter Kasting wrote:

> I suspect you don't have permission.  I added some bots for you.  (And am now
> running the linux bot again to see if the first run failed due to flakiness.)

Thank you so much. Looks like it passed this time. Could you please help me to
commit the patch?

[commit-bot: I haz the power, 2012-10-04 20:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11034019/6004

[commit-bot: I haz the power, 2012-10-04 20:59:30]

Presubmit check for 11034019-6004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
MHX348@motorola.com is not in AUTHORS file. If you are a new contributor, please
visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 1.1s to calculate.

[Joe Thomas, 2012-10-04 21:05:02]

On 2012/10/04 20:59:30, I haz the power (commit-bot) wrote:
> Presubmit check for 11034019-6004 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Warnings **
> mailto:MHX348@motorola.com is not in AUTHORS file. If you are a new
contributor, please
> visit
> http://www.chromium.org/developers/contributing-code and read the "Legal"
> section
> If you are a chromite, verify the contributor signed the CLA.
> 
> Presubmit checks took 1.1s to calculate.

I added my friendly email id to AUTHORS file. I will change that to
mhx348@motorola.com. Also I will sign the CLA with this email address.

[commit-bot: I haz the power, 2012-10-04 21:30:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11034019/22001

[commit-bot: I haz the power, 2012-10-04 21:30:14]

Failed to apply patch for AUTHORS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file AUTHORS
  Hunk #1 FAILED at 202.
  1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

Patch:       AUTHORS
Index: AUTHORS
diff --git a/AUTHORS b/AUTHORS
index
33eb34e8b8867fccb7cc948ddb7ce6e8f3b0574c..6e67ea7942357c205cdd1efbfac4279a1a41036a
100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -202,3 +202,4 @@ Max Vujovic <mvujovic@adobe.com>
 Jakob Weigert <jakob.j.w@googlemail.com>
 Catalin Badea <badea@adobe.com>
 Joshua Lock <joshua.lock@intel.com>
+Joe Thomas <mhx348@motorola.com>

[commit-bot: I haz the power, 2012-10-04 22:33:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11034019/24003

[commit-bot: I haz the power, 2012-10-05 01:04:55]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-05 01:11:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11034019/24003

[commit-bot: I haz the power, 2012-10-05 03:08:14]

Change committed as 160312