MD Refresh: Omnibox Rounded Frame alignment is brittle. |
|||||||||||
Issue descriptionSee 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.
,
Jun 15 2018
,
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
,
Jun 18 2018
,
Jul 20
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.
,
Jul 20
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.
,
Jul 20
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?
,
Jul 20
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.
,
Jul 20
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.
,
Jul 20
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.
,
Jul 20
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.
,
Jul 20
Looking on mac, the top alignment is correct, the 1st suggestion is 1dp too high
,
Jul 20
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.
,
Jul 20
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.
,
Jul 20
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.
,
Jul 20
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.
,
Jul 24
,
Jul 31
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.
,
Jul 31
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.
,
Jul 31
It still looks fine on Windows Canary.
,
Jul 31
But the bottom bar is one px (half a dp) too high for Retina Mac.
,
Jul 31
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.
,
Jul 31
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.
,
Jul 31
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.
,
Jul 31
,
Jul 31
(E.g. there should no longer be any fixed 1px lines once that is resolved.)
,
Aug 8
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.
,
Aug 21
Reducing the priority since it sounds like there may not be any more work required here.
,
Sep 20
,
Sep 26
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by manukh@chromium.org
, Jun 15 201826.3 KB
26.3 KB View Download