Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1763713004:
Adjusts content bubble animation with respect to icon size (Closed)
|
Created:
4 years, 9 months ago by varkha Modified:
4 years, 9 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, tdanderson Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjusts content bubble animation with respect to icon size
This CL corrects several issues with the icon bubble animation:
1. The bubble is not clipping its children, so the image draws outside the bubble bounds. This is MD-specific.
2. The image is being centered at the bubble center when the size is too low, instead of always being positioned based on the leading edge of the bubble, so it doesn't "slide in" correctly shifting during the initial portion of the animation.
3. The animation "closes" down to zero width (MD) or smaller than necessary width (non-MD) and then snaps the image back into place; it should animate down to become just large enough to show the image.
4. Removes the clamp in non-MD size that prevents animation from starting at zero.
5. Avoids clipping icon at the end of the animation (MD) by hiding the border even before the animation ends.
6. Reduces padding at the end of the MD version of the text label.
7. Corrects RTL layout by showing trailing edge of the icon first.
BUG=591411
Committed: https://crrev.com/a2a4fab1693d6985dda4de9e91825fd667f07598
Cr-Commit-Position: refs/heads/master@{#380729}
Patch Set 1 : Adjusts content bubble animation with respect to icon size #Patch Set 2 : Adjusts content bubble animation with respect to icon size (code comment) #
Total comments: 25
Patch Set 3 : Adjusts content bubble animation with respect to icon size (review comments) #
Total comments: 22
Patch Set 4 : Adjusts content bubble animation with respect to icon size (constant speed) #
Total comments: 8
Patch Set 5 : Adjusts content bubble animation with respect to icon size (fix for a regression in growth stage) #
Total comments: 2
Patch Set 6 : Adjusts content bubble animation with respect to icon size (unit test) #Patch Set 7 : Adjusts content bubble animation with respect to icon size (not changing label visibility) #
Total comments: 6
Patch Set 8 : Adjusts content bubble animation with respect to icon size (nits) #
Messages
Total messages: 42 (15 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763713004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763713004/20001
varkha@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@, can you please take a first look? This would benefit from a bit more comments in Layout(), I will pen something in the meanwhile. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkasting@, ping?
https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:127: // animation (slide out, show, then slide in) is 1.0. We probably need more comments here, since now we won't spend the same amount of time animating closed as open, even though we calculate the curves as if we will. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:139: slide_animator_.Stop(); Stop() is redundant with Reset(). You should only need the latter. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:156: SchedulePaint(); Given the definition of SchedulePaint(), it seems like calling it on parent_ should schedule a paint for the entire parent rect, which should eliminate the need to call SchedulePaint() here on the child. Either this should not be added, or I'd like to understand technically why what I just said isn't true. I wonder if the issue is that parent_->Layout() sees that the parent's size hasn't changed and just quickly no-ops or something, without sizing us to our new preferred size. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:265: // Snap animation to target if a user clicks before the image is exposed. Nit: a -> the https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:268: if (pause_animation_state_ < kOpenFraction && Is this first portion of the conditional needed? Seems like if at the end of the animation we animate just down to our target size, this check is redundant. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:132: const int image_min_width = image_->GetPreferredSize().width(); Nit: |image_min_width| seems misleading here since this is really the image's preferred width, and we can allocate less space than this below. I would name this |image_width| and then either inline the current |image_width| temp below into the subsequent SetBounds() call or pick a name for it like |available_width|. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:161: StopAnimation(); I don't think this is the right way to do this. If we stop the animation in Layout(), then depending on whether we're dropping frames, we may not get called until the animation has run past the "minimum size" point; get sized to our preferred size, which will be too small; have Layout() called; then try to stop the animation here. I'm worried about this resulting in flickery drawing. It seems better to change WidthMultiplier() so that we calculate the correct (clamped) "preferred size" at all times, and if we want to stop running the animation once it's not changing anything, put that calculation in AnimationProgressed() or something. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:211: if (ShouldShowBackground() || IsShrinking()) { Won't ShouldShowBackground() always be true if we actually have a running animation? https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:223: return GetBubbleOuterPaddingMd(leading); Nit: We might as well just scrap GetBubbleOuterPaddingMd() and inline it here. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:56: views::Label* label() const { return label_; } Don't make these changes, or else make them by adding parallel functions that return const pointers; const member functions shouldn't return non-const pointers. (We violate this in a gazillion places, but don't add more.) https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:98: // Amount of padding at the edges of the bubble. If |leading| is true, this Nit: For clarity you might want to be a bit more precise about this padding; for example, is it the padding from the edge of the icon/label to the outer edge of the bubble, or does it stop just inside the bubble border, or does it maybe extend out to include some of the whitespace outside the bubble? https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:100: // the end padding. Nit: end -> trailing
I was thinking about stopping it being not elegant, thanks for the nudge! https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:127: // animation (slide out, show, then slide in) is 1.0. On 2016/03/07 20:24:27, Peter Kasting wrote: > We probably need more comments here, since now we won't spend the same amount of > time animating closed as open, even though we calculate the curves as if we > will. Timings should be matching the original comment now. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:139: slide_animator_.Stop(); On 2016/03/07 20:24:27, Peter Kasting wrote: > Stop() is redundant with Reset(). You should only need the latter. Done. also this method is no longer necessary. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:156: SchedulePaint(); On 2016/03/07 20:24:27, Peter Kasting wrote: > Given the definition of SchedulePaint(), it seems like calling it on parent_ > should schedule a paint for the entire parent rect, which should eliminate the > need to call SchedulePaint() here on the child. Either this should not be > added, or I'd like to understand technically why what I just said isn't true. > > I wonder if the issue is that parent_->Layout() sees that the parent's size > hasn't changed and just quickly no-ops or something, without sizing us to our > new preferred size. Done. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:265: // Snap animation to target if a user clicks before the image is exposed. On 2016/03/07 20:24:27, Peter Kasting wrote: > Nit: a -> the Done. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:268: if (pause_animation_state_ < kOpenFraction && On 2016/03/07 20:24:27, Peter Kasting wrote: > Is this first portion of the conditional needed? Seems like if at the end of > the animation we animate just down to our target size, this check is redundant. Done. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:132: const int image_min_width = image_->GetPreferredSize().width(); On 2016/03/07 20:24:27, Peter Kasting wrote: > Nit: |image_min_width| seems misleading here since this is really the image's > preferred width, and we can allocate less space than this below. > > I would name this |image_width| and then either inline the current |image_width| > temp below into the subsequent SetBounds() call or pick a name for it like > |available_width|. Done. Renamed to |image_preferred_width|. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:161: StopAnimation(); On 2016/03/07 20:24:27, Peter Kasting wrote: > I don't think this is the right way to do this. > > If we stop the animation in Layout(), then depending on whether we're dropping > frames, we may not get called until the animation has run past the "minimum > size" point; get sized to our preferred size, which will be too small; have > Layout() called; then try to stop the animation here. I'm worried about this > resulting in flickery drawing. > > It seems better to change WidthMultiplier() so that we calculate the correct > (clamped) "preferred size" at all times, and if we want to stop running the > animation once it's not changing anything, put that calculation in > AnimationProgressed() or something. I have put the correction in IconLabelBubbleView::GetSizeForLabelWidth(). This allows the multiplier to still grow from 0 to 1 and then go back to 0 but the width grows from 0 to max and then shrinks from max to image size plus leading padding at which point the icon is in its target state and the animation stops. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:211: if (ShouldShowBackground() || IsShrinking()) { On 2016/03/07 20:24:27, Peter Kasting wrote: > Won't ShouldShowBackground() always be true if we actually have a running > animation? There is also label()->visible() condition (in the ContentSettingImageView::ShouldShowBackground(). I want to be able to hide the label a bit earlier but continue animating in the last few frames. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:223: return GetBubbleOuterPaddingMd(leading); On 2016/03/07 20:24:27, Peter Kasting wrote: > Nit: We might as well just scrap GetBubbleOuterPaddingMd() and inline it here. I think keeping it separate makes it a bit easier to reason and should also simplify removing non-MD path in future. I'd rather keep it. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:56: views::Label* label() const { return label_; } On 2016/03/07 20:24:27, Peter Kasting wrote: > Don't make these changes, or else make them by adding parallel functions that > return const pointers; const member functions shouldn't return non-const > pointers. > > (We violate this in a gazillion places, but don't add more.) Done. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:98: // Amount of padding at the edges of the bubble. If |leading| is true, this On 2016/03/07 20:24:27, Peter Kasting wrote: > Nit: For clarity you might want to be a bit more precise about this padding; for > example, is it the padding from the edge of the icon/label to the outer edge of > the bubble, or does it stop just inside the bubble border, or does it maybe > extend out to include some of the whitespace outside the bubble? Done. https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:100: // the end padding. On 2016/03/07 20:24:27, Peter Kasting wrote: > Nit: end -> trailing Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763713004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763713004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/1763713004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:156: SchedulePaint(); On 2016/03/08 18:01:03, varkha wrote: > On 2016/03/07 20:24:27, Peter Kasting wrote: > > Given the definition of SchedulePaint(), it seems like calling it on parent_ > > should schedule a paint for the entire parent rect, which should eliminate the > > need to call SchedulePaint() here on the child. Either this should not be > > added, or I'd like to understand technically why what I just said isn't true. > > > > I wonder if the issue is that parent_->Layout() sees that the parent's size > > hasn't changed and just quickly no-ops or something, without sizing us to our > > new preferred size. > > Done. Will remove the commented line obviously before commit.
https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:32: const int ContentSettingImageView::kOpenTimeMS = 15000; You don't want to commit this :) https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:151: // SchedulePaint(); Remove or else uncomment and add notes as to why https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:266: } Nit: Seems like this would make more sense hoisted, so it's clear you don't need to set |pause_animation_state_|. Here's a more comprehensive comment, too. if (slide_animator_.is_animating()) { // If the user clicks while we're animating, the bubble arrow will be // pointing to the image, and if we allow the animation to keep running, the // image will move away from the arrow (or we'll have to move the bubble, // which is even worse). So we want to stop the animation. We have two // choices: jump to the final post-animation state (no label visible), or // pause the animation where we are and continue running after the bubble // closes. The former looks more jerky, so we avoid it unless the animation // hasn't even fully exposed the image yet, in which case pausing with half // an image visible will look broken. const int final_width = image()->GetPreferredSize().width() + GetBubbleOuterPadding(true) + GetBubbleOuterPadding(false); if (!pause_animation_ && (width() > final_width)) { pause_animation_ = true; pause_animation_state_ = slide_animator_.GetCurrentValue(); } slide_animator_.Reset(); } https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:136: ? GetBubbleOuterPadding(icon_has_enough_padding) Can we just always do this? Is it ever actually necessary to center the image instead? Maybe for extension icons pre-MD or something? https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:153: (image_x + image_padding + image_preferred_width >= bounds().width())) { Why is the last conditional clause here necessary? https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:206: if (ShouldShowBackground() || shrinking) { Add comment on why it's necessary to check for |shrinking| here (basically what you said to me when I asked before). https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:208: const int padding_rest = Nit: |remaining_padding| seems better ("rest" sounds like "padding at rest") https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:219: : multiplier * (width + padding_rest + leading_padding + image_width); This means the view will shrink more slowly (in terms of linear movement speed) than it grows. I don't think that's as good as the apparent behavior we had before, where the velocity was constant and the time was different. I think you can achieve this by computing the total width like the second arm here in all cases, then clamping if |shrinking| is true. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:96: // Amount of padding from the edge of the icon / label to the nearest edge of Nit: nearest -> outer (Seems slightly clearer to me; it's pretty obvious we'd choose the nearer edge, but explicitly saying "outer" makes it clear this isn't e.g. the inside edge of the border).
pkasting@, I think I have addressed your comments, can you PTAL? https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:32: const int ContentSettingImageView::kOpenTimeMS = 15000; On 2016/03/08 22:07:06, Peter Kasting wrote: > You don't want to commit this :) Wasn't planning on it! https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:151: // SchedulePaint(); On 2016/03/08 22:07:06, Peter Kasting wrote: > Remove or else uncomment and add notes as to why Done. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/content_setting_image_view.cc:266: } On 2016/03/08 22:07:06, Peter Kasting wrote: > Nit: Seems like this would make more sense hoisted, so it's clear you don't need > to set |pause_animation_state_|. Here's a more comprehensive comment, too. > > if (slide_animator_.is_animating()) { > // If the user clicks while we're animating, the bubble arrow will be > // pointing to the image, and if we allow the animation to keep running, the > // image will move away from the arrow (or we'll have to move the bubble, > // which is even worse). So we want to stop the animation. We have two > // choices: jump to the final post-animation state (no label visible), or > // pause the animation where we are and continue running after the bubble > // closes. The former looks more jerky, so we avoid it unless the animation > // hasn't even fully exposed the image yet, in which case pausing with half > // an image visible will look broken. > const int final_width = image()->GetPreferredSize().width() + > GetBubbleOuterPadding(true) + GetBubbleOuterPadding(false); > if (!pause_animation_ && (width() > final_width)) { > pause_animation_ = true; > pause_animation_state_ = slide_animator_.GetCurrentValue(); > } > slide_animator_.Reset(); > } Done. Thanks for the wording. You also got the subtlety of width() > final_width rather than >= correctly. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:136: ? GetBubbleOuterPadding(icon_has_enough_padding) On 2016/03/08 22:07:06, Peter Kasting wrote: > Can we just always do this? Is it ever actually necessary to center the image > instead? Maybe for extension icons pre-MD or something? Done. Still need to make sure the image is right aligned at the end of the animation. We don't want it to overshoot and then snap back. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:153: (image_x + image_padding + image_preferred_width >= bounds().width())) { On 2016/03/08 22:07:06, Peter Kasting wrote: > Why is the last conditional clause here necessary? I wanted to hide the boundary only when the image is roughly centered in the bubble - having the correct padding on the left and right. If I omit this last condition the boundary will disappear earlier - when the text no longer has room left which is the point where there is icon-to-text-padding + text-to-right-boundary-padding to the right of the icon. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:206: if (ShouldShowBackground() || shrinking) { On 2016/03/08 22:07:06, Peter Kasting wrote: > Add comment on why it's necessary to check for |shrinking| here (basically what > you said to me when I asked before). Done. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:208: const int padding_rest = On 2016/03/08 22:07:06, Peter Kasting wrote: > Nit: |remaining_padding| seems better ("rest" sounds like "padding at rest") Done. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:219: : multiplier * (width + padding_rest + leading_padding + image_width); On 2016/03/08 22:07:06, Peter Kasting wrote: > This means the view will shrink more slowly (in terms of linear movement speed) > than it grows. I don't think that's as good as the apparent behavior we had > before, where the velocity was constant and the time was different. > > I think you can achieve this by computing the total width like the second arm > here in all cases, then clamping if |shrinking| is true. Sure. That makes the animation continue near the end for another few milliseconds without making any apparent change on screen which is why I didn't do it this way but this doesn't seem to cause trouble. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:96: // Amount of padding from the edge of the icon / label to the nearest edge of On 2016/03/08 22:07:06, Peter Kasting wrote: > Nit: nearest -> outer > > (Seems slightly clearer to me; it's pretty obvious we'd choose the nearer edge, > but explicitly saying "outer" makes it clear this isn't e.g. the inside edge of > the border). Done.
https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:136: ? GetBubbleOuterPadding(icon_has_enough_padding) On 2016/03/10 22:25:56, varkha wrote: > On 2016/03/08 22:07:06, Peter Kasting wrote: > > Can we just always do this? Is it ever actually necessary to center the image > > instead? Maybe for extension icons pre-MD or something? > > Done. Still need to make sure the image is right aligned at the end of the > animation. We don't want it to overshoot and then snap back. How can we overshoot given the current definition of GetSizeForLabelWidth()? Won't that always reserve sufficient space? https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:153: (image_x + image_padding + image_preferred_width >= bounds().width())) { On 2016/03/10 22:25:56, varkha wrote: > On 2016/03/08 22:07:06, Peter Kasting wrote: > > Why is the last conditional clause here necessary? > > I wanted to hide the boundary only when the image is roughly centered in the > bubble - having the correct padding on the left and right. If I omit this last > condition the boundary will disappear earlier - when the text no longer has room > left which is the point where there is icon-to-text-padding + > text-to-right-boundary-padding to the right of the icon. That sounds like the (label_width == 0) check is unnecessary, then. Frankly, the label_->visible() check is too, but I can see why it might seem confusing to remove it. But I'd at least remove the first, along with the "it no longer fits and" bit of the comment above. https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:135: int image_padding = GetBubbleOuterPadding(false); Nit: image_trailing_padding? https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:139: image_x = std::min(image_x, width() - image_preferred_width); Doesn't this line result in pinning the trailing edge, rather than the leading edge, of the image during the initial "animate up from zero" portion? https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:216: GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_PADDING); Nit: Given the code below, you could now collapse these two padding values together into one |padding| temp. https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:220: // image width. Nit: How about: The view width should correspondingly grow from zero to fullying 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.)
Thanks for that catch! Was focusing at the shrinking part too much. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:136: ? GetBubbleOuterPadding(icon_has_enough_padding) On 2016/03/11 01:14:42, Peter Kasting wrote: > On 2016/03/10 22:25:56, varkha wrote: > > On 2016/03/08 22:07:06, Peter Kasting wrote: > > > Can we just always do this? Is it ever actually necessary to center the > image > > > instead? Maybe for extension icons pre-MD or something? > > > > Done. Still need to make sure the image is right aligned at the end of the > > animation. We don't want it to overshoot and then snap back. > > How can we overshoot given the current definition of GetSizeForLabelWidth()? > Won't that always reserve sufficient space? I meant this as an explanation of what I have tried to do in a previous patch set, not as a problem. A bigger trouble of course is that I have allowed this to happen during the growth stage as well. https://codereview.chromium.org/1763713004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:153: (image_x + image_padding + image_preferred_width >= bounds().width())) { On 2016/03/11 01:14:42, Peter Kasting wrote: > On 2016/03/10 22:25:56, varkha wrote: > > On 2016/03/08 22:07:06, Peter Kasting wrote: > > > Why is the last conditional clause here necessary? > > > > I wanted to hide the boundary only when the image is roughly centered in the > > bubble - having the correct padding on the left and right. If I omit this last > > condition the boundary will disappear earlier - when the text no longer has > room > > left which is the point where there is icon-to-text-padding + > > text-to-right-boundary-padding to the right of the icon. > > That sounds like the (label_width == 0) check is unnecessary, then. Frankly, > the label_->visible() check is too, but I can see why it might seem confusing to > remove it. But I'd at least remove the first, along with the "it no longer fits > and" bit of the comment above. Done. https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:135: int image_padding = GetBubbleOuterPadding(false); On 2016/03/11 01:14:42, Peter Kasting wrote: > Nit: image_trailing_padding? Done. https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:139: image_x = std::min(image_x, width() - image_preferred_width); On 2016/03/11 01:14:42, Peter Kasting wrote: > Doesn't this line result in pinning the trailing edge, rather than the leading > edge, of the image during the initial "animate up from zero" portion? Yes, it was a bad regression. https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:216: GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_PADDING); On 2016/03/11 01:14:42, Peter Kasting wrote: > Nit: Given the code below, you could now collapse these two padding values > together into one |padding| temp. Done. https://codereview.chromium.org/1763713004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:220: // image width. On 2016/03/11 01:14:42, Peter Kasting wrote: > Nit: How about: > > The view width should correspondingly grow from zero to fullying 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.) Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763713004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763713004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
I'm sorry this review is going so many rounds! Getting this right is complicated :P https://codereview.chromium.org/1763713004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:139: (!label_->visible() || image_x + image_preferred_width > width())) { 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.
Thanks so much, this looks a lot simpler. I have also added a unit test - should have added it earlier, that would avoid at least some of the last-minute regressions. https://codereview.chromium.org/1763713004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:139: (!label_->visible() || image_x + image_preferred_width > width())) { On 2016/03/11 07:15:08, Peter Kasting wrote: > 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)? 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. > > Also, your code here checks if the label is not visible before it (lower down) > calculates if the label should not be visible. 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. > > Also, I realized that the changes in this CL affect > ContentSettingImageView::Update() because it assumed ShouldShowBackground() > means "is the animation running". Yes, I think you are right. > > 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. 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.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763713004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763713004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:151: // edge before slowing or stopping. Nit: After your changes this comment no longer describes the block it's on, or, really, your algorithm at all (since you now only do this conditionally). How about this (takes into account my comments below): ... const bool icon_has_enough_padding = !is_extension_icon_ || ui::MaterialDesignController::IsModeMaterial(); int image_x = GetBubbleOuterPadding(icon_has_enough_padding); const int image_preferred_width = image_->GetPreferredSize().width(); int bubble_trailing_padding = GetBubbleOuterPadding(false); // 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 and trailing padding alone; we don't want // to let the image overlap the edge of the background, as this looks glitchy. // 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 shrink the trailing padding first, // so the image slides all the way to the trailing edge before slowing or // stopping; then we want to shrink the leading padding down to zero. if (!ShouldShowBackground()) { image_x = std::min(image_x, width() - image_preferred_width); bubble_trailing_padding = std::min( bubble_trailing_padding, width() - image_preferred_width - image_x); } // 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; during the very beginning there may not // be enough room to show the image at all. ... https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:166: image_x = std::min(image_x, std::max(width() - image_preferred_width, 0)); Nit: The std::max() call here is actually unnecessary; when there's no background, we should be guaranteed enough width to at least show the image. https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:169: std::max(width() - image_preferred_width - image_x, 0)); Nit: Now that this is happening after the image_x computation, the std::max() call here is unnecessary as well (provable by simple algebra).
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763713004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763713004/160001
Thanks for the review - it actually felt like you did most of the lifting on this, pkasting@! The bubble should look a bit better with all this although at the current speed it is probably not very noticeable. https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right): https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:151: // edge before slowing or stopping. On 2016/03/11 12:05:24, Peter Kasting wrote: > Nit: After your changes this comment no longer describes the block it's on, or, > really, your algorithm at all (since you now only do this conditionally). How > about this (takes into account my comments below): > > ... > const bool icon_has_enough_padding = > !is_extension_icon_ || ui::MaterialDesignController::IsModeMaterial(); > int image_x = GetBubbleOuterPadding(icon_has_enough_padding); > const int image_preferred_width = image_->GetPreferredSize().width(); > int bubble_trailing_padding = GetBubbleOuterPadding(false); > > // 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 and trailing padding alone; we don't want > // to let the image overlap the edge of the background, as this looks glitchy. > // 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 shrink the trailing padding first, > // so the image slides all the way to the trailing edge before slowing or > // stopping; then we want to shrink the leading padding down to zero. > if (!ShouldShowBackground()) { > image_x = std::min(image_x, width() - image_preferred_width); > bubble_trailing_padding = std::min( > bubble_trailing_padding, width() - image_preferred_width - image_x); > } > > // 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; during the very beginning there may not > // be enough room to show the image at all. > ... Done. https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:166: image_x = std::min(image_x, std::max(width() - image_preferred_width, 0)); On 2016/03/11 12:05:24, Peter Kasting wrote: > Nit: The std::max() call here is actually unnecessary; when there's no > background, we should be guaranteed enough width to at least show the image. Done. https://codereview.chromium.org/1763713004/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:169: std::max(width() - image_preferred_width - image_x, 0)); On 2016/03/11 12:05:24, Peter Kasting wrote: > Nit: Now that this is happening after the image_x computation, the std::max() > call here is unnecessary as well (provable by simple algebra). Done.
Thanks for the review - it actually felt like you did most of the lifting on this, pkasting@! The bubble should look a bit better with all this although at the current speed it is probably not very noticeable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by varkha@chromium.org
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/1763713004/#ps160001 (title: "Adjusts content bubble animation with respect to icon size (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763713004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763713004/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Adjusts content bubble animation with respect to icon size This CL corrects several issues with the icon bubble animation: 1. The bubble is not clipping its children, so the image draws outside the bubble bounds. This is MD-specific. 2. The image is being centered at the bubble center when the size is too low, instead of always being positioned based on the leading edge of the bubble, so it doesn't "slide in" correctly shifting during the initial portion of the animation. 3. The animation "closes" down to zero width (MD) or smaller than necessary width (non-MD) and then snaps the image back into place; it should animate down to become just large enough to show the image. 4. Removes the clamp in non-MD size that prevents animation from starting at zero. 5. Avoids clipping icon at the end of the animation (MD) by hiding the border even before the animation ends. 6. Reduces padding at the end of the MD version of the text label. 7. Corrects RTL layout by showing trailing edge of the icon first. BUG=591411 ========== to ========== Adjusts content bubble animation with respect to icon size This CL corrects several issues with the icon bubble animation: 1. The bubble is not clipping its children, so the image draws outside the bubble bounds. This is MD-specific. 2. The image is being centered at the bubble center when the size is too low, instead of always being positioned based on the leading edge of the bubble, so it doesn't "slide in" correctly shifting during the initial portion of the animation. 3. The animation "closes" down to zero width (MD) or smaller than necessary width (non-MD) and then snaps the image back into place; it should animate down to become just large enough to show the image. 4. Removes the clamp in non-MD size that prevents animation from starting at zero. 5. Avoids clipping icon at the end of the animation (MD) by hiding the border even before the animation ends. 6. Reduces padding at the end of the MD version of the text label. 7. Corrects RTL layout by showing trailing edge of the icon first. BUG=591411 Committed: https://crrev.com/a2a4fab1693d6985dda4de9e91825fd667f07598 Cr-Commit-Position: refs/heads/master@{#380729} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a2a4fab1693d6985dda4de9e91825fd667f07598 Cr-Commit-Position: refs/heads/master@{#380729} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
