Update Location Bar Bubbles Layout for Material Design

Update Location Bar Bubbles Layout for Material Design

Update the layout of IconLabelBubbleView in order to match the specs for material design.

TEST=manual testing on device, compared screenshots with spec
BUG=523352
Committed: https://crrev.com/662ed852e0f73f9676d3c1aedee5f144a00e6581
Cr-Commit-Position: refs/heads/master@{#352697}

[jonross, 2015-09-30 22:21:38]

Patchset #1 (id:1) has been deleted

[jonross, 2015-09-30 22:21:42]

Patchset #1 (id:20001) has been deleted

[jonross, 2015-09-30 22:23:02]

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

[jonross, 2015-09-30 22:23:02]

Hey Terry,

Could you take a first look at this review?

Thanks,
Jon

[Peter Kasting, 2015-10-01 01:12:17]

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

[Peter Kasting, 2015-10-01 01:12:17]

https://chromiumcodereview.appspot.com/1378023003/diff/40001/chrome/browser/u...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://chromiumcodereview.appspot.com/1378023003/diff/40001/chrome/browser/u...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:51:
label_->SetAutoColorReadabilityEnabled(false);
You shouldn't need to do this; if you do something is likely wrong with the
colors you're using (maybe because the spec is wrong).

https://chromiumcodereview.appspot.com/1378023003/diff/40001/chrome/browser/u...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:111: const int
image_veritcal_padding = (height() - image_height) / 2;
Nit: vertical -- but I'd probably just inline this into the next statement.

That is, if this is necessary.  It surprises me that you need this conditional
at all; image objects should auto-vertically-center themselves within the
available space.  If that's not happening here I'd like to understand why.

https://chromiumcodereview.appspot.com/1378023003/diff/40001/chrome/browser/u...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/1378023003/diff/40001/chrome/browser/u...
chrome/browser/ui/views/location_bar/location_bar_view.cc:873: return 0;
Conceptually it seems like if this is correct, we'd want to change
GetVerticalThickness() instead (which would change this as a side effect).

[jonross, 2015-10-01 15:46:50]

https://codereview.chromium.org/1378023003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1378023003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:51:
label_->SetAutoColorReadabilityEnabled(false);
On 2015/10/01 01:12:17, Peter Kasting wrote:
> You shouldn't need to do this; if you do something is likely wrong with the
> colors you're using (maybe because the spec is wrong).

Yeah it is rejecting White text on all three background colors.

[Peter Kasting, 2015-10-05 21:53:14]

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (left):

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:48: 
Nit: Don't remove this

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:9: #include
"ui/base/resource/material_design/material_design_controller.h"
Nit: This is no longer needed

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:222: 2;
It's not really clear to me why you moved away from the previous calculation.

I'm also not sure whether this height includes or excludes the 1 px border of
the bubble itself.  I assume excludes based on your usage below?

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:224:
GetLayoutConstant(LOCATION_BAR_BUBBLE_FONT_PIXEL_SIZE);
I still think we should eliminate this and just let the autosizing code pick the
font size.  Sebastien didn't really speak to this question in depth, so if he
feels strongly about the 11 px size I'd like to understand why; perhaps there's
a way to encode his preferences in the code in a way that doesn't hardcode the
font size.

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:389: case BUBBLE_TEXT:
If this is now only used for the search chit, we should probably name it more
specifically, e.g. "KEYWORD_SEARCH_TEXT".

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:881: // In Material
Design horizontal layout disregards the border.
Nit: Remove "horizontal"?

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:883: return 0;
This will change the following besides the bubble calculations:

* The rect we fill with the background color in popup mode
* The round rect we fill with the background color in normal mode
* The location of things like the icon at the front and the star at the end of
the location bar

Make sure all these have been modified in a way that makes them more correct in
MD mode.

[jonross, 2015-10-06 17:58:26]

Patchset #5 (id:120001) has been deleted

[jonross, 2015-10-06 18:00:14]

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (left):

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:48: 
On 2015/10/05 21:53:13, Peter Kasting wrote:
> Nit: Don't remove this

Done.

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:9: #include
"ui/base/resource/material_design/material_design_controller.h"
On 2015/10/05 21:53:13, Peter Kasting wrote:
> Nit: This is no longer needed

Done.

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:222: 2;
On 2015/10/05 21:53:14, Peter Kasting wrote:
> It's not really clear to me why you moved away from the previous calculation.
> 
> I'm also not sure whether this height includes or excludes the 1 px border of
> the bubble itself.  I assume excludes based on your usage below?

Before MD bubbles were inset further than other items in the location bar.
During the switch to MD layout this was leading to needing a negative inset,
which we rejected.

Bubble padding was separated from the rest of padding, and bubble height was
calculated separately from GetInternalHeight which already adds the default
padding.

However this spot was missed.

bubble_height here does incorporate the 1 pixel border of the bubble in order to
determine the size available for the actual text.

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:224:
GetLayoutConstant(LOCATION_BAR_BUBBLE_FONT_PIXEL_SIZE);
On 2015/10/05 21:53:14, Peter Kasting wrote:
> I still think we should eliminate this and just let the autosizing code pick
the
> font size.  Sebastien didn't really speak to this question in depth, so if he
> feels strongly about the 11 px size I'd like to understand why; perhaps
there's
> a way to encode his preferences in the code in a way that doesn't hardcode the
> font size.

I agree with removing this. Current calculations result in 12px size. If this is
deemed undesirable I'd recommend that we change the specification for the
bubbles internal padding.

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:389: case BUBBLE_TEXT:
On 2015/10/05 21:53:14, Peter Kasting wrote:
> If this is now only used for the search chit, we should probably name it more
> specifically, e.g. "KEYWORD_SEARCH_TEXT".

Done.

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:881: // In Material
Design horizontal layout disregards the border.
On 2015/10/05 21:53:13, Peter Kasting wrote:
> Nit: Remove "horizontal"?

Done.

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:883: return 0;
On 2015/10/05 21:53:14, Peter Kasting wrote:
> This will change the following besides the bubble calculations:
> 
> * The rect we fill with the background color in popup mode
> * The round rect we fill with the background color in normal mode
> * The location of things like the icon at the front and the star at the end of
> the location bar
> 
> Make sure all these have been modified in a way that makes them more correct
in
> MD mode.

This improves the background color for both modes. There was a gap introduced
with the shift to programmatic rendering of the border. I want to do a follow-up
where I adjust the rounded corners of the background to match the md-corners.

The other icons at the beginning/end of the location bar make use of a different
padding value now. Which already incorporated the border thickness.

[Peter Kasting, 2015-10-06 19:15:42]

LGTM

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:222: 2;
On 2015/10/06 18:00:14, jonross wrote:
> On 2015/10/05 21:53:14, Peter Kasting wrote:
> > It's not really clear to me why you moved away from the previous
calculation.
> > 
> > I'm also not sure whether this height includes or excludes the 1 px border
of
> > the bubble itself.  I assume excludes based on your usage below?
> 
> Before MD bubbles were inset further than other items in the location bar.
> During the switch to MD layout this was leading to needing a negative inset,
> which we rejected.
> 
> Bubble padding was separated from the rest of padding, and bubble height was
> calculated separately from GetInternalHeight which already adds the default
> padding.
> 
> However this spot was missed.
> 
> bubble_height here does incorporate the 1 pixel border of the bubble in order
to
> determine the size available for the actual text.

OK.  Just make sure that the lock in a non-EV case (e.g.
https://codereview.chromium.org/ ) is positioned at the same place as the lock
inside the EV bubble (e.g. https://paypal.com/ ).  We used to have problems a
long time ago where when the EV or keyword search bubbles appeared, the icons in
them no longer aligned horizontally with the icons in other cases (e.g. the
omnibox dropdown).

Nit: I might order the terms in this expression differently so they went
logically from top to bottom:

  const int bubble_padding =
      GetVerticalEdgeThickness() +
      GetLayoutConstant(LOCATION_BAR_BUBBLE_VERTICAL_PADDING) +
      kBubbleInteriorVerticalPadding;
  const int bubble_content_height =
      GetPreferredSize().height() - (bubble_padding * 2);

[jonross, 2015-10-06 20:42:35]

On 2015/10/06 19:15:42, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/1378023003/diff/100001/chrome/browser/ui/view...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:222: 2;
> On 2015/10/06 18:00:14, jonross wrote:
> > On 2015/10/05 21:53:14, Peter Kasting wrote:
> > > It's not really clear to me why you moved away from the previous
> calculation.
> > > 
> > > I'm also not sure whether this height includes or excludes the 1 px border
> of
> > > the bubble itself.  I assume excludes based on your usage below?
> > 
> > Before MD bubbles were inset further than other items in the location bar.
> > During the switch to MD layout this was leading to needing a negative inset,
> > which we rejected.
> > 
> > Bubble padding was separated from the rest of padding, and bubble height was
> > calculated separately from GetInternalHeight which already adds the default
> > padding.
> > 
> > However this spot was missed.
> > 
> > bubble_height here does incorporate the 1 pixel border of the bubble in
order
> to
> > determine the size available for the actual text.
> 
> OK.  Just make sure that the lock in a non-EV case (e.g.
> https://codereview.chromium.org/ ) is positioned at the same place as the lock
> inside the EV bubble (e.g. https://paypal.com/ ).  We used to have problems a
> long time ago where when the EV or keyword search bubbles appeared, the icons
in
> them no longer aligned horizontally with the icons in other cases (e.g. the
> omnibox dropdown).
> 
> Nit: I might order the terms in this expression differently so they went
> logically from top to bottom:
> 
>   const int bubble_padding =
>       GetVerticalEdgeThickness() +
>       GetLayoutConstant(LOCATION_BAR_BUBBLE_VERTICAL_PADDING) +
>       kBubbleInteriorVerticalPadding;
>   const int bubble_content_height =
>       GetPreferredSize().height() - (bubble_padding * 2);

The locks stay in the same position for non-EV and EV cases.

Updated the expression as well.

[jonross, 2015-10-06 20:42:50]

The CQ bit was checked by jonross@chromium.org

[jonross, 2015-10-06 20:42:50]

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/1378023003/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-10-06 20:43:18]

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

[commit-bot: I haz the power, 2015-10-06 22:07:48]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-10-06 22:09:24]

Patchset 6 (id:??) landed as
https://crrev.com/662ed852e0f73f9676d3c1aedee5f144a00e6581
Cr-Commit-Position: refs/heads/master@{#352697}