Skip to content

Commit

Permalink
ash: Hide bubble launcher scroll view gradient during open animation
Browse files Browse the repository at this point in the history
This is a performance optimization for the animation. The launcher
opens with the scroll view scrolled to the top. The gradient only
shows on the bottom ~16 pixels of the scroll view, so it's not very
noticeable if we don't show the gradient until the animation ends.

Tested animation smoothness on kevin (arm), official branded build,
with ARC disabled, 10 runs.

Before optimization: smoothness mean 66, stddev 10
After optimization:  smoothness mean 80, stddev 12

T-test P=0.01 for a significant difference.

Bug: 1264178
Test: added to ash_unittests
Change-Id: I1e67c38d06a9c290785d36437623222ad6fa932d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3259244
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938292}
  • Loading branch information
James Cook committed Nov 4, 2021
1 parent 7247b5c commit aff9649
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 13 deletions.
36 changes: 28 additions & 8 deletions ash/app_list/views/app_list_bubble_apps_page.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "ash/controls/scroll_view_gradient_helper.h"
#include "ash/public/cpp/metrics_util.h"
#include "ash/public/cpp/style/color_provider.h"
#include "base/bind.h"
#include "base/check.h"
#include "base/metrics/histogram_functions.h"
#include "base/time/time.h"
Expand Down Expand Up @@ -171,8 +172,8 @@ void AppListBubbleAppsPage::StartShowAnimation() {
"Apps.ClamshellLauncher.AnimationSmoothness.OpenAppsPage", value);
})));

// TODO(https://crbug.com/1264178): Disable the gradient mask on the scroll
// view to improve performance.
// Disable the gradient mask on the scroll view to improve performance.
gradient_helper_->HideGradient(true);

// Animate the views. Each section is initially offset down, then slides up
// into its final position. If a section isn't visible, skip it. The further
Expand All @@ -196,7 +197,13 @@ void AppListBubbleAppsPage::StartShowAnimation() {

// The apps grid is always visible.
vertical_offset += kSectionOffset;
SlideViewIntoPosition(scrollable_apps_grid_view_, vertical_offset);
// Use a special cleanup callback to show the gradient mask at the end of the
// animation. No need to use SlideViewIntoPosition() because this view always
// has a layer.
StartSlideInAnimation(
scrollable_apps_grid_view_, vertical_offset,
base::BindRepeating(&AppListBubbleAppsPage::OnAppsGridViewAnimationEnded,
weak_factory_.GetWeakPtr()));
}

void AppListBubbleAppsPage::SlideViewIntoPosition(views::View* view,
Expand All @@ -216,25 +223,33 @@ void AppListBubbleAppsPage::SlideViewIntoPosition(views::View* view,
view->layer()->SetFillsBoundsOpaquely(false);
}

// Set the initial offset via a layer transform.
gfx::Transform translate_down;
translate_down.Translate(0, vertical_offset);
view->layer()->SetTransform(translate_down);

// If we created a layer for the view, undo that when the animation ends.
// The underlying views don't expose weak pointers directly, so use a weak
// pointer to this view, which owns its children.
auto cleanup = create_layer ? base::BindRepeating(
&AppListBubbleAppsPage::DestroyLayerForView,
weak_factory_.GetWeakPtr(), view)
: base::DoNothing();
StartSlideInAnimation(view, vertical_offset, cleanup);
}

void AppListBubbleAppsPage::StartSlideInAnimation(
views::View* view,
int vertical_offset,
base::RepeatingClosure cleanup) {
DCHECK(view->layer());

// Animation spec:
//
// Y Position: Down (offset) → End position
// Duration: 250ms
// Ease: (0.00, 0.00, 0.20, 1.00)

// Set the initial offset via a layer transform.
gfx::Transform translate_down;
translate_down.Translate(0, vertical_offset);
view->layer()->SetTransform(translate_down);

// Animate the transform back to the identity transform.
constexpr gfx::Transform kIdentity;
views::AnimationBuilder()
Expand Down Expand Up @@ -320,6 +335,11 @@ void AppListBubbleAppsPage::DestroyLayerForView(views::View* view) {
view->DestroyLayer();
}

void AppListBubbleAppsPage::OnAppsGridViewAnimationEnded() {
// Recreate the gradient mask, if necessary.
gradient_helper_->HideGradient(false);
}

BEGIN_METADATA(AppListBubbleAppsPage, views::View)
END_METADATA

Expand Down
17 changes: 14 additions & 3 deletions ash/app_list/views/app_list_bubble_apps_page.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "ash/app_list/views/apps_grid_view_focus_delegate.h"
#include "ash/app_list/views/recent_apps_view.h"
#include "ash/ash_export.h"
#include "base/callback_forward.h"
#include "base/memory/weak_ptr.h"
#include "ui/base/metadata/metadata_header_macros.h"
#include "ui/views/view.h"
Expand Down Expand Up @@ -59,9 +60,9 @@ class ASH_EXPORT AppListBubbleAppsPage : public views::View,
// Starts the launcher show animation.
void StartShowAnimation();

// Animates `view` using a layer animation. The layer is pushed down by
// `vertical_offset` at the start of the animation and animates back to its
// original position. Public for testing.
// Animates `view` using a layer animation. Creates the layer if needed. The
// layer is pushed down by `vertical_offset` at the start of the animation and
// animates back to its original position. Public for testing.
void SlideViewIntoPosition(views::View* view, int vertical_offset);

// views::View:
Expand Down Expand Up @@ -92,10 +93,20 @@ class ASH_EXPORT AppListBubbleAppsPage : public views::View,

void UpdateSeparatorVisibility();

// Starts a vertical slide animation for `view` with `vertical_offset` as the
// initial offset. The view must already have a layer. Runs the `cleanup`
// callback when the animation ends or aborts.
void StartSlideInAnimation(views::View* view,
int vertical_offset,
base::RepeatingClosure cleanup);

// Destroys the layer for `view`. Not static so it can be used with weak
// pointers.
void DestroyLayerForView(views::View* view);

// Callback for when the apps grid view animation ends.
void OnAppsGridViewAnimationEnded();

views::ScrollView* scroll_view_ = nullptr;
ContinueSectionView* continue_section_ = nullptr;
RecentAppsView* recent_apps_ = nullptr;
Expand Down
24 changes: 24 additions & 0 deletions ash/app_list/views/app_list_bubble_view_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,30 @@ TEST_F(AppListBubbleViewTest, ShowAnimationCreatesAndDestroysLayers) {
EXPECT_TRUE(apps_grid_view->layer());
}

TEST_F(AppListBubbleViewTest, ShowAnimationDestroysAndRestoresGradientMask) {
// Enable animations.
base::test::ScopedFeatureList feature(
features::kProductivityLauncherAnimation);
ui::ScopedAnimationDurationScaleMode duration(
ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);

// Show an app list with enough apps to fill the page and trigger a gradient
// at the bottom.
AddAppItems(50);
ShowAppList();

// Gradient mask layer is suppressed during show animation for performance.
auto* scroll_view = GetAppsPage()->scroll_view();
EXPECT_FALSE(scroll_view->layer()->layer_mask_layer());

// Finish the animation.
auto* apps_grid_view = GetAppsGridView();
WaitForLayerAnimation(apps_grid_view->layer());

// Gradient mask layer is restored.
EXPECT_TRUE(scroll_view->layer()->layer_mask_layer());
}

TEST_F(AppListBubbleViewTest, ShowAnimationRecordsSmoothnessHistogram) {
base::HistogramTester histograms;

Expand Down
18 changes: 16 additions & 2 deletions ash/controls/scroll_view_gradient_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,17 @@ ScrollViewGradientHelper::ScrollViewGradientHelper(

ScrollViewGradientHelper::~ScrollViewGradientHelper() = default;

void ScrollViewGradientHelper::HideGradient(bool hide) {
hide_gradient_ = hide;
UpdateGradientZone();
}

void ScrollViewGradientHelper::UpdateGradientZone() {
if (hide_gradient_) {
RemoveMaskLayer();
return;
}

const gfx::Rect visible_rect = scroll_view_->GetVisibleRect();
// Show the top gradient if the scroll view is not scrolled to the top.
const bool show_top_gradient = visible_rect.y() > 0;
Expand All @@ -61,8 +71,7 @@ void ScrollViewGradientHelper::UpdateGradientZone() {

// If no gradient is needed, remove the mask layer.
if (top_gradient_bounds.IsEmpty() && bottom_gradient_bounds.IsEmpty()) {
scroll_view_->layer()->SetMaskLayer(nullptr);
gradient_layer_.reset();
RemoveMaskLayer();
return;
}

Expand All @@ -87,4 +96,9 @@ void ScrollViewGradientHelper::UpdateGradientZone() {
scroll_view_->SchedulePaint();
}

void ScrollViewGradientHelper::RemoveMaskLayer() {
scroll_view_->layer()->SetMaskLayer(nullptr);
gradient_layer_.reset();
}

} // namespace ash
10 changes: 10 additions & 0 deletions ash/controls/scroll_view_gradient_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,24 @@ class ASH_EXPORT ScrollViewGradientHelper {
// Updates the gradients based on `scroll_view_` bounds and scroll position.
void UpdateGradientZone();

// When `hide` is true, hides the gradient mask and destroys its layer. This
// may improve animation performance if the layer's bounds are changing.
void HideGradient(bool hide);

GradientLayerDelegate* gradient_layer_for_test() {
return gradient_layer_.get();
}

private:
// Removes the scroll view mask layer.
void RemoveMaskLayer();

// The scroll view being decorated.
views::ScrollView* const scroll_view_;

// When true, the gradient is forced hidden.
bool hide_gradient_ = false;

// Draws the fade in/out gradients via a `scroll_view_` mask layer.
std::unique_ptr<GradientLayerDelegate> gradient_layer_;

Expand Down
25 changes: 25 additions & 0 deletions ash/controls/scroll_view_gradient_helper_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,5 +128,30 @@ TEST_F(ScrollViewGradientHelperTest, ShowsGradientsBasedOnScrollPosition) {
EXPECT_FALSE(HasGradientAtBottom());
}

TEST_F(ScrollViewGradientHelperTest, HideGradientRemovesMaskLayer) {
// Create a tall contents view.
AddScrollViewContentsWithHeight(500);
gradient_helper_->UpdateGradientZone();

// Precondition: Mask layer and delegate exist.
ASSERT_TRUE(scroll_view_->layer()->layer_mask_layer());
ASSERT_TRUE(gradient_helper_->gradient_layer_for_test());

// Hiding the gradient removes the layer and delegate.
gradient_helper_->HideGradient(true);
EXPECT_FALSE(scroll_view_->layer()->layer_mask_layer());
EXPECT_FALSE(gradient_helper_->gradient_layer_for_test());

// Updating the gradient does not restore the layer.
gradient_helper_->UpdateGradientZone();
EXPECT_FALSE(scroll_view_->layer()->layer_mask_layer());
EXPECT_FALSE(gradient_helper_->gradient_layer_for_test());

// Showing the gradient restores the layer and delegate.
gradient_helper_->HideGradient(false);
EXPECT_TRUE(scroll_view_->layer()->layer_mask_layer());
EXPECT_TRUE(gradient_helper_->gradient_layer_for_test());
}

} // namespace
} // namespace ash

0 comments on commit aff9649

Please sign in to comment.