[OriginChip] Show the URL on Esc key pressed

[OriginChip] Show the URL on Esc key pressed

This change makes it so hitting the Esc while the omnibox has focus shows the URL unless search term replacement is being performed, in which case we revert to the original search terms.

This holds for both the hide-on-mouse-release and hide-on-input options.  For the latter, this means that a click in the omnibox will also hide the origin chip.  This supports the case where the user is trying to get at the URL and forgets to click on the origin chip or misses it due to existing muscle memory.  In this case, an Esc will show the URL as users are accustomed to.

In no case will hitting Esc cause the origin chip to reappear.

BUG=349497
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260173

[Justin Donnelly, 2014-03-27 13:47:36]

https://codereview.chromium.org/214613003/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/214613003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:507:
controller_->GetToolbarModel()->set_url_replacement_enabled(true);
I'm not sure what this was trying to accomplish.  I think maybe to make it so
that after editing a URL the query terms would come back on Esc?

If so, the new behavior seems more consistent to me (edit a URL and Esc brings
back the URL (because url_replacement_enabled is still false) edit query terms
and Esc brings back the query terms.  And this call was making the origin chip
reappear on Esc in some cases (because WouldOmitURLDueToOriginChip depends on
the state of url_replacement_enabled).

https://codereview.chromium.org/214613003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:521:
controller()->GetToolbarModel()->set_origin_chip_enabled(!in_progress);
I don't think there was ever any good reason why I did this as opposed to the
"if in_progress then set_enabled(false)" approach.  The latter (as implemented
now) works fine because focus loss always restores the chip.

https://codereview.chromium.org/214613003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:989: view_->RevertAll();
As with my first note above, resetting search term replacement makes the origin
chip reappear which we don't want.

[Justin Donnelly, 2014-03-27 16:09:17]

Rachel, Marc-Antoine - can one of you test this on Mac when you get a chance?

[Peter Kasting, 2014-03-27 17:46:54]

The behavior you've implemented here is precisely what Greg and I described in
the meeting as "modal" and said we didn't want.

In particular, if you click an empty omnibox, start typing, and hit escape, we
should show the URL, not nothing.

[Justin Donnelly, 2014-03-27 17:58:49]

On 2014/03/27 17:46:54, Peter Kasting wrote:
> The behavior you've implemented here is precisely what Greg and I described in
> the meeting as "modal" and said we didn't want.
> 
> In particular, if you click an empty omnibox, start typing, and hit escape, we
> should show the URL, not nothing.

I understood that we considered modal behavior undesirable and ideally would
avoid it. But we've already got modal behavior based on whether you click on the
chip or the omnibox and whether you're on a search results page or not.  The
behavior here is consistent (always returning you to the state you were in
immediately after the chip went away) and simple to implement given the existing
modality.

If we don't do it the way I've done it here, wouldn't we need to be consistent
and also show the URL if you were editing search terms and then hit escape? 
Aside from being more complex to implement, that seems like an unpleasant user
experience.

[groby-ooo-7-16, 2014-03-27 18:00:52]

This doesn't enable the origin chip if input is not in progress any more, i.e.
on focus loss. (Postponing testing though, since it sounds like Peter disagrees
w/ solution)

In general, I'd prefer if we can define the desired show/hide behaviors around
_all_ cases and implement tests. At this point, it feels like we're having a
rather brittle solution which makes it easy to introduce regressions.

[Peter Kasting, 2014-03-27 18:05:06]

On 2014/03/27 17:58:49, Justin Donnelly wrote:
> If we don't do it the way I've done it here, wouldn't we need to be consistent
> and also show the URL if you were editing search terms and then hit escape?

No.
 
> Aside from being more complex to implement, that seems like an unpleasant user
> experience.

Hence "no" above.

"Consistency" is rarely a good target to shoot for, because usually by being
more consistent in one way you're less consistent with another.  For example,
the solution you propose makes post-escape state more consistent with pre-escape
state, but my solution makes post-escape state when focusing the omnibox via
chip click more consistent with post-escape state when focusing the omnibox via
omnibox click.  And that latter consistency is what both Greg and I thought more
valuable.  You're correct that making us display the URL even in the search term
case would be even more consistent along this vector, but I agree that it would
come at an overall UX cost.  Which is all another way of saying "trying to
consider 'consistency' in general is just going to be problematic".

[Justin Donnelly1, 2014-03-27 18:19:58]

To clarify, should Esc show the URL immediately after the user clicked in
the Omnibox or only after they start editing?

I don't think it's worth a big debate, so I'll do as you're suggesting.
 But it really seems like a non-ideal experience to me.  User expectations
are a matter of opinion, obviously, but I think clicking into the omnibox,
having it be empty and then Esc suddenly surfacing a URL will be surprising
to users.  As opposed to bringing them back to the state they were most
recently in.


On Thu, Mar 27, 2014 at 11:05 AM, <pkasting@chromium.org> wrote:

> On 2014/03/27 17:58:49, Justin Donnelly wrote:
>
>> If we don't do it the way I've done it here, wouldn't we need to be
>> consistent
>> and also show the URL if you were editing search terms and then hit
>> escape?
>>
>
> No.
>
>
>  Aside from being more complex to implement, that seems like an unpleasant
>> user
>> experience.
>>
>
> Hence "no" above.
>
> "Consistency" is rarely a good target to shoot for, because usually by
> being
> more consistent in one way you're less consistent with another.  For
> example,
> the solution you propose makes post-escape state more consistent with
> pre-escape
> state, but my solution makes post-escape state when focusing the omnibox
> via
> chip click more consistent with post-escape state when focusing the
> omnibox via
> omnibox click.  And that latter consistency is what both Greg and I
> thought more
> valuable.  You're correct that making us display the URL even in the
> search term
> case would be even more consistent along this vector, but I agree that it
> would
> come at an overall UX cost.  Which is all another way of saying "trying to
> consider 'consistency' in general is just going to be problematic".
>
> https://codereview.chromium.org/214613003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2014-03-27 18:23:49]

On 2014/03/27 18:19:58, jdonnelly_google.com wrote:
> To clarify, should Esc show the URL immediately after the user clicked in
> the Omnibox or only after they start editing?

Ideally the former; that's the only way it will serve the use case for which I
originally proposed this behavior.

> I don't think it's worth a big debate, so I'll do as you're suggesting.
>  But it really seems like a non-ideal experience to me.  User expectations
> are a matter of opinion, obviously, but I think clicking into the omnibox,
> having it be empty and then Esc suddenly surfacing a URL will be surprising
> to users.  As opposed to bringing them back to the state they were most
> recently in.

I have been wrong before :).  I'm certainly not opposed to changing the behavior
if after we implement this and use it it turns out it feels terrible.  That's
basically what happened with doing hide-on-edit on all pages, which was also
something I originally wanted.

[Justin Donnelly1, 2014-03-27 18:26:35]

On Thu, Mar 27, 2014 at 11:23 AM, <pkasting@chromium.org> wrote:

> I have been wrong before :)


I'm never wrong, so the odds are in my favor. ;)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Donnelly, 2014-03-27 20:42:42]

On 2014/03/27 18:26:35, jdonnelly_google.com wrote:
> I'm never wrong, so the odds are in my favor. ;)

Well, that didn't last long.  I think there's still some merit in my approach,
but now that I've used the non-modal approach I can see the value in it.

Peter, please take another look.

[Justin Donnelly, 2014-03-27 21:00:56]

On 2014/03/27 18:00:52, groby wrote:
> This doesn't enable the origin chip if input is not in progress any more, i.e.
> on focus loss.

Rachel - I've implemented Peter's solution, so please test again when you get a
chance.  Let me know if you want to walk through it together.  It seems like
OnKillFocus -> OnDidKillFocus should still be restoring the chip but I must have
missed something.

> In general, I'd prefer if we can define the desired show/hide behaviors around
> _all_ cases and implement tests. At this point, it feels like we're having a
> rather brittle solution which makes it easy to introduce regressions.

Can't disagree with that.  I've promised it before, but I really think I'll be
able to set aside a good chunk of time next week for some proper unit tests.  If
not, I think I can at least promise to hack at a few tests to get the ball
rolling.

[Peter Kasting, 2014-03-27 21:10:09]

https://codereview.chromium.org/214613003/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/214613003/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:971: UpdatePermanentText();
How come we need to call this?

https://codereview.chromium.org/214613003/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:974: return true;
I think this skips the histogram below when we don't want to.

Maybe instead we need to do something like:

  bool should_disable_url_replacement =
      controller_->GetToolbarModel()->url_replacement_enabled() &&
      !controller_->GetToolbarModel()->WouldPerformSearchTermReplacement(true);
  ...
  if (!has_zero_suggest_match && !should_disable_url_replacement &&
      !user_input_in_progress_ && view_->IsSelectAll())
    return false;
  ...
  if (should_disable_url_replacement)
    controller_->GetToolbarModel()->set_url_replacement_enabled(false);
  view_->RevertWithoutResettingSearchTermReplacement();
  ...

[groby-ooo-7-16, 2014-03-27 21:32:47]

Seems OK on OSX - any use of ESC gets previous URL/search back, and focus loss
is indeed handled

One weird edge case:
1) Search for a search term
2) Display URL (click on chip or cmd-l)
3) Click into URL, without editing. Just place the cursor
4) Hit ESC -> get URL, selected. No origin chip
5) Hit ESC a second time, get search terms without origin chip

Step 5 is _only_ possible when step 3 occurs. Yay state! ;)

[Justin Donnelly, 2014-03-27 21:52:16]

https://codereview.chromium.org/214613003/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/214613003/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:971: UpdatePermanentText();
On 2014/03/27 21:10:09, Peter Kasting wrote:
> How come we need to call this?

UpdatePermanentText is what calls ToolbarModelImpl::GetText() to determine what
the text should be given the new state.  This is the same as what OmniboxView
does to show and hide the URL.

https://codereview.chromium.org/214613003/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:974: return true;
On 2014/03/27 21:10:09, Peter Kasting wrote:
> I think this skips the histogram below when we don't want to.
> 
> Maybe instead we need to do something like:
> 
>   bool should_disable_url_replacement =
>       controller_->GetToolbarModel()->url_replacement_enabled() &&
>      
!controller_->GetToolbarModel()->WouldPerformSearchTermReplacement(true);
>   ...
>   if (!has_zero_suggest_match && !should_disable_url_replacement &&
>       !user_input_in_progress_ && view_->IsSelectAll())
>     return false;
>   ...
>   if (should_disable_url_replacement)
>     controller_->GetToolbarModel()->set_url_replacement_enabled(false);
>   view_->RevertWithoutResettingSearchTermReplacement();
>   ...

Done.

[Peter Kasting, 2014-03-27 21:53:28]

On 2014/03/27 21:32:47, groby wrote:
> Seems OK on OSX - any use of ESC gets previous URL/search back, and focus loss
> is indeed handled
> 
> One weird edge case:
> 1) Search for a search term
> 2) Display URL (click on chip or cmd-l)
> 3) Click into URL, without editing. Just place the cursor
> 4) Hit ESC -> get URL, selected. No origin chip
> 5) Hit ESC a second time, get search terms without origin chip

That seems wrong -- I'd expect to either get (4) (for
non-search-term-replacement cases) or (5), but not one and then the other.

[groby-ooo-7-16, 2014-03-27 21:55:21]

On 2014/03/27 21:53:28, Peter Kasting wrote:
> On 2014/03/27 21:32:47, groby wrote:
> > Seems OK on OSX - any use of ESC gets previous URL/search back, and focus
loss
> > is indeed handled
> > 
> > One weird edge case:
> > 1) Search for a search term
> > 2) Display URL (click on chip or cmd-l)
> > 3) Click into URL, without editing. Just place the cursor
> > 4) Hit ESC -> get URL, selected. No origin chip
> > 5) Hit ESC a second time, get search terms without origin chip
> 
> That seems wrong -- I'd expect to either get (4) (for
> non-search-term-replacement cases) or (5), but not one and then the other.

The wrongest part being that it only happens when you position the caret. No
idea why that'd be.

[Justin Donnelly, 2014-03-27 21:58:43]

On 2014/03/27 21:55:21, groby wrote:
> On 2014/03/27 21:53:28, Peter Kasting wrote:
> > On 2014/03/27 21:32:47, groby wrote:
> > > Seems OK on OSX - any use of ESC gets previous URL/search back, and focus
> loss
> > > is indeed handled
> > > 
> > > One weird edge case:
> > > 1) Search for a search term
> > > 2) Display URL (click on chip or cmd-l)
> > > 3) Click into URL, without editing. Just place the cursor
> > > 4) Hit ESC -> get URL, selected. No origin chip
> > > 5) Hit ESC a second time, get search terms without origin chip
> > 
> > That seems wrong -- I'd expect to either get (4) (for
> > non-search-term-replacement cases) or (5), but not one and then the other.
> 
> The wrongest part being that it only happens when you position the caret. No
> idea why that'd be.

Well, for extra fun, I don't see this on Views.  Following that sequence of
steps, (5) has no visible effect.  I tried with both hide-on-mouse-release and
hide-on-user-input.

[Peter Kasting, 2014-03-27 22:04:12]

LGTM.  I don't know why Mac is acting weird.  I blame it on Mac being Mac.

https://codereview.chromium.org/214613003/diff/40001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/214613003/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:967: // terms.
Nit: How about this comment:

When using the origin chip, hitting escape to revert all should either display
the URL (when search term replacement would not be performed for this page) or
the search terms (when it would).  To accomplish this, we'll need to disable URL
replacement iff it's currently enabled and search term replacement wouldn't
normally happen.

[Justin Donnelly, 2014-03-27 22:10:07]

https://codereview.chromium.org/214613003/diff/40001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/214613003/diff/40001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:967: // terms.
On 2014/03/27 22:04:12, Peter Kasting wrote:
> Nit: How about this comment:
> 
> When using the origin chip, hitting escape to revert all should either display
> the URL (when search term replacement would not be performed for this page) or
> the search terms (when it would).  To accomplish this, we'll need to disable
URL
> replacement iff it's currently enabled and search term replacement wouldn't
> normally happen.

Done.

[Justin Donnelly, 2014-03-27 22:10:11]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:11:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/214613003/60001

[commit-bot: I haz the power, 2014-03-28 02:29:45]

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

[commit-bot: I haz the power, 2014-03-28 02:29:45]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Justin Donnelly, 2014-03-28 02:48:46]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-28 02:49:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/214613003/60001

[commit-bot: I haz the power, 2014-03-28 03:12:47]

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

[commit-bot: I haz the power, 2014-03-28 03:12:48]

Retried try job too often on ios_dbg_simulator for step(s) url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[Justin Donnelly, 2014-03-28 03:40:14]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-28 03:41:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/214613003/60001

[commit-bot: I haz the power, 2014-03-28 04:37:16]

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

[commit-bot: I haz the power, 2014-03-28 04:37:17]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[Justin Donnelly, 2014-03-28 04:42:44]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-28 04:44:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/214613003/60001

[commit-bot: I haz the power, 2014-03-28 05:52:29]

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

[commit-bot: I haz the power, 2014-03-28 05:52:30]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Justin Donnelly, 2014-03-28 13:55:23]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-28 13:57:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/214613003/70001

[commit-bot: I haz the power, 2014-03-28 15:16:48]

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

[commit-bot: I haz the power, 2014-03-28 15:16:48]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Justin Donnelly, 2014-03-28 15:21:18]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-03-28 15:24:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/214613003/70001

[commit-bot: I haz the power, 2014-03-28 17:02:01]

Change committed as 260173