Add a top shadow to material design omnibox dropdown

Add a top shadow to material design omnibox dropdown

Use a tiled asset to give the top edge of
the omnibox dropdown a shadow in
material design instead of rendering
a solid separator line.

BUG=520679
TEST=manual

Committed: https://crrev.com/2eccc8354d41e0da85d88af97f85d1ad21179d9b
Cr-Commit-Position: refs/heads/master@{#346393}

[tdanderson, 2015-08-27 14:36:55]

tdanderson@chromium.org changed reviewers:
+ pkasting@chromium.org

[tdanderson, 2015-08-27 14:36:55]

Peter, can you please take a look?

[Peter Kasting, 2015-08-27 19:05:04]

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:228: // the location
bar.
I think you mean "toolbar" rather than "location bar" here.

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:203:
top_edge_overlap += top_shadow_->height();
This looks wrong from the way you write the code; I think the material value
ought to just be the top shadow height, and not add in the client edge
thickness, since we're not drawing both a shadow and a client edge.

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h (right):

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h:159: const
gfx::ImageSkia* bottom_shadow_;  // Ptr owned by resource bundle.
Nit: Since there are now two of these, just write "These pointers are owned by
the resource bundle." in a comment above the two.

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h:160: const
gfx::ImageSkia* top_shadow_;     // Ptr owned by resource bundle.
Tiny nit: Reverse these (and in the .cc file) since we logically think about
things top-to-bottom.

[tdanderson, 2015-08-27 19:47:21]

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:228: // the location
bar.
On 2015/08/27 19:05:04, Peter Kasting wrote:
> I think you mean "toolbar" rather than "location bar" here.

Done.

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:203:
top_edge_overlap += top_shadow_->height();
On 2015/08/27 19:05:04, Peter Kasting wrote:
> This looks wrong from the way you write the code; I think the material value
> ought to just be the top shadow height, and not add in the client edge
> thickness, since we're not drawing both a shadow and a client edge.

If the material value is just the top shadow height (1) then the tiled shadow
asset will be rendered directly on top of the content separator (which means the
separator still visible). Adding the extra overlap makes the top of the popup
occlude the separator.

If your comment was instead about my wording/structure instead of the
correctness, how would you suggest I clarify?

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h (right):

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h:159: const
gfx::ImageSkia* bottom_shadow_;  // Ptr owned by resource bundle.
On 2015/08/27 19:05:04, Peter Kasting wrote:
> Nit: Since there are now two of these, just write "These pointers are owned by
> the resource bundle." in a comment above the two.

Done.

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h:160: const
gfx::ImageSkia* top_shadow_;     // Ptr owned by resource bundle.
On 2015/08/27 19:05:04, Peter Kasting wrote:
> Tiny nit: Reverse these (and in the .cc file) since we logically think about
> things top-to-bottom.

Done.

[Peter Kasting, 2015-08-27 19:54:10]

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:203:
top_edge_overlap += top_shadow_->height();
On 2015/08/27 19:47:20, tdanderson wrote:
> On 2015/08/27 19:05:04, Peter Kasting wrote:
> > This looks wrong from the way you write the code; I think the material value
> > ought to just be the top shadow height, and not add in the client edge
> > thickness, since we're not drawing both a shadow and a client edge.
> 
> If the material value is just the top shadow height (1) then the tiled shadow
> asset will be rendered directly on top of the content separator (which means
the
> separator still visible). Adding the extra overlap makes the top of the popup
> occlude the separator.

I looked at your screenshots and I can't see what you're trying to do clearly. 
It looks as if you're still rendering something very much like a content
separator.

The problem with your patch as implemented is that I think it will make the
bottom of the toolbar appear to shift up by 1 px when the dropdown is open.  If
you want to use an asset that looks like the separator line, then instead of
this patch as-is you're going to need to change things so the toolbar bottom
separator isn't drawn when you're drawing your shadow, so they don't overlap.

Unless I'm mistaken about how all this works.

[tdanderson, 2015-08-28 15:21:33]

On 2015/08/27 19:54:10, Peter Kasting wrote:
>
https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
> File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):
> 
>
https://codereview.chromium.org/1319983002/diff/1/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:203:
> top_edge_overlap += top_shadow_->height();
> On 2015/08/27 19:47:20, tdanderson wrote:
> > On 2015/08/27 19:05:04, Peter Kasting wrote:
> > > This looks wrong from the way you write the code; I think the material
value
> > > ought to just be the top shadow height, and not add in the client edge
> > > thickness, since we're not drawing both a shadow and a client edge.
> > 
> > If the material value is just the top shadow height (1) then the tiled
shadow
> > asset will be rendered directly on top of the content separator (which means
> the
> > separator still visible). Adding the extra overlap makes the top of the
popup
> > occlude the separator.
> 
> I looked at your screenshots and I can't see what you're trying to do clearly.

> It looks as if you're still rendering something very much like a content
> separator.
> 
> The problem with your patch as implemented is that I think it will make the
> bottom of the toolbar appear to shift up by 1 px when the dropdown is open. 
If
> you want to use an asset that looks like the separator line, then instead of
> this patch as-is you're going to need to change things so the toolbar bottom
> separator isn't drawn when you're drawing your shadow, so they don't overlap.
> 
> Unless I'm mistaken about how all this works.

I posted a side-by-side screenshot in crbug.com/520679, comment #13.
The intent is for the asset to look like a shadow cast onto the
toolbar instead of a solid separator line. For that reason I
interpreted what you see in the screenshot as being the right
thing to do. Let me know what you think / we'll see what the
designers say.

[Peter Kasting, 2015-08-29 01:00:23]

LGTM

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:497: // client edge.
Let's remove this comment from this file, as it basically is about why the
overlap has the value it has, which is something this callee doesn't need or
care to know.  This info belongs more properly in the caller; I'll give thoughts
there about how to convey this.

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:200: // shadow
asset.
OK, so if we move the comment in the other file out, I would replace the comment
here with something like the following, that gives the info that used to be in
that other comment and expands on it a bit:

"In non-material mode, we want the popup to appear is if it's overlaying the top
of the page content, i.e. is flush against the client edge at the bottom of the
toolbar.  However, if the bookmark bar is attached, we want to draw over it (so
as not to push the results below it), but that means the toolbar won't be
drawing a client edge separating itself from the popup.  So we unconditionally
overlap the toolbar by the thickness of the client edge and draw our own edge
(see OnPaint()), which fixes the attached bookmark bar case without breaking the
other case."

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:202: if
(ui::MaterialDesignController::IsModeMaterial())
...Then above this line, I'd add a further comment:

"In material mode, we cover the bookmark bar similarly, but instead of appearing
below the client edge, we want the popup to appear to overlay the bottom of the
toolbar.  So instead of drawing a client edge atop the popup, we shift the popup
to completely cover the client edge, and then draw an additional semitransparent
shadow above that.  So the total overlap necessary is the client edge thickness
plus the shadow height."

[tdanderson, 2015-08-31 15:23:19]

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:497: // client edge.
On 2015/08/29 01:00:23, Peter Kasting wrote:
> Let's remove this comment from this file, as it basically is about why the
> overlap has the value it has, which is something this callee doesn't need or
> care to know.  This info belongs more properly in the caller; I'll give
thoughts
> there about how to convey this.

Done.

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:200: // shadow
asset.
On 2015/08/29 01:00:23, Peter Kasting wrote:
> OK, so if we move the comment in the other file out, I would replace the
comment
> here with something like the following, that gives the info that used to be in
> that other comment and expands on it a bit:
> 
> "In non-material mode, we want the popup to appear is if it's overlaying the
top
> of the page content, i.e. is flush against the client edge at the bottom of
the
> toolbar.  However, if the bookmark bar is attached, we want to draw over it
(so
> as not to push the results below it), but that means the toolbar won't be
> drawing a client edge separating itself from the popup.  So we unconditionally
> overlap the toolbar by the thickness of the client edge and draw our own edge
> (see OnPaint()), which fixes the attached bookmark bar case without breaking
the
> other case."

Done.

https://codereview.chromium.org/1319983002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:202: if
(ui::MaterialDesignController::IsModeMaterial())
On 2015/08/29 01:00:23, Peter Kasting wrote:
> ...Then above this line, I'd add a further comment:
> 
> "In material mode, we cover the bookmark bar similarly, but instead of
appearing
> below the client edge, we want the popup to appear to overlay the bottom of
the
> toolbar.  So instead of drawing a client edge atop the popup, we shift the
popup
> to completely cover the client edge, and then draw an additional
semitransparent
> shadow above that.  So the total overlap necessary is the client edge
thickness
> plus the shadow height."

Done.

[tdanderson, 2015-08-31 15:23:40]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2015-08-31 15:23:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1319983002/#ps40001
(title: "new comments")

[commit-bot: I haz the power, 2015-08-31 15:23:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1319983002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1319983002/40001

[commit-bot: I haz the power, 2015-08-31 15:53:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-31 15:54:32]

Patchset 3 (id:??) landed as
https://crrev.com/2eccc8354d41e0da85d88af97f85d1ad21179d9b
Cr-Commit-Position: refs/heads/master@{#346393}