From d2ca3e8488453da2b6f8f6054323dcdaa0b128e5 Mon Sep 17 00:00:00 2001 From: calamity Date: Mon, 11 May 2015 03:08:48 -0700 Subject: [PATCH] Make the ChromeOS app list button trigger on mouse press. This CL makes the app list button trigger on mouse press by adding a set_notify_action() method to CustomButton which lets clients choose when the ButtonListener should be notified. This will fix an issue with the app list reshowing on slow mouse presses and decrease perceived app list startup time. BUG=484860 Review URL: https://codereview.chromium.org/1129733002 Cr-Commit-Position: refs/heads/master@{#329132} --- ash/shelf/app_list_button.cc | 1 + ui/views/controls/button/custom_button.cc | 10 +- ui/views/controls/button/custom_button.h | 14 ++ ui/views/controls/button/custom_button_unittest.cc | 180 +++++++++++++-------- 4 files changed, 138 insertions(+), 67 deletions(-) diff --git a/ash/shelf/app_list_button.cc b/ash/shelf/app_list_button.cc index 8ae3c78985b0..acb0dd3e723f 100644 --- a/ash/shelf/app_list_button.cc +++ b/ash/shelf/app_list_button.cc @@ -47,6 +47,7 @@ AppListButton::AppListButton(views::ButtonListener* listener, SetSize(gfx::Size(kShelfSize, kShelfSize)); SetFocusPainter(views::Painter::CreateSolidFocusPainter( kFocusBorderColor, gfx::Insets(1, 1, 1, 1))); + set_notify_action(CustomButton::NOTIFY_ON_PRESS); } AppListButton::~AppListButton() { diff --git a/ui/views/controls/button/custom_button.cc b/ui/views/controls/button/custom_button.cc index 0fa08f09be44..5040c4ea9cf1 100644 --- a/ui/views/controls/button/custom_button.cc +++ b/ui/views/controls/button/custom_button.cc @@ -128,6 +128,11 @@ bool CustomButton::OnMousePressed(const ui::MouseEvent& event) { SetState(STATE_PRESSED); if (request_focus_on_press_) RequestFocus(); + if (IsTriggerableEvent(event) && notify_action_ == NOTIFY_ON_PRESS) { + NotifyClick(event); + // NOTE: We may be deleted at this point (by the listener's notification + // handler). + } } return true; } @@ -152,7 +157,7 @@ void CustomButton::OnMouseReleased(const ui::MouseEvent& event) { } SetState(STATE_HOVERED); - if (IsTriggerableEvent(event)) { + if (IsTriggerableEvent(event) && notify_action_ == NOTIFY_ON_RELEASE) { NotifyClick(event); // NOTE: We may be deleted at this point (by the listener's notification // handler). @@ -316,7 +321,8 @@ CustomButton::CustomButton(ButtonListener* listener) animate_on_state_change_(true), is_throbbing_(false), triggerable_event_flags_(ui::EF_LEFT_MOUSE_BUTTON), - request_focus_on_press_(true) { + request_focus_on_press_(true), + notify_action_(NOTIFY_ON_RELEASE) { hover_animation_.reset(new gfx::ThrobAnimation(this)); hover_animation_->SetSlideDuration(kHoverFadeDurationMs); } diff --git a/ui/views/controls/button/custom_button.h b/ui/views/controls/button/custom_button.h index f356c76b8a08..8afb6d6fe3ea 100644 --- a/ui/views/controls/button/custom_button.h +++ b/ui/views/controls/button/custom_button.h @@ -23,6 +23,12 @@ namespace views { class VIEWS_EXPORT CustomButton : public Button, public gfx::AnimationDelegate { public: + // An enum describing the events on which a button should notify its listener. + enum NotifyAction { + NOTIFY_ON_PRESS, + NOTIFY_ON_RELEASE, + }; + // The menu button's class name. static const char kViewClassName[]; @@ -61,6 +67,11 @@ class VIEWS_EXPORT CustomButton : public Button, animate_on_state_change_ = value; } + // Sets the event on which the button should notify its listener. + void set_notify_action(NotifyAction notify_action) { + notify_action_ = notify_action; + } + void SetHotTracked(bool is_hot_tracked); bool IsHotTracked() const; @@ -130,6 +141,9 @@ class VIEWS_EXPORT CustomButton : public Button, // See description above setter. bool request_focus_on_press_; + // The event on which the button should notify its listener. + NotifyAction notify_action_; + DISALLOW_COPY_AND_ASSIGN(CustomButton); }; diff --git a/ui/views/controls/button/custom_button_unittest.cc b/ui/views/controls/button/custom_button_unittest.cc index 70841f982baa..7a8fcc04949d 100644 --- a/ui/views/controls/button/custom_button_unittest.cc +++ b/ui/views/controls/button/custom_button_unittest.cc @@ -24,15 +24,25 @@ namespace views { namespace { -class TestCustomButton : public CustomButton { +class TestCustomButton : public CustomButton, public ButtonListener { public: - explicit TestCustomButton(ButtonListener* listener) - : CustomButton(listener) { + explicit TestCustomButton() + : CustomButton(this) { } ~TestCustomButton() override {} + void ButtonPressed(Button* sender, const ui::Event& event) override { + notified_ = true; + } + + bool notified() { return notified_; } + + void Reset() { notified_ = false; } + private: + bool notified_ = false; + DISALLOW_COPY_AND_ASSIGN(TestCustomButton); }; @@ -43,101 +53,141 @@ void PerformGesture(CustomButton* button, ui::EventType event_type) { button->OnGestureEvent(&gesture_event); } -} // namespace +class CustomButtonTest : public ViewsTestBase { + public: + CustomButtonTest() {} + ~CustomButtonTest() override {} + + void SetUp() override { + ViewsTestBase::SetUp(); + + // Create a widget so that the CustomButton can query the hover state + // correctly. + widget_.reset(new Widget); + Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP); + params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET; + params.bounds = gfx::Rect(0, 0, 650, 650); + widget_->Init(params); + widget_->Show(); + + // Position the widget in a way so that it is under the cursor. + gfx::Point cursor = gfx::Screen::GetScreenFor( + widget_->GetNativeView())->GetCursorScreenPoint(); + gfx::Rect widget_bounds = widget_->GetWindowBoundsInScreen(); + widget_bounds.set_origin(cursor); + widget_->SetBounds(widget_bounds); + + button_ = new TestCustomButton(); + widget_->SetContentsView(button_); + } -typedef ViewsTestBase CustomButtonTest; + Widget* widget() { return widget_.get(); } + TestCustomButton* button() { return button_; } -// Tests that hover state changes correctly when visiblity/enableness changes. -TEST_F(CustomButtonTest, HoverStateOnVisibilityChange) { - // Create a widget so that the CustomButton can query the hover state - // correctly. - scoped_ptr widget(new Widget); - Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP); - params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET; - params.bounds = gfx::Rect(0, 0, 650, 650); - widget->Init(params); - widget->Show(); + private: + scoped_ptr widget_; + TestCustomButton* button_; - aura::test::TestCursorClient cursor_client( - widget->GetNativeView()->GetRootWindow()); + DISALLOW_COPY_AND_ASSIGN(CustomButtonTest); +}; - // Position the widget in a way so that it is under the cursor. - gfx::Point cursor = gfx::Screen::GetScreenFor( - widget->GetNativeView())->GetCursorScreenPoint(); - gfx::Rect widget_bounds = widget->GetWindowBoundsInScreen(); - widget_bounds.set_origin(cursor); - widget->SetBounds(widget_bounds); +} // namespace - TestCustomButton* button = new TestCustomButton(NULL); - widget->SetContentsView(button); +// Tests that hover state changes correctly when visiblity/enableness changes. +TEST_F(CustomButtonTest, HoverStateOnVisibilityChange) { + aura::test::TestCursorClient cursor_client( + widget()->GetNativeView()->GetRootWindow()); gfx::Point center(10, 10); - button->OnMousePressed(ui::MouseEvent( + button()->OnMousePressed(ui::MouseEvent( ui::ET_MOUSE_PRESSED, center, center, ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON)); - EXPECT_EQ(CustomButton::STATE_PRESSED, button->state()); + EXPECT_EQ(CustomButton::STATE_PRESSED, button()->state()); - button->OnMouseReleased(ui::MouseEvent( + button()->OnMouseReleased(ui::MouseEvent( ui::ET_MOUSE_RELEASED, center, center, ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON)); - EXPECT_EQ(CustomButton::STATE_HOVERED, button->state()); + EXPECT_EQ(CustomButton::STATE_HOVERED, button()->state()); - button->SetEnabled(false); - EXPECT_EQ(CustomButton::STATE_DISABLED, button->state()); + button()->SetEnabled(false); + EXPECT_EQ(CustomButton::STATE_DISABLED, button()->state()); - button->SetEnabled(true); - EXPECT_EQ(CustomButton::STATE_HOVERED, button->state()); + button()->SetEnabled(true); + EXPECT_EQ(CustomButton::STATE_HOVERED, button()->state()); - button->SetVisible(false); - EXPECT_EQ(CustomButton::STATE_NORMAL, button->state()); + button()->SetVisible(false); + EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state()); - button->SetVisible(true); - EXPECT_EQ(CustomButton::STATE_HOVERED, button->state()); + button()->SetVisible(true); + EXPECT_EQ(CustomButton::STATE_HOVERED, button()->state()); // In Aura views, no new hover effects are invoked if mouse events // are disabled. cursor_client.DisableMouseEvents(); - button->SetEnabled(false); - EXPECT_EQ(CustomButton::STATE_DISABLED, button->state()); + button()->SetEnabled(false); + EXPECT_EQ(CustomButton::STATE_DISABLED, button()->state()); - button->SetEnabled(true); - EXPECT_EQ(CustomButton::STATE_NORMAL, button->state()); + button()->SetEnabled(true); + EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state()); - button->SetVisible(false); - EXPECT_EQ(CustomButton::STATE_NORMAL, button->state()); + button()->SetVisible(false); + EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state()); - button->SetVisible(true); - EXPECT_EQ(CustomButton::STATE_NORMAL, button->state()); + button()->SetVisible(true); + EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state()); +} + +// Tests the different types of NotifyActions. +TEST_F(CustomButtonTest, NotifyAction) { + gfx::Point center(10, 10); + + // By default the button should notify its listener on mouse release. + button()->OnMousePressed(ui::MouseEvent( + ui::ET_MOUSE_PRESSED, center, center, ui::EventTimeForNow(), + ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON)); + EXPECT_EQ(CustomButton::STATE_PRESSED, button()->state()); + EXPECT_FALSE(button()->notified()); + + button()->OnMouseReleased(ui::MouseEvent( + ui::ET_MOUSE_RELEASED, center, center, ui::EventTimeForNow(), + ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON)); + EXPECT_EQ(CustomButton::STATE_HOVERED, button()->state()); + EXPECT_TRUE(button()->notified()); + + // Set the notify action to its listener on mouse press. + button()->Reset(); + button()->set_notify_action(CustomButton::NOTIFY_ON_PRESS); + button()->OnMousePressed(ui::MouseEvent( + ui::ET_MOUSE_PRESSED, center, center, ui::EventTimeForNow(), + ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON)); + EXPECT_EQ(CustomButton::STATE_PRESSED, button()->state()); + EXPECT_TRUE(button()->notified()); + + // The button should no longer notify on mouse release. + button()->Reset(); + button()->OnMouseReleased(ui::MouseEvent( + ui::ET_MOUSE_RELEASED, center, center, ui::EventTimeForNow(), + ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON)); + EXPECT_EQ(CustomButton::STATE_HOVERED, button()->state()); + EXPECT_FALSE(button()->notified()); } // Tests that gesture events correctly change the button state. TEST_F(CustomButtonTest, GestureEventsSetState) { - // Create a widget so that the CustomButton can query the hover state - // correctly. - scoped_ptr widget(new Widget); - Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP); - params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET; - params.bounds = gfx::Rect(0, 0, 650, 650); - widget->Init(params); - widget->Show(); - aura::test::TestCursorClient cursor_client( - widget->GetNativeView()->GetRootWindow()); - - TestCustomButton* button = new TestCustomButton(NULL); - widget->SetContentsView(button); + widget()->GetNativeView()->GetRootWindow()); - EXPECT_EQ(CustomButton::STATE_NORMAL, button->state()); + EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state()); - PerformGesture(button, ui::ET_GESTURE_TAP_DOWN); - EXPECT_EQ(CustomButton::STATE_PRESSED, button->state()); + PerformGesture(button(), ui::ET_GESTURE_TAP_DOWN); + EXPECT_EQ(CustomButton::STATE_PRESSED, button()->state()); - PerformGesture(button, ui::ET_GESTURE_SHOW_PRESS); - EXPECT_EQ(CustomButton::STATE_PRESSED, button->state()); + PerformGesture(button(), ui::ET_GESTURE_SHOW_PRESS); + EXPECT_EQ(CustomButton::STATE_PRESSED, button()->state()); - PerformGesture(button, ui::ET_GESTURE_TAP_CANCEL); - EXPECT_EQ(CustomButton::STATE_NORMAL, button->state()); + PerformGesture(button(), ui::ET_GESTURE_TAP_CANCEL); + EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state()); } // Ensure subclasses of CustomButton are correctly recognized as CustomButton. -- 2.11.4.GIT