Adjusts content bubble animation with respect to icon size

chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc

Index: chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
diff --git a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
index f51e30a49c0ff20605da55c8b3c2967800534558..38d97fbcceaebe57e59d7366e1a1129cc4545340 100644
--- a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
+++ b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
@@ -105,6 +105,10 @@ double IconLabelBubbleView::WidthMultiplier() const {
   return 1.0;
 }
 
+bool IconLabelBubbleView::IsShrinking() const {
+  return false;
+}
+
 int IconLabelBubbleView::GetImageAndPaddingWidth() const {
   const int image_width = image_->GetPreferredSize().width();
   return image_width
@@ -123,15 +127,37 @@ void IconLabelBubbleView::Layout() {
   // this up when MD is on by default.
   bool icon_has_enough_padding =
       !is_extension_icon_ || ui::MaterialDesignController::IsModeMaterial();
-  const int image_width = image_->GetPreferredSize().width();
-  image_->SetBounds(std::min((width() - image_width) / 2,
-                             GetBubbleOuterPadding(icon_has_enough_padding)),
-                    0, image_->GetPreferredSize().width(), height());
+  const int image_preferred_width = image_->GetPreferredSize().width();
+  // image is positioned at a constant offset when the bubble is growing or when
+  // there is enough room to fit the whole image. This is done so that the image
+  // slides in as a part of the sliding surface and doesn't shift relative to
+  // the background.
+  const bool shrinking = IsShrinking();
+  int image_trailing_padding = GetBubbleOuterPadding(false);
+  int image_x = GetBubbleOuterPadding(icon_has_enough_padding);
+  if (shrinking &&
+      (!label_->visible() || image_x + image_preferred_width > width())) {

[Peter Kasting, 2016/03/11 07:15:08]

Hmm.  Doesn't this mean that once we get to the point of hiding the label, the
image will suddenly snap rightwards (since its trailing padding will suddenly
drop to zero)?

Also, your code here checks if the label is not visible before it (lower down)
calculates if the label should not be visible.

Also, I realized that the changes in this CL affect
ContentSettingImageView::Update() because it assumed ShouldShowBackground()
means "is the animation running".

Finally, while thinking about the above, I got a bit worried about how we're
mucking with label visibility.  I'm pretty sure in the end there aren't
problems, but just in case, the rest of my proposed changes here move back to a
model where the label is "visible" through the whole animation, even when it's
zero-size.

OK, so here are my proposed fixes to all this:

(1) Change ContentSettingImageView::Update():

Specifically, this line:

  if (string_id && !ShouldShowBackground()) {

...can instead be:

  if (string_id && !label_.visible()) {

(You could also check whether the animation is running or paused, but this
should be equivalent and is shorter.)

(2) Instead of making ContentSettingImageView::ShouldShowBackground() check the
label visibility, and changing the label visibility in this function, make the
implementation of that function be:

bool ContentSettingImageView::ShouldShowBackground() const {
  return (slide_animator_.is_animating() || pause_animation_) &&
      (!IsShrinking() || (label()->width() > 0));
}

(3) Change the body of this function to the following, which should address me
concerns and have clearer comments to boot:

{
  // Compute the label bounds.  The label gets whatever size is left over after
  // accounting for the preferred image width and padding amounts.  Note that if
  // the label has zero size it doesn't actually matter what we compute its X
  // value to be, since it won't be visible, so the X value can be "wrong"
  // compared to where the right edge of the image is computed to be below.
  // This means doing this layout doesn't doesn't depend on any of the layout
  // below.  That layout, however, may need for this layout to have already
  // happened, since the value of ShouldShowBackground() we read below may
  // depend on whether the label has nonzero size.  Therefore, we do this first.
  const int label_x = GetBubbleOuterPadding(true) + GetImageAndPaddingWidth();
  const int label_width =
      std::max(0, width() - label_x - GetBubbleOuterPadding(false));
  label_->SetBounds(label_x, 0, label_width, height());

  // Now compute the image bounds.  In non-MD, the leading padding depends on
  // whether this is an extension icon, since extension icons and
  // Chrome-provided icons are different sizes.  In MD, these sizes are the
  // same, so it's not necessary to handle the two types differently.
  const bool icon_has_enough_padding =
      !is_extension_icon_ || ui::MaterialDesignController::IsModeMaterial();
  int image_x = GetBubbleOuterPadding(icon_has_enough_padding);

  // When animating, there may not be enough space for even just the leading
  // padding, image, and trailing padding.  In this case, shrink the trailing
  // padding first, so that as the view animates in the image slides in from the
  // trailing edge, and as it animates out it slides all the way to the trailing
  // edge before slowing or stopping.
  const int image_preferred_width = image_->GetPreferredSize().width();
  const int bubble_trailing_padding =
      std::min(GetBubbleOuterPadding(false),
               std::max(width() - image_preferred_width - image_x, 0));

  // If ShouldShowBackground() is true, then either we show a background in the
  // steady state, or we're not yet in the last portion of the animation.  In
  // these cases, we leave the leading padding alone, to keep the image pinned
  // near the leading edge of the view.  If this is false, however, then we're
  // only showing the image, and either the view width is the image width, or
  // it's animating downwards and getting close to it.  In these cases, we want
  // to start shrinking the leading padding down to zero once the trailing
  // padding has gone down to zero.  This is equivalent to checking if the view
  // width is not even large enough to accommodate the image preferred width
  // and the full leading padding.
  if (!ShouldShowBackground())
    image_x = std::min(image_x, std::max(width() - image_preferred_width, 0));

  // Now that we've computed the padding values, give the image all the
  // remaining width.  This will be less than the image's preferred width during
  // the first portion of the animation.
  const int image_width = std::min(image_preferred_width,
                                   width() - image_x - bubble_trailing_padding);
  image_->SetBounds(image_x, 0, image_width, height());
}

You should, of course, double-check my logic and test that this looks right when
slowed way down.

[varkha, 2016/03/11 10:16:20]

No. This code was getting triggered only once image right boundary touches the
right
border of |this| bubble view (which means that the image itself is already at
the
final position. At this point all that the animation is doing is squeezing the
left padding to bubble_width - image_preferred_width. In other words we only get
into this when there should be no trailing padding.
Yes, I have kept it for the case if some derived class decides to manipulate
label visibility (i.e. when the label is hidden to begin with). In case when we
animate, this check should have been redundant.
Yes, I think you are right.
I have added (1) and (2) as-is.
(3) needed a couple tweaks
 - I don't want to allow image_width to be negative (benign).
 - trailing padding should not start at zero, it looks bad since the icon
protrudes
   beyond the right edge when the bubble is growing and very small. I wanted a
small
   trailing padding to be there and that results in proper clipping.

+    image_trailing_padding = 0;
+    image_x = std::min(image_x, std::max(0, width() - image_preferred_width));
+  }
+  const int image_width = std::min(image_preferred_width,
+                                   width() - image_x - image_trailing_padding);
+  image_->SetBounds(image_x, 0, image_width, height());
+
+  int label_x = GetBubbleOuterPadding(true) + GetImageAndPaddingWidth();
+  const int label_width =
+      std::max(0, width() - label_x - GetBubbleOuterPadding(false));
+
+  // Hiding the label when the leading edge of the image plus padding crosses
+  // the right edge of the bubble allows the final frames of the animation to
+  // smoothly transition from showing a bubble to showing just the image.
+  if (shrinking && label_->visible() &&
+      (image_x + image_preferred_width + image_trailing_padding >=
+          bounds().width())) {
+    label_->SetVisible(false);
+  }
 
-  int pre_label_width = GetBubbleOuterPadding(true) + GetImageAndPaddingWidth();
-  label_->SetBounds(pre_label_width, 0,
-                    width() - pre_label_width - GetBubbleOuterPadding(false),
-                    height());
+  label_->SetBounds(label_x, 0, label_width, height());
 }
 
 void IconLabelBubbleView::OnNativeThemeChanged(
@@ -177,20 +203,40 @@ SkColor IconLabelBubbleView::GetParentBackgroundColor() const {
              : parent_background_color_;
 }
 
-gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int width) const {
+gfx::Size IconLabelBubbleView::GetSizeForLabelWidth(int label_width) const {
   gfx::Size size(image_->GetPreferredSize());
-  if (ShouldShowBackground()) {
-    const int non_label_width = GetBubbleOuterPadding(true) +
-                                GetImageAndPaddingWidth() +
-                                GetBubbleOuterPadding(false);
-    size = gfx::Size(WidthMultiplier() * (width + non_label_width), 0);
-    if (!ui::MaterialDesignController::IsModeMaterial())
-      size.SetToMax(background_painter_->GetMinimumSize());
+  bool shrinking = IsShrinking();
+  // Animation continues for the last few pixels even after the label is not
+  // visible in order to slide the icon into its final position. Therefore it
+  // is necessary to animate |total_width| even when the background is hidden
+  // as long as the animation is still shrinking.
+  if (ShouldShowBackground() || shrinking) {
+    const int image_width = size.width();
+    const int padding = GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_PADDING) +
+        GetBubbleOuterPadding(true) + GetBubbleOuterPadding(false);
+    // |multiplier| grows from zero to one, stays equal to one and then shrinks
+    // to zero again. The view width should correspondingly grow from zero to
+    // fully showing both label and icon, stay there, then shrink to just large
+    // enough to show the icon. We don't want to shrink all the way back to
+    // zero, since this would mean the view would completely disappear and then
+    // pop back to an icon after the animation finishes.
+    int total_width = WidthMultiplier() * (label_width + image_width + padding);
+    if (shrinking)
+      total_width = std::max(total_width, image_width);
+    size.set_width(total_width);
   }
-
   return size;
 }
 
+int IconLabelBubbleView::GetBubbleOuterPadding(bool leading) const {
+  if (ui::MaterialDesignController::IsModeMaterial())
+    return GetBubbleOuterPaddingMd(leading);
+
+  return GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING) -
+         GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
+         (leading ? 0 : GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING));
+}
+
 void IconLabelBubbleView::SetLabelBackgroundColor(
     SkColor chip_background_color) {
   // The background images are painted atop |parent_background_color_|.
@@ -205,15 +251,6 @@ void IconLabelBubbleView::SetLabelBackgroundColor(
       SkColorGetA(chip_background_color)));
 }
 
-int IconLabelBubbleView::GetBubbleOuterPadding(bool leading) const {
-  if (ui::MaterialDesignController::IsModeMaterial())
-    return GetBubbleOuterPaddingMd(leading);
-
-  return GetLayoutConstant(LOCATION_BAR_HORIZONTAL_PADDING) -
-         GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
-         (leading ? 0 : GetLayoutConstant(ICON_LABEL_VIEW_TRAILING_PADDING));
-}
-
 int IconLabelBubbleView::GetBubbleOuterPaddingMd(bool leading) const {
   // When the image is empty, leading and trailing padding are equal.
   if (image_->GetPreferredSize().IsEmpty() || !leading)