Feels more like a P3.

 Issue 866192  has been merged into this issue.

fyi: This happens for the Lock icon hover button only when the divider is visible. This means when the flag #omnibox-ui-jog-textfield-on-popup is "disabled".

mehmet@
i assume this 1px reduction should affect the unlabeled location icon (1st screenshot) as well as the labeled icon (2nd screenshot)?
screenshots, top: original, middle: 1 pixel thinner, bottom: difference highlighted in red

Deleted: icon.png 86.9 KB

	icon.png 86.9 KB View Download

Deleted: labeled icon.png 118 KB

	labeled icon.png 118 KB View Download

Hi manukh@

Yes, correct. The 1px reduction should affect the unlabeled as well as the labeled icon. 

Both screenshots looks good to me :)

Thank you very much in advance.

In addition to my last comment for you information: The 1px too far right of the hover button of the "labeled" icon is probably a result of the regression which I reported a while ago (issue 848421). I noticed there, that the divider between the labeled icons and the Omnibox text moved 1px to the left. Therefor the hover button overlaps the divider by 1px in the labeled cases too.

After thinking again about it, maybe it would be better not to fix it for the "labeled" case, because fixing the regression in issue 848421 would probably fix the 1px overlapping for the labeled cases when the divider moves again by 1px to the right to center it?

WDYT?

mehmet@, i think ur right in that the 2 issues r related; though I don't think that should prevent me from adjusting the labeled icon while i'm on this issue. The fixes for the two issues won't necessarily be additive; i.e., changing it now won't cause it to be over-adjusted to the left after the other fix given that my changes doesn't actually move the separator; it just shrinks the hover icon. Previously, the hover icon was drawn to include the separator width + 1 pixel beyond the separator, whereas my change is to remove that 1 extra pixel (see before + after screenshots attache).
Plus it's slightly simpler to apply my change for all icons with separators, rather than just unlabeled icons with separators. 

Deleted: before.png 29.0 KB

	before.png 29.0 KB View Download

Deleted: after.png 29.0 KB

	after.png 29.0 KB View Download

manukh@: Thanks for the update and explanation. Okay, looking forward to see this issue fixed soon :)

Thanks in advance.

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

commit fda94f7ffb0c082347f32c7cd978108fb514c303
Author: manuk <manukh@chromium.org>
Date: Wed Aug 29 19:32:59 2018

Decrease width of location icon by 1 px when separator is shown.

The location bar displays a separator between the location icon and the omnibox text when either the location icon has an attached label, or when omnibox text jog is disabled. Previously, the location icon was 1 px too wide (33 px without a label instead of 32 px) when the separator was shown. This cl decreases the location icon width by 1 px when a separator is shown, and increases the text padding by 1 px in order to retain alignment between the omnibox and results popup texts. This change applies to location icons with and without a label. This change only applies with material refresh.

Bug: 865523
Change-Id: I5205a9ee81af6e33bbcb79637c69dd7444fc9b09
Reviewed-on: https://chromium-review.googlesource.com/1188886
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587244}
[modify] https://crrev.com/fda94f7ffb0c082347f32c7cd978108fb514c303/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
[modify] https://crrev.com/fda94f7ffb0c082347f32c7cd978108fb514c303/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h
[modify] https://crrev.com/fda94f7ffb0c082347f32c7cd978108fb514c303/chrome/browser/ui/views/location_bar/selected_keyword_view.cc

Thanks manukh@. I‘ll verify it in Canary at the weekend. 

Tested the issue on chrome version# 70.0.3534.4(build without fix) and on latest chrome# 70.0.3537.0. Didn't find any difference in width in the Lock Icon when hovered on it.

@manuk: Please find the attached screenshot of build with and without fix and help us in verifying the fix.

Thanks!

Deleted: 865523.png 26.9 KB

	865523.png 26.9 KB View Download

@c#18, the size of that hover button is correct (32px).
Have you disabled the `#omnibox-ui-jog-textfield-on-popup` flag?
To reproduce the issue, u will need to use a location icon with a separator. Disabling the flag `#omnibox-ui-jog-textfield-on-popup` is the clearest way to show the separator and see the difference. It should be 33px without the fix, and 32px with the fix.

Hello manukh@: Thanks again for fixing this issue. I checked latest Canary and I can verify that the size of the hover button of the unlabeled icon is reduced by 1px at the right side (before 33px - now 32px). Also the hover button of the labeled icon is reduced by 1px as you explained in your comments above.

Unfortunately I noticed a small regression with your change: The separator also moved by 1px to the left :(

Correct me if I am wrong but the intention of the fix was only to reduce the size of the hover buttons by 1px; the position of the separators should not change, so that the right edges of the unlabeled and labeled hover button is nicely aligned with the separator after the fix, correct? Can you please take a look again? Thanks in advance :)

I thought the separator was aligned according to the right edge of the icon, so I left it the same distance from the end of the icon. That is, by decreasing the icon width by 1px, I also moved the separator left 1px.

I'll go ahead and adjust the separator 1px right.

Thanks manukh@ for looking into the issue again.

Is it possible to set the separator exactly aligned at the edge of the hover button? In the screenshot it looks like the separator is 1px to the right of the edge of the button. Thanks :)

mehmet@, at least on my linux desktop, it is currently (master branch & screenshot below) exactly aligned to the edge of the hover button. I thought c#20 meant to move it 1px right as in c#23.

maybe the results are platform specific. can you confirm you see the same alignment in master as the screenshot below?

Deleted: display.png 25.7 KB

	display.png 25.7 KB View Download

Hi manukh@: In your screenshot in c#25 it looks perfect as expected. I think this will look on OSX perfect as well. Unfortunately I am not a developer and can't build a master (if you mean with master a build to test it?).

Yes, by master, i meant building on the master branch, but i think canary should look like master as the change was merged 6 days ago.

So if what you're seeing on canary doesn't look like c#25, maybe there is 1px difference between the os's. I'll check what canary looks like on windows.

Okay, thanks for the clarification. On my Mac it is 1px to the left. Two screenshots are attached. I should mention that I am using a Non-Retina MacBook Air. Maybe on a Retina Mac it is looking perfect as in C#25. Unfortunately I have no Retina Mac to check it :(

Deleted: Mac_Non_Retina_1.png 21.1 KB

	Mac_Non_Retina_1.png 21.1 KB View Download

Deleted: Mac_Non_Retina_2.png 33.1 KB

	Mac_Non_Retina_2.png 33.1 KB View Download

windows canary screenshot below for reference

Deleted: display.png 15.7 KB

	display.png 15.7 KB View Download

So, on Windows it looks perfect too. 

Do you have a chance to test it on a Mac Retina? If it is looking perfectly aligned there too, I think then you can ignore the Non-Retina issue and close this report as fixed :)

Removing Needs-Feedback; feedback was provided.

Seems correct on my Retina Mac.

This is on Canary Mac 71.0.3543.0

Deleted: out.png 36.2 KB

	out.png 36.2 KB View Download

Deleted: Screen Shot 2018-09-05 at 9.40.54 AM.png 263 KB

	Screen Shot 2018-09-05 at 9.40.54 AM.png 263 KB View Download

Deleted: Screen Shot 2018-09-05 at 9.40.40 AM.png 263 KB

	Screen Shot 2018-09-05 at 9.40.40 AM.png 263 KB View Download

Thank you very much, tommycli@. Yes it looks correct on Retina.

So, let us ignore the non-retina issue and close this issue then as fixed.

WDYT?

Thanks :)