Small Omnibox Popups get their Elevations Clamped below UX spec |
||
Issue descriptionWe recently implemented the Omnibox popup shadows including the "clobber" state where no results are displayed. We are running into a problem where the shadows rendered for small height Omnibox popups are smaller than the mock, despite elevation being set at 16. See SizeReducedElevation.png compared to Mocks.png. When the Omnibox is large, shadows appear as expected. See FullShadow.png. The root cause is that shadow.cc clamps the max elevation for small windows, by design here: https://cs.chromium.org/chromium/src/ui/compositor_extra/shadow.cc?dr=CSs&g=0&l=112 This was introduced on purpose in this CL: https://codereview.chromium.org/2586653002 I have a few questions: 1) bklmn@ - Do we care? I'm guessing we do and want it to look like the mocks? 2) estade@ - Can you give some additional color as to why we need to clamp the elevation for small windows? Reading the original CL, it looks like it's to prevent excess redraws / perf reasons. Does it makes sense for us to pass an additional flag through Widget::InitParams that essentially says params.draw_full_shadow_for_small_windows?
,
May 24 2018
Hey Tommy. Yes, ideally if we can get this to spec, it would be ideal. LMK how this progresses.
,
May 24 2018
Hey Tommy. Yes, ideally if we can get this to spec, it would be ideal. LMK how this progresses.
,
May 25 2018
I also noticed that the MD shadows in the code base here: https://cs.chromium.org/chromium/src/ui/gfx/shadow_value.cc?sq=package:chromium&g=0&l=74 do not match the specs posted in go/chrome-ux-GM2. I added a comment in that presentation Slide 25. The gist is that I'm looking for a comprehensive table of elevation => offset, blur, spread values, so I can put that in the code. Or ideally -- a mathematical formula. I assume we want to update those shadows Chrome and ChromeOS-wide to match the new specs right?
,
Jun 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a2fa3276b5ef5c0ba7a562e8448de95bfcba51e5 commit a2fa3276b5ef5c0ba7a562e8448de95bfcba51e5 Author: Tommy C. Li <tommycli@chromium.org> Date: Tue Jun 19 16:40:22 2018 Omnibox UI Refresh: Make 16 elevation shadow that matches specs This makes the 16 elevation shadow for the Omnibox match the specs given for MD Refresh. This CL also divorces the Omnibox shadows from the Aura window shadow drawing logic. Bug: 845305 , 823535 Change-Id: Ibbda26b465b528805a1be591a23a20cc09df7643 Reviewed-on: https://chromium-review.googlesource.com/1100006 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Tommy Li <tommycli@chromium.org> Cr-Commit-Position: refs/heads/master@{#568489} [modify] https://crrev.com/a2fa3276b5ef5c0ba7a562e8448de95bfcba51e5/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc [modify] https://crrev.com/a2fa3276b5ef5c0ba7a562e8448de95bfcba51e5/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view_browsertest.cc [modify] https://crrev.com/a2fa3276b5ef5c0ba7a562e8448de95bfcba51e5/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.cc [modify] https://crrev.com/a2fa3276b5ef5c0ba7a562e8448de95bfcba51e5/chrome/browser/ui/views/omnibox/rounded_omnibox_results_frame.h [modify] https://crrev.com/a2fa3276b5ef5c0ba7a562e8448de95bfcba51e5/ui/gfx/shadow_value.cc
,
Jun 19 2018
That last CL should have fixed this. |
||
►
Sign in to add a comment |
||
Comment 1 by est...@chromium.org
, May 22 2018