pkasting: any hints you can provide as to why this is happening on Windows but not Linux?

(just trying to help): It looks to me like one is using the Baseline of the font and the other is using the decent (or font height). This has been a problem (inconsistency) area between platforms in the past, and the symptoms look the same. I'm just looking at the picture though, so this may be off base.

I don't know about the Windows-vs.-Linux difference, but it looks like the issue is that the beginning of PaintMatch() now offsets |y| by GetVerticalMargin(), when it didn't use to.

I suspect both the old code and new code are wrong, if their desire is to center the vertical ascent within the line (as the omnibox textfield does).  The old code just happens to get closer because the vertical ascent itself is not centered within the font's height; this is also why if you select the text in the omnibox, the selection is not vertically centered.  If you moved the text down so you centered that selection, you'd probably get the same alignment in the omnibox as you're seeing in the screenshot above.

I don't have time at the moment to find where/how the omnibox is doing this and tell you how to fix it, but probably the code in the dropdown needs to do the same.

My change is not meant to have produced any visible different for 1-line suggestions. So if the old code was wrong, the new code should be wrong in the same way.

> it looks like the issue is that the beginning of PaintMatch() now offsets |y|
> by GetVerticalMargin(), when it didn't use to.

What it used to do instead to produce the same effect is draw the text in a display rect that was larger than the height of the font by the equivalent of 2*GetVerticalMargin() and relying on RenderText to draw the text vertically centered. My change increases y by GetVerticalMargin() but sets the rectangle to the height of the text.

On Linux, this yields the same result. On Windows, it doesn't. I can only guess as to why.

Probably because of how vertical text centering is implemented.  We try to vertically center the font's "main ascent" (the height of e.g. capital A) within the provided box, if it's large enough.  This looks visibly centered but is sometimes physically not centered, because e.g. fonts may have additional space above this area for diacritics.

This is why when you select the omnibox text, the selection bounds (which match the font's actual line height) look like they're not centered, at least on Windows.

I suspect on Linux either we don't have the right metrics for this, so we return the full font height as the ascent; or the UI font you're using is more centered-looking within its line height than the Windows one.

Ok, thanks for the info. I'm currently wrestling with getting a working Windows build. Hopefully I'll have that straightened out soon and can make some progress on this.

pkasting: if you know answers of the top of your head for my questions below, great. If not, I'll keep digging, I just want to document my understanding so far.

Here's what is happening:

1. Within the selection area (the area that has a different background color when selected, as in the omnibox text field), the vertical ascent is centered (as you described earlier)*. As a result, the text often seems below center.

2. When the RenderText is laid out in a display rect that's taller than the text and the selection area, it positions the text slightly above center (*on Windows only*). This is the source of the bug. I assumed that if I reduced the height of the display rect by 2x and increased y by x, no change would occur. On Linux, this is true, but not on Windows. Is this slightly above center positioning a bug? An intentional hedge against the behavior from point 1?

More importantly, what's the solution? I can easily fix the issue with an OS_WIN-only adjustment to y but that's obviously a hack. Are any of the things I describe above bugs that should be fixed? Is the Linux behavior a bug that should be fixed? (I hope not because it's much more intuitive that the vertically-centered text is actually exactly vertically centered.)

* Actually, it doesn't really look centered. Even just looking at the ascent, it seems below center (see attached screenshot). Is this a bug?

Deleted: Capture.PNG 5.2 KB

	Capture.PNG 5.2 KB View Download

(1) is not correct.  The vertical ascent is not centered within the selection (as is visible in the screenshot); the selection is merely from the bottom of the descent to the top of the internal leading.  Rather, I think what you're seeing is: when a textfield is sized taller than the font (that is, taller than that selection box), the font's position within that entire height is adjusted to try and vertically center the ascent.  This is what causes the selection in your screenshot to be above-center for the omnibox.  This is also why you think "it doesn't really look centered": you're evaluating ascent-within-selection (which is indeed below center) when you should be evaluating ascent-within-textfield (which has been counterbalanced to be correct).

That in turn is what you're seeing in (2): the textfield aligning the font so it appears vertically centered.  I would like to see a screenshot of the omnibox, with selection, on Linux, to evaluate what's happening there.  If it's not being moved at all, this is either because the font metrics are such that no movement is necessary (e.g. if descent == internal leading), or because on Linux we can't get the right metrics (which I know has been true in the past), or because on Linux this isn't implemented (which is a bug).  But whatever the cause, Linux would be the outlier, and Windows is correct.

Some possible routes forward:

* Continue to give text rows sufficient height to vertically center themselves
* Implement baseline-alignment between the selection icon and the top text row
* Read the height data out of the font and adjust manually (basically, the callsite-specific version of the previous bullet)

Ok, please ignore everything I said in Comment 8. pkasting's explanations are correct, though there's a key piece of unstated information that was preventing me from understanding what was going on. (It maybe should have been obvious from the available information but I had to read the RenderText code and experiment before I could see it.)

RenderText, as pkasting describes, attempts to vertically center the ascent (technically, the cap height) in the available space. But in the case where the font height equals the height of the display rect it simply can't make this adjustment or elements of the glyphs could be clipped by the display rect.

Making the height of the display rect the height of the font is exactly what I did in http://crrev.com/467038. So the normal adjustment upwards in order to center the cap height is no longer taking place and the text appears lower than it used to. (On Linux this doesn't happen because, again as pkasting speculated, the font isn't reporting the cap height so RenderText didn't previously and still doesn't now make this adjustment.)

I think there's a simple solution (CL inbound): just provide the display rect with some extra padding and remove that same amount of padding from the spacing I'm applying during layout above and below suggestions as well as the smaller middle spacing used in 2-line suggestions. This will allow RenderText to make the correct adjustments on Windows and will have no visible effect on Linux.

Fix landed a few days ago: https://codereview.chromium.org/2936533004/

Verified the fix on Win Canary 61.0.3135.0.

This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

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

Verified this issue using latest chrome version #61.0.3135.4 on Mac 10.12.4, Win 10 and Ubuntu 14.04 as well.

Observed now the suggestion list is centre aligned with the search icon in Omnibox.

Please find the screen cast.

Thanks!!

Deleted: Issue.mp4 356 KB

	Issue.mp4 356 KB View Download

I'd prefer to wait until M61 as we're pretty far into the release and the impact of this bug seems low for the size of the patch (~40 line delta).  If you feel strongly this should be included feel free to add the Merge-Review-60 label back for re-review.