Do not close the omnibox popup on mouseDown.

Do not close the omnibox popup on mouseDown.

This does not match Safari 6, Spotlight, or Chrome omnibox behavior on other
platforms.

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

[Jered, 2012-08-15 19:22:37]

Here is a fix for crbug.com/142887

[Jered, 2012-08-16 17:35:57]

On 2012/08/15 19:22:37, Jered wrote:
> Here is a fix for crbug.com/142887

Sorry, I see that rsesek@ has replied on the bug but shess@ hasn't, maybe they
are very busy? rsesek@ could you take a quick look at this?

[Robert Sesek, 2012-08-16 17:47:24]

Are there tests for this?

Also, Scott is the right person to review this. So while it's LG2M, you should
get his. Rohit may also be able to do a more full review.

[Scott Hess - ex-Googler, 2012-08-16 21:28:18]

AFAICT from poking around and trying to remember, this code probably dates from
a time when page actions and the like could exist in the omnibox while editing. 
In that case, we needed to drop the popup so that other popups from page actions
and the like wouldn't get in a fight with it.

Aha, git annotate rules.
http://crrev.com/52126
http://crbug.com/40145

So this was really from when decorations were handled differently.

I'm worried about the comment in the field.mm file which says "Also matches
other platforms".  Have you checked any of these alleged other platforms?  That
doesn't necessarily mean we shouldn't match Safari, but it might mean we should
ask around to see if there are likely to be subtle bugs stemming from such a
change breaking assumptions baked into other code.

If this goes forward, remove the -mouseDown: in the editor entirely.  Also,
experiment with clicks that are very close to the edges of the field.  The field
editor covers a subset of the field area, basically the easiest way to do this
is to move the mouse up or down until the I-beam cursor switches to an arrow. 
[Maybe just put some logging in to see the difference.]  Those are the cases
handled by the field's -mouseDown:.  I am not entirely convinced whether those
should leave the popup up or not, but I think I could go either way.

[Jered, 2012-08-16 21:34:57]

On 2012/08/16 21:28:18, shess wrote:
> AFAICT from poking around and trying to remember, this code probably dates
from
> a time when page actions and the like could exist in the omnibox while
editing. 
> In that case, we needed to drop the popup so that other popups from page
actions
> and the like wouldn't get in a fight with it.
> 
> Aha, git annotate rules.
> http://crrev.com/52126
> http://crbug.com/40145
> 
> So this was really from when decorations were handled differently.
> 
> I'm worried about the comment in the field.mm file which says "Also matches
> other platforms".  Have you checked any of these alleged other platforms?
Windows, Linux and ChromeOS do not close the omnibox on mouse down.
Android does so on tap.

> That
> doesn't necessarily mean we shouldn't match Safari, but it might mean we
should
> ask around to see if there are likely to be subtle bugs stemming from such a
> change breaking assumptions baked into other code.

Whom should we ask?
> If this goes forward, remove the -mouseDown: in the editor entirely.  Also,
> experiment with clicks that are very close to the edges of the field.  The
field
> editor covers a subset of the field area, basically the easiest way to do this
> is to move the mouse up or down until the I-beam cursor switches to an arrow. 
> [Maybe just put some logging in to see the difference.]  Those are the cases
> handled by the field's -mouseDown:.  I am not entirely convinced whether those
> should leave the popup up or not, but I think I could go either way.

Ok, I'll experiment.

[Scott Hess - ex-Googler, 2012-08-16 22:24:25]

On 2012/08/16 21:34:57, Jered wrote:
> On 2012/08/16 21:28:18, shess wrote:
> > AFAICT from poking around and trying to remember, this code probably dates
> from
> > a time when page actions and the like could exist in the omnibox while
> editing. 
> > In that case, we needed to drop the popup so that other popups from page
> actions
> > and the like wouldn't get in a fight with it.
> > 
> > Aha, git annotate rules.
> > http://crrev.com/52126
> > http://crbug.com/40145
> > 
> > So this was really from when decorations were handled differently.
> > 
> > I'm worried about the comment in the field.mm file which says "Also matches
> > other platforms".  Have you checked any of these alleged other platforms?
> Windows, Linux and ChromeOS do not close the omnibox on mouse down.
> Android does so on tap.

If Windows and Linux don't close on mouse down, no need to research further. 
Your change will bring OSX in line with other platforms.

> > If this goes forward, remove the -mouseDown: in the editor entirely.  Also,
> > experiment with clicks that are very close to the edges of the field.  The
> field
> > editor covers a subset of the field area, basically the easiest way to do
this
> > is to move the mouse up or down until the I-beam cursor switches to an
arrow. 
> > [Maybe just put some logging in to see the difference.]  Those are the cases
> > handled by the field's -mouseDown:.  I am not entirely convinced whether
those
> > should leave the popup up or not, but I think I could go either way.
> 
> Ok, I'll experiment.

I'm having troubles crafting a story where clicks which are in-field but not
in-editor should remove the popup.  The only decoration in the view, now, is the
search/document/whatever icon on the left, and it doesn't really do anything
when clicked so it wouldn't cause visual clutter to leave the field up.  So I
think removing the method from the editor, removing the close from the field
will do it.

[Jered, 2012-08-16 22:28:02]

On 2012/08/16 22:24:25, shess wrote:
> On 2012/08/16 21:34:57, Jered wrote:
> > On 2012/08/16 21:28:18, shess wrote:
> > > AFAICT from poking around and trying to remember, this code probably dates
> > from
> > > a time when page actions and the like could exist in the omnibox while
> > editing. 
> > > In that case, we needed to drop the popup so that other popups from page
> > actions
> > > and the like wouldn't get in a fight with it.
> > > 
> > > Aha, git annotate rules.
> > > http://crrev.com/52126
> > > http://crbug.com/40145
> > > 
> > > So this was really from when decorations were handled differently.
> > > 
> > > I'm worried about the comment in the field.mm file which says "Also
matches
> > > other platforms".  Have you checked any of these alleged other platforms?
> > Windows, Linux and ChromeOS do not close the omnibox on mouse down.
> > Android does so on tap.
> 
> If Windows and Linux don't close on mouse down, no need to research further. 
> Your change will bring OSX in line with other platforms.
> 
> > > If this goes forward, remove the -mouseDown: in the editor entirely. 
Also,
> > > experiment with clicks that are very close to the edges of the field.  The
> > field
> > > editor covers a subset of the field area, basically the easiest way to do
> this
> > > is to move the mouse up or down until the I-beam cursor switches to an
> arrow. 
> > > [Maybe just put some logging in to see the difference.]  Those are the
cases
> > > handled by the field's -mouseDown:.  I am not entirely convinced whether
> those
> > > should leave the popup up or not, but I think I could go either way.
> > 
> > Ok, I'll experiment.
> 
> I'm having troubles crafting a story where clicks which are in-field but not
> in-editor should remove the popup.  The only decoration in the view, now, is
the
> search/document/whatever icon on the left, and it doesn't really do anything
> when clicked so it wouldn't cause visual clutter to leave the field up.  So I
> think removing the method from the editor, removing the close from the field
> will do it.

Ok, done. I scrubbed around clicking in the field but not the editor, and
leaving
the popup open still felt reasonable to me.

[Scott Hess - ex-Googler, 2012-08-16 22:29:13]

lgtm

[Jered, 2012-08-16 22:38:30]

On 2012/08/16 22:29:13, shess wrote:
> lgtm

Thanks for reviewing. I looked through the unit tests for autocomplet editor &
field, and saw none referencing this behavior.

[commit-bot: I haz the power, 2012-08-17 15:50:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/10836270/6001

[commit-bot: I haz the power, 2012-08-17 22:25:45]

Change committed as 152155