New issue
Advanced search Search tips

Issue 852994 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 822003

Blocking:
issue 823535



Sign in to add a comment

MD Refresh: Omnibox Rounded Frame alignment is brittle.

Project Member Reported by tommycli@chromium.org, Jun 14 2018

Issue description

See the screenshot of the Actual vs. the Mock.

Looks like the suggestions start one pixel too low in Actual vs. the Mock.

It's a polish bug.
 
Actual.png
3.8 KB View Download
Mock.png
9.8 KB View Download

Comment 1 by manukh@chromium.org, Jun 15 2018

omnibox results frame inset.png
26.3 KB View Download
Labels: Proj-MdRefresh OS-Chrome OS-Linux OS-Mac OS-Windows
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 15 2018

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

commit bda25618bf3579f8271bbfbfb2e5754694dd58b0
Author: manuk <manukh@chromium.org>
Date: Fri Jun 15 18:27:42 2018

omnibox: decrease bottom inset of omnibox results frame for material design refresh by 1

Previously, the insets were 5 vertically and 6 horizontally. Now, the bottom inset has been decreased to 4 in order to align the bottom of the results frame's top section with the bottom of the location bar

Bug: 852994
Change-Id: If3a7d85c3edc3a8d055ff53d910c688ced9171ec
Reviewed-on: https://chromium-review.googlesource.com/1102661
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567736}
[modify] https://crrev.com/bda25618bf3579f8271bbfbfb2e5754694dd58b0/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.cc

Comment 4 by manukh@chromium.org, Jun 18 2018

Status: Fixed (was: Assigned)
Cc: pbos@chromium.org jdonnelly@chromium.org
Labels: -Pri-3 Pri-2
Status: Assigned (was: Fixed)
This seems to have regressed in my build.

It looks like it's possible the whole top portion of the Omnibox has shrunk 1dp in every dimension, but it would be good to verify against the screenshot in c#1 using an image manipulation program.

Either that or the toolbar expanded. pbos@ -- has the toolbar expanded recently?

We will want to fix this and merge it to 69.
Screenshot from 2018-07-20 10-44-32.png
40.7 KB View Download
Is there any chance that we could make this alignment less brittle, e.g. that it gets placed correctly based on the actual bottom border?

I only know that the toolbar has expanded height in Hindi on Windows, see  issue 862514 , as a side effect of making ToolbarButton inherit LabelButton. Not sure if that had other effects, LMK if you see any.
pbos: Oof. The frame is placed with respect to the Omnibox itself.

From your comment, the toolbar border has a variable distance from the bottom of the Omnibox itself?

How variable are we talking about? Do 95% of users have the same distance at least?
It looks like the top alignment is not correct now either.

Per pbos's feedback in c#6, perhaps the correct fix is to calculate the inset values based on the toolbar bounds rather than the location bar bounds.
I don't know how the location bar is placed vs the rest of the toolbar, the ToolbarButton height was pushed in Hindi as a side effect, it was not intentional and we need to fix  issue 862514  in one way or another before release.
Re #8, we also need to make sure the text doesn't shift around when bringing up the popup, so we need them to both be in alignment. Not sure what the easiest fix here is. I think we should first figure out what actually happened before spitballing a fix.
Re #10, the frame actually makes a hole to the underlying textfield, so I don't think we will have a problem with the textfield shifting.

But yes - I take your point, and agree that we should figure out what happened to break alignment in the first place.
Looking on mac, the top alignment is correct, the 1st suggestion is 1dp too high 
Screen Shot 2018-07-20 at 11.08.30 AM.png
179 KB View Download
It looks fine on Windows Canary with the default fonts -- and on Manuk's machine.

So likely it's affected by the font size setting on Linux (and potentially other platforms). Mine is set to 11pt.
Can you start chrome with --draw-view-bounds-rects and see if the ToolbarButtons are square or not? I think that should give a good hint for whether it's the ToolbarButton height being pushed or not.
On my windows machine, we have 4dp on top of the omnibox and 5dp on the bottom (With the rule line set inside). 

Ideally, we can have 4 on top and bottom. 
Screen Shot 2018-07-20 at 11.20.50 AM.png
283 KB View Download
Oof, dumb comment. I guess it's 5 on the bottom because the 1px rule line is inset. 

The rule line is actually .5px on my screenshot but that maybe due to the desire to keep rule lines 1px on 2x devices. I thought we just didn't scale up but I could be wrong. 
Labels: Group-Omnibox
pbos@
the original cl (june 15) for this issue set the insets to `5 (top), 6, 4, 6` and it aligned fine. For reasons which i haven't investigated, by June 28 the omnibox popup was higher than the toolbar by 1 px, so another CL changed the top inset from `5` to `4`. Now the omnibox popup is lower by 1 px, and reverting the 2nd CL, returning the top inset to `5` corrects the alignment.
See image; top is current behavior with 4px top inset; bottom is fixed and original behavior with 5px top inset.
I'll make a CL to revert the June 28 5->4px change. If you'd like, I can look for what changed between june 15 and 28 that may have caused the misalignment.
concat.png
78.8 KB View Download
Only for your info: On macOS 10.13.6 on a Non-Retina Display MacBook Air everything looks fine.

Tested with Chrome Canary Version 70.0.3508.0.

Two screenshot are attached.
Not_Zoomed.png
64.1 KB View Download
zoomed.png
20.2 KB View Download
It still looks fine on Windows Canary.
Windows.PNG
24.2 KB View Download
Summary: MD Refresh: Omnibox Rounded Frame alignment is brittle. (was: MD Refresh: Omnibox suggestions seem to be one pixel too high)
But the bottom bar is one px (half a dp) too high for Retina Mac.
Screen Shot 2018-07-31 at 10.55.25 AM.png
20.3 KB View Download
Labels: -M-69
In light of c#19 through c#21, we should probably leave the current insets alone --

unless we can fix Retina Mac without messing up Windows and non-Retina Mac. That would be worthwhile.

But it remains true that the alignment is overall brittle and varies between HiDPI, font sizes, and platforms.

A "real" fix would likely be to explicitly align the Omnibox frame to the toolbar via a more complicated calculation. That's also likely beyond scope for M69.
For Retina Mac, I'll point out that the border itself looks to be 1px thick, whereas everything else in our UI is specified in integer dp units. I'm not sure if that's intentional.

That by itself might make it impossible to line the bottom portion up beyond an accuracy of 1px.
Yes, it looks like the white space above and below the Omnibox has not the same vertical height on Retina Display. I have no Retina Display, but it is noticeable in the screenshot of C#21 when you measure the white space above and below the Reload button. The Toolbar bottom grey border looks 1px too low on Retina. Shifting it up by 1px on Retina would align it with the top of the first suggestion hover like on non-Retina and would make the white space on top/bottom consistent. If you want, I can open a new report for this issue. Just let me know.
Cc: bsep@chromium.org
Completely removing hairline strokes is issue 822003.
(E.g. there should no longer be any fixed 1px lines once that is resolved.)
Blockedon: 822003
Labels: Hotlist-Polish M-70
Okay great. We can revisit how this looks once 822003 is fixed. Maybe it'll be "just fine".

In any case this will be a polish thing for M70 and beyond. It's working "pretty well" for 69.
Labels: -Pri-2 Pri-3
Reducing the priority since it sounds like there may not be any more work required here.
Labels: -Proj-MdRefresh Proj-DesktopUI
Labels: Hotlist-DesktopUITriaged

Sign in to add a comment