Adjust elision of omnibox keyword search label.

Adjust elision of omnibox keyword search label.

The search label will be limited to 50% of the omnibox width.
The minimum width will be just the search icon, although it's unlikely that
the omnibox will ever be that small on most platforms.

See bug for extended discussion.

BUG=694152
Review-Url: https://codereview.chromium.org/2731113002
Cr-Commit-Position: refs/heads/master@{#457255}
Committed: https://chromium.googlesource.com/chromium/src/+/cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9

[Evan Stade, 2017-03-07 01:19:53]

Description was changed from

==========
wip

BUG=694152
==========

to

==========
Make elision of omnibox keyword search label match EV label.

The search label will be limited to 50% of the omnibox width.
See bug for extended discussion.

BUG=694152
==========

[Evan Stade, 2017-03-07 01:19:53]

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

[Evan Stade, 2017-03-07 01:19:58]

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

[Evan Stade, 2017-03-07 01:20:14]

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:121: 0, width() -
label_x - bubble_trailing_padding - kSpaceBesideSeparator);
if you don't do this then the ellipsis will touch the separator. It wasn't a bug
without the elision because we effectively gave the label slightly more space
than needed, which it didn't use and left blank.

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:512: const double
kMaxBubbleFraction = 0.5;
I didn't even notice till just now this is the same thing we're doing for the EV
bubble.

[commit-bot: I haz the power, 2017-03-07 01:21:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2017-03-07 02:28:45]

LGTM

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:512: const double
kMaxBubbleFraction = 0.5;
On 2017/03/07 01:20:14, Evan Stade wrote:
> I didn't even notice till just now this is the same thing we're doing for the
EV
> bubble.

Honestly, we should probably apply these limits to any security text
("secure"/"insecure" as well, i.e. it should probably be done for all the
leading decoration options.

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:491: const double
kMaxBubbleFraction = 0.5;
Nit: Now that we've moved away from "bubble" presentations, what if we called
this kMaxLeadingDecorationFraction?  See also my comment about applying it to
all leading decorations.

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:26:
set_can_process_events_within_subtree(false);
Don't do this (it looks like an artifact of the "prevent clicks on this" idea)

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:62: }
Nit: Shorter:

  const int partial_label_width = partial_label_.GetPreferredSize().width();
  const bool show_full_label =
      width() > GetSizeForLabelWidth(partial_label_width).width();
  SetLabel((show_full_label ? full_label_ : partial_label_).text());

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:86:
partial_label_.SetText(location_bar_util::CalculateMinString(short_name));
This lost the protection the old code had against the min string being empty,
which seems like it could happen if the search engine short name begins with
whitespace or a period.  I would imagine that's worth preserving (and maybe
unittesting)

[commit-bot: I haz the power, 2017-03-07 03:45:05]

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

[commit-bot: I haz the power, 2017-03-07 03:45:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Evan Stade, 2017-03-07 18:04:44]

seems I broke this when "cleaning it up" for review. Sorry about that.

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (left):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:52: gfx::Size
SelectedKeywordView::GetMinimumSize() const {
I mis-spoke before, this actually is necessary and the implementation should be

return GetSizeForLabelWidth(0).width();

because the decoration will indeed refuse to go smaller than this even if it has
to take up >50% of the omnibox to do so. Of course the omnibox would have to be
teeny weeny for this to occur.

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:86:
partial_label_.SetText(location_bar_util::CalculateMinString(short_name));
On 2017/03/07 02:28:45, Peter Kasting wrote:
> This lost the protection the old code had against the min string being empty,
> which seems like it could happen if the search engine short name begins with
> whitespace or a period.  I would imagine that's worth preserving (and maybe
> unittesting)

hmmm, this is even better if I just remove CalculateMinString. The Label view
will already be doing elision so there's no need. (And in fact we can get rid of
the helper if Mac is also updated.)

One thing I just noticed from testing without the fade elision is that we can
get into a state where the label is nothing but an ellipsis. With fade elision,
that can't happen as you can at least see a partially opaque "ama" (for the
amazon example). I don't know how to fix this state. So, to open this can of
worms again, we can...

1) use fade tail always. Also, make the label extend all the way to the
separator. This actually gives us more room, e.g. you would see

[icon] Search Amazon.com| 

but the .com would be fading out. With ellipsis, we see something like

[icon] Search Amazo...  |

because it looks bad if we don't leave that space beside the ellipsis. This
effectively doubles the unusable space.

2) use fade tail only when the label is really small. Compare

[icon] Amazo|

(again with fading text) to

[icon] ...  |

3) like #2, but truncating instead of fading. We'd get

[icon] Am   |

[Peter Kasting, 2017-03-08 03:13:12]

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):

https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:86:
partial_label_.SetText(location_bar_util::CalculateMinString(short_name));
On 2017/03/07 18:04:44, Evan Stade wrote:
> On 2017/03/07 02:28:45, Peter Kasting wrote:
> > This lost the protection the old code had against the min string being
empty,
> > which seems like it could happen if the search engine short name begins with
> > whitespace or a period.  I would imagine that's worth preserving (and maybe
> > unittesting)
> 
> hmmm, this is even better if I just remove CalculateMinString. The Label view
> will already be doing elision so there's no need. (And in fact we can get rid
of
> the helper if Mac is also updated.)
> 
> One thing I just noticed from testing without the fade elision is that we can
> get into a state where the label is nothing but an ellipsis. With fade
elision,
> that can't happen as you can at least see a partially opaque "ama" (for the
> amazon example). I don't know how to fix this state.

Set the "minimum size" to the required width for several label characters?  Then
we can't shrink all the way down that small.  Actually I'm kind of surprised
that the omnibox min width is small enough we can get that small at all.

> 1) use fade tail always. Also, make the label extend all the way to the
> separator. This actually gives us more room, e.g. you would see
> 
> [icon] Search Amazon.com| 
> 
> but the .com would be fading out. With ellipsis, we see something like
> 
> [icon] Search Amazo...  |
> 
> because it looks bad if we don't leave that space beside the ellipsis. This
> effectively doubles the unusable space.

I reserve judgment until I see a screenshot.

> 2) use fade tail only when the label is really small. Compare
> 
> [icon] Amazo|
> 
> (again with fading text) to
> 
> [icon] ...  |
> 
> 3) like #2, but truncating instead of fading. We'd get
> 
> [icon] Am   |

Not a fan of these two.  (3) kinda feels broken.  If we were willing to do (2),
I'd just do (1), or some variant thereof.

https://codereview.chromium.org/2731113002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (left):

https://codereview.chromium.org/2731113002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/selected_keyword_view.cc:83:
location_bar_util::CalculateMinString(short_name));
With the removal of this call, I think only the Mac version of this is calling
this method.

It would be nice, but probably not necessary, to fix Mac in a similar way to
views with this CL.  If you'd rather not do that, can we hoist
CalculateMinString up to the Mac-specific code, so that when MacViews lands and
that goes away, this method will automatically die too?

[Evan Stade, 2017-03-08 18:32:16]

On 2017/03/08 03:13:12, Peter Kasting wrote:
>
https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/location_bar/selected_keyword_view.cc (right):
> 
>
https://codereview.chromium.org/2731113002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/location_bar/selected_keyword_view.cc:86:
> partial_label_.SetText(location_bar_util::CalculateMinString(short_name));
> On 2017/03/07 18:04:44, Evan Stade wrote:
> > On 2017/03/07 02:28:45, Peter Kasting wrote:
> > > This lost the protection the old code had against the min string being
> empty,
> > > which seems like it could happen if the search engine short name begins
with
> > > whitespace or a period.  I would imagine that's worth preserving (and
maybe
> > > unittesting)
> > 
> > hmmm, this is even better if I just remove CalculateMinString. The Label
view
> > will already be doing elision so there's no need. (And in fact we can get
rid
> of
> > the helper if Mac is also updated.)
> > 
> > One thing I just noticed from testing without the fade elision is that we
can
> > get into a state where the label is nothing but an ellipsis. With fade
> elision,
> > that can't happen as you can at least see a partially opaque "ama" (for the
> > amazon example). I don't know how to fix this state.
> 
> Set the "minimum size" to the required width for several label characters? 
Then
> we can't shrink all the way down that small.  Actually I'm kind of surprised
> that the omnibox min width is small enough we can get that small at all.
> 
> > 1) use fade tail always. Also, make the label extend all the way to the
> > separator. This actually gives us more room, e.g. you would see
> > 
> > [icon] Search Amazon.com| 
> > 
> > but the .com would be fading out. With ellipsis, we see something like
> > 
> > [icon] Search Amazo...  |
> > 
> > because it looks bad if we don't leave that space beside the ellipsis. This
> > effectively doubles the unusable space.
> 
> I reserve judgment until I see a screenshot.

Is this the screenshot you want?
https://bugs.chromium.org/p/chromium/issues/detail?id=694152#c25
Or are you asking to see the ellipsis with no space beside it?

[Evan Stade, 2017-03-08 19:13:27]

it doesn't look like this is going to land all that soon and I'm about to be
gone for a little under a week. Should we craft a minimal patch to fix the
visual glitch on the bug and merge that, then worry about this more elaborate
solution?

[Peter Kasting, 2017-03-08 22:00:15]

On 2017/03/08 18:32:16, Evan Stade wrote:
> On 2017/03/08 03:13:12, Peter Kasting wrote:
> > I reserve judgment until I see a screenshot.
> 
> Is this the screenshot you want?
> https://bugs.chromium.org/p/chromium/issues/detail?id=694152#c25
> Or are you asking to see the ellipsis with no space beside it?

It wasn't clear to me that that screenshot implemented the "extend the label all
the way to the separator" part.  That's the thing that makes fade eliding
noticeably better, but also the thing that worries me.

On 2017/03/08 19:13:27, Evan Stade wrote:
> it doesn't look like this is going to land all that soon and I'm about to be
> gone for a little under a week. Should we craft a minimal patch to fix the
> visual glitch on the bug and merge that, then worry about this more elaborate
> solution?

You have a standing LGTM from me here to move forward with whatever you think is
the right path.  I'm throwing in opinions since you asked, but I'm also fine
with landing whatever and fixing whatever as a followup.  I trust you :)

[Evan Stade, 2017-03-15 17:26:18]

Description was changed from

==========
Make elision of omnibox keyword search label match EV label.

The search label will be limited to 50% of the omnibox width.
See bug for extended discussion.

BUG=694152
==========

to

==========
Adjust elision of omnibox keyword search label.

The search label will be limited to 50% of the omnibox width.
The minimum width will be just the search icon, although it's unlikely that
the omnibox will ever be that small on most platforms.

See bug for extended discussion.

BUG=694152
==========

[Evan Stade, 2017-03-15 17:55:20]

On 2017/03/08 22:00:15, Peter Kasting wrote:
> 
> On 2017/03/08 19:13:27, Evan Stade wrote:
> > it doesn't look like this is going to land all that soon and I'm about to be
> > gone for a little under a week. Should we craft a minimal patch to fix the
> > visual glitch on the bug and merge that, then worry about this more
elaborate
> > solution?
> 
> You have a standing LGTM from me here to move forward with whatever you think
is
> the right path.  I'm throwing in opinions since you asked, but I'm also fine
> with landing whatever and fixing whatever as a followup.  I trust you :)

thanks. Wasn't sure, since the patch keeps changing. I talked to Hwi and she
updated the bug. We'll go with tail fading for now.

[Evan Stade, 2017-03-15 18:02:28]

estade@chromium.org changed reviewers:
+ tapted@chromium.org

[Evan Stade, 2017-03-15 18:02:29]

+tapted for c/b/ui/cocoa

[Evan Stade, 2017-03-15 18:26:32]

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

[commit-bot: I haz the power, 2017-03-15 18:27:15]

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-03-15 19:34:54]

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

[commit-bot: I haz the power, 2017-03-15 19:34:55]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-03-15 22:33:40]

On 2017/03/15 18:02:29, Evan Stade wrote:
> +tapted for c/b/ui/cocoa

lgtm

[Evan Stade, 2017-03-15 22:53:59]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-03-15 22:53:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2731113002/#ps80001
(title: "move helper to mac only code")

[commit-bot: I haz the power, 2017-03-15 22:54:52]

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-03-15 23:12:43]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489618439190910,
"parent_rev": "93c3717d732bdb7b6dcb8e549d94e55fc952c146", "commit_rev":
"cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9"}

[commit-bot: I haz the power, 2017-03-15 23:13:26]

Description was changed from

==========
Adjust elision of omnibox keyword search label.

The search label will be limited to 50% of the omnibox width.
The minimum width will be just the search icon, although it's unlikely that
the omnibox will ever be that small on most platforms.

See bug for extended discussion.

BUG=694152
==========

to

==========
Adjust elision of omnibox keyword search label.

The search label will be limited to 50% of the omnibox width.
The minimum width will be just the search icon, although it's unlikely that
the omnibox will ever be that small on most platforms.

See bug for extended discussion.

BUG=694152

Review-Url: https://codereview.chromium.org/2731113002
Cr-Commit-Position: refs/heads/master@{#457255}
Committed:
https://chromium.googlesource.com/chromium/src/+/cdedb4788d9ead1bdb685cfa1e76...
==========

[commit-bot: I haz the power, 2017-03-15 23:13:27]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cdedb4788d9ead1bdb685cfa1e76...