[omnibox] Home cursor, then restore, on revert

If text was appended to a URL, the omnibox was shifting it to show the new text. This change makes it such that the text is only shifted if the user is editing. Otherwise, it homes the cursor, showing the origin.

BUG=681740
Review-Url: https://codereview.chromium.org/2860503004
Cr-Commit-Position: refs/heads/master@{#472440}
Committed: https://chromium.googlesource.com/chromium/src/+/5a8c1da2cda8f195695bc226dacdd35964df4113

[Kevin Bailey, 2017-05-15 17:01:15]

Description was changed from

==========
Always home cursor

BUG=
==========

to

==========
If text was appended to a URL, the omnibox was shifting it to show the new text.
This change makes it such that the text is only shifted if the user is editing.
Otherwise, it homes the cursor, showing the origin.

BUG=681740
==========

[Kevin Bailey, 2017-05-15 17:01:15]

krb@chromium.org changed reviewers:
+ pkasting@chromium.org

[Kevin Bailey, 2017-05-15 17:02:23]

Hi Peter,

I'm curious what you think of this, as a response to the issue. It requires
minimal change to the browser tests.

[Peter Kasting, 2017-05-15 19:07:10]

To make sure I have this clear, the issue is what happens when the underlying
URL updates while the user has their cursor in the omnibox but has not actually
changed the text.  We modify the text to display the new URL, but then have to
decide where to place the cursor, which also affects what's scrolled into view
in the case where the full text is too long to display all at once.  Did I get
it right?

If so, there aren't any really good choices.  The user having their cursor in
the omnibox means whatever we do will probably be disconcerting.

In the absence of security concerns, what do you think the usability tradeoffs
are of these three options in this specific case?:

(1) Cursor at beginning
(2) Cursor at end
(3) Cursor at previous cursor position (clamped to length of new string)

Today we do (2) and your patch does (1).  I want to know what you think of (3)
and also what you think users would perceive of these options apart from the
security issue.

[Kevin Bailey, 2017-05-15 20:36:35]

On 2017/05/15 19:07:10, Peter Kasting wrote:
> To make sure I have this clear, the issue is what happens when the underlying
> URL updates while the user has their cursor in the omnibox but has not
actually
> changed the text.  We modify the text to display the new URL, but then have to
> decide where to place the cursor, which also affects what's scrolled into view
> in the case where the full text is too long to display all at once.  Did I get
> it right?

yup

> If so, there aren't any really good choices.

ya

> The user having their cursor in
> the omnibox means whatever we do will probably be disconcerting.
> 
> In the absence of security concerns, what do you think the usability tradeoffs
> are of these three options in this specific case?:
> 
> (1) Cursor at beginning
> (2) Cursor at end
> (3) Cursor at previous cursor position (clamped to length of new string)
> 
> Today we do (2) and your patch does (1).  I want to know what you think of (3)
> and also what you think users would perceive of these options apart from the
> security issue.

#3 is intuitively correct, but I tried it (setting the cursor to whatever value
it currently was) but it still scrolls the text, until the cursor is in the
left-most position, which isn't terribly different from the original behavior.

Preserving the cursor also doesn't turn out to be useful in this case, since
the URL is being replaced anyways. Thus, the cursor is just placed within the
new URL. I think the user will be confused either way.

Preserving the cursor but scrolling to the left would be nice, but scrolling
right appears to be a Textfield side-effect from the append? I'm looking and
not seeing a trivial way to undo that.

[Peter Kasting, 2017-05-15 21:05:48]

On 2017/05/15 20:36:35, Kevin Bailey wrote:
> Preserving the cursor but scrolling to the left would be nice, but scrolling
> right appears to be a Textfield side-effect from the append? I'm looking and
> not seeing a trivial way to undo that.

Can we do something like:

SetCursorPosition(0);
SetCursorPosition(old cursor position);

In which case I'd assume that the initial call would scroll left, then the next
call wouldn't change the scroll position as long as the old cursor position was
still onscreen?  (If it's not onscreen, then it would scroll, but that's
unavoidable.)

Two other thoughts related to this scrolling stuff:
* Maybe Textfield's automatic "scroll cursor into view" behavior should try to
place the cursor in the middle of the region, or a third of the way from one of
the sides, instead of all the way at the edge.
* Maybe we need APIs in Textfield to control the scroll position separately from
the cursor position.  This would be useful in the omnibox anyway, because
there's a couple places that do things to adjust the cursor position which are
really about scrolling.

These are changes I've thought made sense for some time, and I thought at one
point I got some positive noises from msw (Textfield owner) about them.

[Kevin Bailey, 2017-05-15 23:33:30]

On 2017/05/15 21:05:48, Peter Kasting wrote:
> On 2017/05/15 20:36:35, Kevin Bailey wrote:
> > Preserving the cursor but scrolling to the left would be nice, but scrolling
> > right appears to be a Textfield side-effect from the append? I'm looking and
> > not seeing a trivial way to undo that.
> 
> Can we do something like:
> 
> SetCursorPosition(0);
> SetCursorPosition(old cursor position);
> 
> In which case I'd assume that the initial call would scroll left, then the
next
> call wouldn't change the scroll position as long as the old cursor position
was
> still onscreen?  (If it's not onscreen, then it would scroll, but that's
> unavoidable.)
> 
> Two other thoughts related to this scrolling stuff:
> * Maybe Textfield's automatic "scroll cursor into view" behavior should try to
> place the cursor in the middle of the region, or a third of the way from one
of
> the sides, instead of all the way at the edge.
> * Maybe we need APIs in Textfield to control the scroll position separately
from
> the cursor position.  This would be useful in the omnibox anyway, because
> there's a couple places that do things to adjust the cursor position which are
> really about scrolling.
> 
> These are changes I've thought made sense for some time, and I thought at one
> point I got some positive noises from msw (Textfield owner) about them.

This version homes the cursor, then replaces it. It doesn't require any changes
to the browser tests.

It doesn't look to me as though we have access to the Textfield and its
RenderText,
without some creative casting. Am I just missing it? If not, would it be worth
it plumbing that in another CL so we're not calling 'SetWindowText...' twice?

[Peter Kasting, 2017-05-15 23:51:23]

On 2017/05/15 23:33:30, Kevin Bailey wrote:
> It doesn't look to me as though we have access to the Textfield and its
> RenderText,
> without some creative casting. Am I just missing it? If not, would it be worth
> it plumbing that in another CL so we're not calling 'SetWindowText...' twice?

OmniboxEditModel doesn't know that the view is built on a Textfield, because
that's a views-ism, and wouldn't be true on e.g. Mac.

If you want to plumb something, you have to write an API in OmniboxView, then
you have to implement that in the OmniboxViewXYZ classes in terms of how the
underlying concrete controls behave.

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:374: if (has_focus() &&
user_input_was_in_progress) {
Do we even need this distinction anymore?  I'm wondering if we can use the code
in the } else { in all cases.

[Kevin Bailey, 2017-05-16 01:01:01]

ack, to the plumbing requirements

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:374: if (has_focus() &&
user_input_was_in_progress) {
On 2017/05/15 23:51:23, Peter Kasting wrote:
> Do we even need this distinction anymore?  I'm wondering if we can use the
code
> in the } else { in all cases.

I don't know all the cases that Revert() is called so I'm not confident saying,
'yes'. e.g. If permanent_text_ is shorter than where the user placed the cursor,
what happens? Perhaps we can still make this unconditional, but min() the two
numbers?

Other than this concern, I tried a lot of things and couldn't break it, so
conceivably it's a good strategy.

[Peter Kasting, 2017-05-16 05:53:54]

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:374: if (has_focus() &&
user_input_was_in_progress) {
On 2017/05/16 01:01:01, Kevin Bailey wrote:
> On 2017/05/15 23:51:23, Peter Kasting wrote:
> > Do we even need this distinction anymore?  I'm wondering if we can use the
> code
> > in the } else { in all cases.
> 
> I don't know all the cases that Revert() is called so I'm not confident
saying,
> 'yes'.

There are several, but not so many that you couldn't look through them all, I
think.  Might be good practice :)

> If permanent_text_ is shorter than where the user placed the cursor,
> what happens? Perhaps we can still make this unconditional, but min() the two
> numbers?

I'm pretty sure we need to do that even in the case where the user was editing
-- consider typing a whole bunch, then hitting escape.

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:381:
view_->SetWindowTextAndCaretPos(permanent_text_, start, false, true);
Even if you don't plumb full facilities for updating the scrolling separately
from the caret, it might be worth plumbing a better way to muck with the caret. 
The first call needs to update the text too, but the second doesn't, nor does it
need to do all the associated notifications.

[Kevin Bailey, 2017-05-17 01:53:54]

Ok, bots are finally happy with the latest browser test. (Preserving the cursor
required a tiny change.)

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:374: if (has_focus() &&
user_input_was_in_progress) {
On 2017/05/16 05:53:53, Peter Kasting wrote:
> 
> There are several, but not so many that you couldn't look through them all, I
> think.  Might be good practice :)

I said 'cases', not 'places'. :) I looked through all the RevertAll() calls;
they look reasonable.

> I'm pretty sure we need to do that even in the case where the user was editing
> -- consider typing a whole bunch, then hitting escape.

Done.

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:381:
view_->SetWindowTextAndCaretPos(permanent_text_, start, false, true);
On 2017/05/16 05:53:53, Peter Kasting wrote:
> Even if you don't plumb full facilities for updating the scrolling separately
> from the caret, it might be worth plumbing a better way to muck with the
caret. 

Ya, exposing SetCursorPosition() wouldn't be too hard. Separate CL ok?

> The first call needs to update the text too, but the second doesn't, nor does
it
> need to do all the associated notifications.

I removed the first notification.

[Peter Kasting, 2017-05-17 02:44:14]

LGTM with hope for more testing

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:381:
view_->SetWindowTextAndCaretPos(permanent_text_, start, false, true);
On 2017/05/17 01:53:54, Kevin Bailey wrote:
> On 2017/05/16 05:53:53, Peter Kasting wrote:
> > Even if you don't plumb full facilities for updating the scrolling
separately
> > from the caret, it might be worth plumbing a better way to muck with the
> caret. 
> 
> Ya, exposing SetCursorPosition() wouldn't be too hard. Separate CL ok?

Sure.

> > The first call needs to update the text too, but the second doesn't, nor
does
> it
> > need to do all the associated notifications.
> 
> I removed the first notification.

What I meant was, when making two SetWindowTextAndCaretPos() calls, the first
call may update the text and thus needs to make underlying notifications, but
the second call won't.

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:870: EXPECT_EQ(0U, end);
Maybe before RevertAll() we should explicitly move the cursor (e.g. to position
1) and check that that cursor position is preserved.  If not that, then there
should be some other similar test in this CL, which clearly tests that the
result of reverting is neither setting the cursor to start or end but preserving
it.

Perhaps there should also be something that verifies that when the new text is
shorter, the length gets clamped.

[Kevin Bailey, 2017-05-17 03:57:52]

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:381:
view_->SetWindowTextAndCaretPos(permanent_text_, start, false, true);
On 2017/05/17 02:44:14, Peter Kasting wrote:
> 
> What I meant was, when making two SetWindowTextAndCaretPos() calls, the first
> call may update the text and thus needs to make underlying notifications, but
> the second call won't.

I was thinking we wanted to notify when the cursor was correct, and not before.
I don't see where it compares the old and new text; it appears to
unconditionally notify (as long as the parameter is true.) So this is good, ya?

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:870: EXPECT_EQ(0U, end);
On 2017/05/17 02:44:14, Peter Kasting wrote:
> Maybe before RevertAll() we should explicitly move the cursor (e.g. to
position
> 1) and check that that cursor position is preserved.  If not that, then there
> should be some other similar test in this CL, which clearly tests that the
> result of reverting is neither setting the cursor to start or end but
preserving
> it.
> 
> Perhaps there should also be something that verifies that when the new text is
> shorter, the length gets clamped.

ack, good suggestions.

[Peter Kasting, 2017-05-17 03:59:21]

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2860503004/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:381:
view_->SetWindowTextAndCaretPos(permanent_text_, start, false, true);
On 2017/05/17 03:57:52, Kevin Bailey wrote:
> On 2017/05/17 02:44:14, Peter Kasting wrote:
> > 
> > What I meant was, when making two SetWindowTextAndCaretPos() calls, the
first
> > call may update the text and thus needs to make underlying notifications,
but
> > the second call won't.
> 
> I was thinking we wanted to notify when the cursor was correct, and not
before.
> I don't see where it compares the old and new text; it appears to
> unconditionally notify (as long as the parameter is true.) So this is good,
ya?

Correctnesswise it's fine.  My concern is that it does excessive work, and the
fix would presumably be the move-cursor-only API.

[Kevin Bailey, 2017-05-17 04:10:16]

The CQ bit was checked by krb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 04:11:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 07:33:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 07:33:31]

Dry run: This issue passed the CQ dry run.

[Kevin Bailey, 2017-05-17 13:28:52]

The CQ bit was checked by krb@chromium.org

[commit-bot: I haz the power, 2017-05-17 13:29:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 13:34:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495027732142080,
"parent_rev": "f4fc1f04fe1a0d835a508720fdd90841a8d1583b", "commit_rev":
"5a8c1da2cda8f195695bc226dacdd35964df4113"}

[commit-bot: I haz the power, 2017-05-17 13:34:17]

Description was changed from

==========
If text was appended to a URL, the omnibox was shifting it to show the new text.
This change makes it such that the text is only shifted if the user is editing.
Otherwise, it homes the cursor, showing the origin.

BUG=681740
==========

to

==========
If text was appended to a URL, the omnibox was shifting it to show the new text.
This change makes it such that the text is only shifted if the user is editing.
Otherwise, it homes the cursor, showing the origin.

BUG=681740

Review-Url: https://codereview.chromium.org/2860503004
Cr-Commit-Position: refs/heads/master@{#472440}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a8c1da2cda8f195695bc226dacd...
==========

[commit-bot: I haz the power, 2017-05-17 13:34:19]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5a8c1da2cda8f195695bc226dacd...

[Mark P, 2017-05-22 23:58:47]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2017-05-22 23:58:48]

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:870: EXPECT_EQ(0U, end);
On 2017/05/17 03:57:52, Kevin Bailey wrote:
> On 2017/05/17 02:44:14, Peter Kasting wrote:
> > Maybe before RevertAll() we should explicitly move the cursor (e.g. to
> position
> > 1) and check that that cursor position is preserved.  If not that, then
there
> > should be some other similar test in this CL, which clearly tests that the
> > result of reverting is neither setting the cursor to start or end but
> preserving
> > it.
> > 
> > Perhaps there should also be something that verifies that when the new text
is
> > shorter, the length gets clamped.
> 
> ack, good suggestions.

I don't see a change after this comment.  Did you decide not to do it?

[Kevin Bailey, 2017-05-23 00:03:02]

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:870: EXPECT_EQ(0U, end);
On 2017/05/22 23:58:48, Mark P wrote:
> On 2017/05/17 03:57:52, Kevin Bailey wrote:
> > On 2017/05/17 02:44:14, Peter Kasting wrote:
> > > Maybe before RevertAll() we should explicitly move the cursor (e.g. to
> > position
> > > 1) and check that that cursor position is preserved.  If not that, then
> there
> > > should be some other similar test in this CL, which clearly tests that the
> > > result of reverting is neither setting the cursor to start or end but
> > preserving
> > > it.
> > > 
> > > Perhaps there should also be something that verifies that when the new
text
> is
> > > shorter, the length gets clamped.
> > 
> > ack, good suggestions.
> 
> I don't see a change after this comment.  Did you decide not to do it?

https://codereview.chromium.org/2891653003/

[Mark P, 2017-05-23 03:59:59]

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/2860503004/diff/120001/chrome/browser/ui/omni...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:870: EXPECT_EQ(0U, end);
On 2017/05/23 00:03:02, Kevin Bailey wrote:
> On 2017/05/22 23:58:48, Mark P wrote:
> > On 2017/05/17 03:57:52, Kevin Bailey wrote:
> > > On 2017/05/17 02:44:14, Peter Kasting wrote:
> > > > Maybe before RevertAll() we should explicitly move the cursor (e.g. to
> > > position
> > > > 1) and check that that cursor position is preserved.  If not that, then
> > there
> > > > should be some other similar test in this CL, which clearly tests that
the
> > > > result of reverting is neither setting the cursor to start or end but
> > > preserving
> > > > it.
> > > > 
> > > > Perhaps there should also be something that verifies that when the new
> text
> > is
> > > > shorter, the length gets clamped.
> > > 
> > > ack, good suggestions.
> > 
> > I don't see a change after this comment.  Did you decide not to do it?
> 
> https://codereview.chromium.org/2891653003/

Awesome.  I love follow-through. :-)