Confirmed on Win Dev

+CC estade since he's worked with this code

Able to reproduce the issue on windows 7 using chrome version 58.0.3018.0 

Also, in the abbreviated state right before we entirely run out of room for this text:

I think in the old days this size was the width of the magnifying glass icon + appropriate padding, so you'd lose the text and keep the icon.  Not sure

Now we show the icon plus "S...", plus the divider, which isn't helpful at all.  And the divider doesn't have the correct padding in front of it, but is jammed up against the ellipsis.

We should probably just show the blue magnifying glass in this case.  The other option would be to show just the glass + search engine short name, but I doubt that's worth doing, since we're trying to use as little width as possible, and that's almost as long as the original thing.

Since both these issues are related, leaving this on here, but feel free to break out into a new bug if that doesn't make sense.

Using the per-revision bisect providing the bisect results,
Good build: 58.0.2992.0 (Revision: 445908).
Bad build: 58.0.2993.0 (Revision: 446204).

You are probably looking for a change made after 446198 (known good), but no later than 446199 (first known bad).

CHANGE-LOG URL:
---------------
https://chromium.googlesource.com/chromium/src/+log/22d3bc7d75a5f9180bca7860ee23c3f731607eda..39f5ad11373872bbb7e398fb7943b289cc790c39

From the CL above, assigning the issue to the concern owner

@estade: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url: https://codereview.chromium.org/2642893002

Note :Able to reproduce the issue in Ubuntu 14.04 & Win 10.0 and not in Mac 10.12.3.and able to reproduce in latest Canary #58.0.3018.0

Seems similar to the  bug 691917 .Hence merging this into 691917.
Please feel free to undupe if not similar.

Thanks,

The  issue 691917  fixed on today's dev 58.0.3018.0.But able to reproduce this issue.Hence reopening this issue.

I found the problem but it's not apparent to me what the best fix is.

The omnibox has 3dip insets on all sides because it's a Textfield. This means the item_edit_padding comes out to 1 - 3 = -2. In other words, the omnibox overlaps the leading decorations by 2dip, which is weird. When there is no leading decoration, the omnibox gets an x position of -1 (edge thickness - 2 = -1).

Options,
- don't use the default textfield insets. Change to 1.
- or, don't apply the item edit padding when there are no leading icons to pad against. See https://codereview.chromium.org/2710893002/

However neither of these solutions preserve the original behavior in terms of how much horizontal padding there is in the case of no leading decorations. How much do we care? Was the desired value for this ever spec'd? It's pretty rare that there's no leading decorations so it might have been overlooked. +maxwalker what should it look like when there's no leading decoration, i.e. how much padding between border and text?

Technically, if the omnibox is the first thing, we should make sure we're using the edge-to-first-item, rather than item-to-edit, padding value before subtracting the insets.  In this case, that will make no practical difference, since all these are the same.

I think the positioning as-is is correct.  I would either draw the border after the children, or at least after the omnibox, or else have the LocationBarView clip child drawing to be inside the border region.

maxwalker can you comment on intended behavior (see last paragraph of comment 8)

For Max' benefit, here is a mock of what would happen if the border-drawing bug were fixed and no other changes were made.

I think this is OK.  The other option is that we ensure the selected keyword view collapses down, at a minimum, to just the magnifying glass instead of nothing at all.  That would avoid the need to address the border issue.

Deleted: Untitled.png 193 KB

	Untitled.png 193 KB View Download

Thanks for bringing this up! After thinking about this more I find it problematic that we have a state with no leading decoration: there is no clear indication that a non-default search engine is active and that it can be exited/deleted via backspace or escape. We could consider a behavior that preserves the Search chip (to some degree) and is more inline with our treatment for the EV/Security chip.

Some more observations and thoughts on this:
- The minimum possible omnibox width I could get when shrinking the window is 114dp (https://goo.gl/Ipt1AI, is that correct?).
- The text padding on the right side of the omnibox is 6dp (https://goo.gl/2pfQFq).
- The padding on each side of the divider is 8dp.
- We should always preserve a minimum click target for the text/URL. 24dp would fit ~ 3 characters.
- A reasonable minimum width for the Search chip could be 40dp (tiny 114dp omnibox with space for 24dp for text).

I created a proposal and spec that shows what this would look like when resizing the window or entering text that exceeds the available space. See attachment. What do you think?

Alternatives (also attached):
- Icon only: the blue icon could be confused with the gray standard search icon and it's less clear that you can delete it with backspace.
- Center ellipsis (like EV chip): not ideal as it cuts the most important "Search" part of the string sooner.

As a quick fix Peter's suggestion seems fine to me.

Deleted: Non-Touch-Chip-Keyword-Search-Narrow-200.png 409 KB

	Non-Touch-Chip-Keyword-Search-Narrow-200.png 409 KB View Download

Deleted: Alternatives.png 44.2 KB

	Alternatives.png 44.2 KB View Download

Perhaps omitting the word "Search" is a good first step for shrinking the chip, after which we can start to elide the engine name, reserving at least enough space for ~4 non-elided characters.

re: click targeting, clicking the search chip currently does nothing. Can we make that just focus the omnibox (and select all text as if you clicked on the input)?

If we want/need a quick fix just to merge to m58, I think https://codereview.chromium.org/2710893002/ is sufficient and appealingly simple.

https://codereview.chromium.org/2713253002/ does what is described in #13. Here are some screen grabs.

Deleted: non-elided.png 11.1 KB

	non-elided.png 11.1 KB View Download

Deleted: elided.png 8.7 KB

	elided.png 8.7 KB View Download

Deleted: ultra-small.png 5.4 KB

	ultra-small.png 5.4 KB View Download

For reference, the intended minimum width of the editable textfield itself is 10 "average characters", plus insets.  There's some additional padding for the surrounding location bar as well.

I would suggest allowing the search chip to shrink all the way down to the blue magnifier (divider up to you) when the omnibox is this small -- to me it seems better to see 7 chars of input text than 2 chars plus "Goog...".  But I defer to Max.

maxwalker, wdyt?

maxwalker is ooo I guess. +ainslie, who can provide feedback on the right way to elide here?

hwi@'s the right one

Here's some feedback. 

re: Ultra small
- Showing an icon-only to allow a little more legibility during typing sounds reasonable to me.
- Omnibox can get even smaller with Home icon and extension icons (see the screenshot), and icon-only seems a better fit considering that such case exists. 
- With this, we're making a trade-off as Max pointed out regarding less clarity on the state with the icon-only(c#12).

re: Elided (screenshot on c14)
- Search engine name elided in the screenshot feels a bit unsatisfying to me
- Wondering if the search term's max-width/length can be adjusted to show full search engine name (probably up to x characters of a name, x=10?)

re: divider
- Probably no divider for icon-only as long as it keeps the existing padding(8dp) just to match the regular behavior with the gray magnifying glass

Deleted: min.png 20.4 KB

	min.png 20.4 KB View Download

- Wondering if the search term's max-width/length can be adjusted to show full search engine name (probably up to x characters of a name, x=10?)

That's what is happening, except x=4. x=10 in this case would just mean no elision for "Google" but we'd still have elision for, e.g. "monoprice.com" (which is the format we get when the search engine is automatically discovered). Are you saying that x=10 is simply more satisfying than x=4?

So it sounds like you're suggesting three states (instead of just 2). Those states would be:

{icon} Search Abcdefghijlkmnopqrs |
{icon} Abcdefghij... |
{icon}

Is that accurate?

Are you saying that x=10 is simply more satisfying than x=4? - Yes
Is that accurate? - Yes

okie dokie. Peter, this plan sgty?

The only concern I have is the technical implementation of having three elide states (full, elided, minimal).  I'm not sure how to do that easily in the current code, and I'm not sure it will end up buying very much over just the two states.  But I'm not opposed to it on principle if it's not a big time/complexity cost.

indeed, it's more work to have more than two states but I am optimistic it won't be too hard to be worth it.

yea, having precisely three discrete states is technically difficult. But here's something that's similar, easier, and has some advantages:

1 - Draw search label at full size when the omnibox is sufficiently wide
2 - Limit the search label to n% (n=50?) of the omnibox. Thus when the browser omnibox shrinks to less than twice the full size of the search label, the search label starts to shrink. It can then be any size between full and minimum sizes.
3 - At a certain point as it shrinks, change from "Search [enginename]" to just "[enginename]". In either case, the text may be elided. So you may see "Search ama..." and then after it shrinks a bit more, you get just "amazon" then "ama...".

The main advantage I see is that the appearance search label doesn't depend on the text you've typed. It's kind of weird that right now you can take up the entire omnibox with "Search amazon.com |" if you haven't typed anything, then as you start typing the amount of space for the entered text jumps.

I've attached some screen grabs.

Deleted: wqJQHGdqHRg.png 83.2 KB

	wqJQHGdqHRg.png 83.2 KB View Download

Deleted: KVgUe3pJU0r (1).png 33.2 KB

	KVgUe3pJU0r (1).png 33.2 KB View Download

I'm fine with this in general.  The one change I'd make is that as soon as there's not room for "Search <name>", I'd remove "Search" immediately, and after that begin eliding the name, instead of eliding the name, and then later removing "Search" and potentially restoring part of the name.

Yea, that would be a little better, but it's difficult to do because it still requires knowledge of a third state in between minimum and preferred.

I forgot to mention it, but one other thing I did here was to change the elision from an ellipsis character to a fade. Perhaps we could fade from the left when "Search" is still visible so it goes

"Search amazon"
"...arch amazon"
"amazon"
"amaz..."

I wouldn't do that.  I also would be wary of fade eliding here, as the rest of the omnibox (e.g. the dropdown) uses ellipses.

I'm not sure why any third state is needed here.  I'd think in the Layout() code we'd do something like:

  std::string display_string = "amazon";  // or whatever engine
  if (width() >= GetPreferredWidth())
    display_string = l10n_util::GetStringF(IDS_BLAH, display_string);
  ... put |display_string| in an auto-eliding label or something

bookmarks use fade eliding, but I don't feel strongly one way or another about that.

As I understand that code snippet, it doesn't work (that's actually the first thing I tried). Say you have either "Search amazon.com" or "amazon.com" and the amount of space you are allotted is somewhere in between. Then you can either show:

"Search amazo... |" (or some other kind of elision)

or

"amazon.com      |"

and the latter looks pretty bad.

During size calculation, we could repurpose "GetMinimumSize" to mean "GetADifferentAndSmallerPreferredSize", but still be willing to shrink to less than that, but it seems kind of hacky, and it also requires changes to LocationBarLayout which as you (Peter) know is pretty complicated. Touching that has a high chance of breaking something else in turn.

re: eliding indicator, 
- "..." seems a good fit since it's the indicator used for EV chip. 
- keeping ellipsis-*tail* throughout the state changes seems smooth, imo.

I see.  You're right that there's basically a third state in here.  Sad.

It's not trivial, but the way I'd consider addressing this is with an interface like:

class LocationBarLayoutParticipant {  // That's a terrible name
  virtual gfx::Size GetSmallerSize() {
    return this_as_view->GetPreferredSize();
  }

  View* this_as_view;  // Could also achieve this with templating
};

...and then override GetSmallerSize() for keyword views.

For English, it makes sense to elide like "...earch Amazon" at first until the pre-name part of the string is gone.  But what if some internationalized label is basically "ABC $1 DEF"?  It's not clear to me how the code should know where to elide something like this.

So my instinct would have been to suggestion the eliding you mention in comment 27, but I'm worried about i18n implications, and thinking we need to bite the bullet and deal with the three states directly.

(To be clear, that's my way of saying that I think if we just tail-elide, switching from "Search ama..." to "amazon.co..." is really bad and should be avoided)

why is that switch really bad? It's really rare that you would actually see that switch happen. It only happens as you resize the browser window. How often do you start an omnibox search then start resizing the window?

It would also happen as you start typing more, no?  That certainly causes the selected keyword view to shrink in size today.

(I ask because the i18n motivation, which is most likely something like "Amazon.com Search" becoming "Amazon.com Sea..." becoming "Amaz..." would actually work better than in English.)

see comment 25 for detailed answer to that --- in short, no, what you type has no impact in this world.

So when does the "minimum size, magnifying glass only" state come into play?  Ever?  Does the text here ever shrink at all when you type more text than can fit in the current omnibox?

> So when does the "minimum size, magnifying glass only" state come into play? 
 Ever?

nope.

> Does the text here ever shrink at all when you type more text than can fit in the current omnibox?

no, you get the same amount of space for typing text no matter what you type or don't type (N% of omnibox, or omnibox - preferred width of search label, whichever is greater). I can send you a patch to test it out. I think this feels nicer in practice than what we have. The text will scroll if you run out of room, just as it does currently, although the "run out of room" point will be different.

OK.  Let's go with it.

Adding the following for a record.

Updated position on c#30, re: "..." vs fading, 

- Old: "..." seems a good fit since it's the indicator used for EV chip.

- Updated after discussing with estade@: 
Given the small display space, tail-fading effect (shown on c#25) looks much simpler and better than tail eliding with "..."

- Side note: This adds a difference from EV chip middle eliding that uses "..." and from Views url string tail treatment(i.e. solid cut, no effect) in the omnibox. We can revisit in the future for any necessary unification. The fading implementation to fix this bug is a useful reference point then.

bug 701856 filed for ev cert follow up

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9

commit cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9
Author: estade <estade@chromium.org>
Date: Wed Mar 15 23:12:39 2017

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}

[modify] https://crrev.com/cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9/chrome/browser/ui/cocoa/location_bar/selected_keyword_decoration.mm
[delete] https://crrev.com/93c3717d732bdb7b6dcb8e549d94e55fc952c146/chrome/browser/ui/location_bar/location_bar_util.cc
[delete] https://crrev.com/93c3717d732bdb7b6dcb8e549d94e55fc952c146/chrome/browser/ui/location_bar/location_bar_util.h
[modify] https://crrev.com/cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
[modify] https://crrev.com/cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9/chrome/browser/ui/views/location_bar/selected_keyword_view.cc

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc437f4143b27bb3fb511a7d20d88188c5fd1f57

commit fc437f4143b27bb3fb511a7d20d88188c5fd1f57
Author: Evan Stade <estade@chromium.org>
Date: Fri Mar 17 14:49:32 2017

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}
(cherry picked from commit cdedb4788d9ead1bdb685cfa1e765ae15fcd7ea9)

Review-Url: https://codereview.chromium.org/2748103013 .
Cr-Commit-Position: refs/branch-heads/3029@{#262}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/fc437f4143b27bb3fb511a7d20d88188c5fd1f57/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/fc437f4143b27bb3fb511a7d20d88188c5fd1f57/chrome/browser/ui/cocoa/location_bar/selected_keyword_decoration.mm
[delete] https://crrev.com/111a01efebf7e49dbe8bbd3c2b4b7c187aefd7b1/chrome/browser/ui/location_bar/location_bar_util.cc
[delete] https://crrev.com/111a01efebf7e49dbe8bbd3c2b4b7c187aefd7b1/chrome/browser/ui/location_bar/location_bar_util.h
[modify] https://crrev.com/fc437f4143b27bb3fb511a7d20d88188c5fd1f57/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
[modify] https://crrev.com/fc437f4143b27bb3fb511a7d20d88188c5fd1f57/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/fc437f4143b27bb3fb511a7d20d88188c5fd1f57/chrome/browser/ui/views/location_bar/selected_keyword_view.cc

Verified the issue on windows 10 and Ubuntu 14.04 using chrome beta version #58.0.3029.33 as per comment #0

Observed that Omnibox border did not miss when long name was given after pressing Ctrl+e. Hence, the fix is working as expected.

Attaching screen cast for reference.

Hence, adding the verified labels.

Thanks...!!

Deleted: 694152.mp4 1.2 MB

	694152.mp4 1.2 MB View Download