MD Buttons: Don't add layers for hidden -> hidden transitions.

ui/views/controls/button/custom_button_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/button/custom_button.h"
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/layout.h"
 #include "ui/display/screen.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/views/animation/ink_drop_host.h"
+#include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/animation/test/ink_drop_host_view_test_api.h"
 #include "ui/views/animation/test/test_ink_drop.h"
 #include "ui/views/animation/test/test_ink_drop_host.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/controls/button/checkbox.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/button/radio_button.h"
 #include "ui/views/controls/button/toggle_button.h"
@@ skipping 39 matching lines @@
   ~TestCustomButton() override {}
 
   void ButtonPressed(Button* sender, const ui::Event& event) override {
     pressed_ = true;
   }
 
   void OnClickCanceled(const ui::Event& event) override {
     canceled_ = true;
   }
 
+  // InkDropHostView:
+  void AddInkDropLayer(ui::Layer* ink_drop_layer) override {

[bruthig, 2016/10/18 14:31:26]

Feel free to add this behavior to the TestInkDropHost instead.
See
https://cs.chromium.org/chromium/src/ui/views/animation/test/test_ink_drop_ho...

[tapted, 2016/10/19 02:59:31]

Done.

+    ++ink_drop_layer_add_count_;
+    CustomButton::AddInkDropLayer(ink_drop_layer);
+  }
+  void RemoveInkDropLayer(ui::Layer* ink_drop_layer) override {
+    ++ink_drop_layer_remove_count_;
+    CustomButton::RemoveInkDropLayer(ink_drop_layer);
+  }
+
   bool pressed() { return pressed_; }
   bool canceled() { return canceled_; }
+  int ink_drop_layer_add_count() { return ink_drop_layer_add_count_; }
+  int ink_drop_layer_remove_count() { return ink_drop_layer_remove_count_; }
 
   void Reset() {
     pressed_ = false;
     canceled_ = false;
   }
 
   // Raised visibility of OnFocus() to public
   void OnFocus() override { CustomButton::OnFocus(); }
 
  private:
   bool pressed_ = false;
   bool canceled_ = false;
 
+  int ink_drop_layer_add_count_ = 0;
+  int ink_drop_layer_remove_count_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(TestCustomButton);
 };
 
 }  // namespace
 
 class CustomButtonTest : public ViewsTestBase {
  public:
   CustomButtonTest() {}
   ~CustomButtonTest() override {}
 
@@ skipping 19 matching lines @@
   }
 
   void CreateButtonWithInkDrop(std::unique_ptr<InkDrop> ink_drop,
                                bool has_ink_drop_action_on_click) {
     delete button_;
     button_ = new TestCustomButton(has_ink_drop_action_on_click);
     InkDropHostViewTestApi(button_).SetInkDrop(std::move(ink_drop));
     widget_->SetContentsView(button_);
   }
 
+  void CreateButtonWithRealInkDrop() {
+    delete button_;
+    button_ = new TestCustomButton(false);
+    InkDropHostViewTestApi(button_).SetInkDrop(
+        base::MakeUnique<InkDropImpl>(button_));
+    widget_->SetContentsView(button_);
+  }
+
  protected:
   Widget* widget() { return widget_.get(); }
   TestCustomButton* button() { return button_; }
   void SetDraggedView(View* dragged_view) {
     widget_->dragged_view_ = dragged_view;
   }
 
   void SimulateKeyEvent(ui::KeyEvent* event) { widget()->OnKeyEvent(event); }
 
  private:
@@ skipping 425 matching lines @@
 
   button()->OnMouseDragged(ui::MouseEvent(
       ui::ET_MOUSE_PRESSED, center, center, ui::EventTimeForNow(),
       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
 
   EXPECT_EQ(InkDropState::HIDDEN, ink_drop->GetTargetInkDropState());
 
   SetDraggedView(nullptr);
 }
 
+// Test that hiding an already-hidden ink drop doesn't add layers.
+TEST_F(CustomButtonTest, AlwaysHiddenInkDropHasNoLayers) {

[bruthig, 2016/10/18 14:31:26]

I think this test would make more sense in InkDropImplTest and we should
probably add one that verifies a layer is not added when SetHover(false) is
called when the highlight is already hidden.

[tapted, 2016/10/19 02:59:31]

Hm - I wanted to include coverage of the NativeWidgetMac codepath, since that
was where the DCHECK was being hit. I guess in reality that was during Close()
codepaths, but it was feeling a bit like test overkill. However, moving the
AnimateToState(hidden) stuff to InkDropImplTest makes it neater, so I've updated
the test here to test the Close() codepaths (e.g. it will now DCHECK on Mac
without the fix in ink_drop_impl.cc whereas the new InkDropImplTest will only
fail the test expecations) - see what you think.

+  CreateButtonWithRealInkDrop();
+
+  EXPECT_TRUE(button()->visible());
+  EXPECT_FALSE(button()->layer());
+
+  button()->SetVisible(false);
+  EXPECT_FALSE(button()->visible());
+
+  // The animation may immediately end and remove the layer. So count calls as
+  // well as checking for the presence of a layer.
+  EXPECT_FALSE(button()->layer());
+  EXPECT_EQ(0, button()->ink_drop_layer_add_count());
+  EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
+
+  // Sanity check that making the button visible and clicking it should add a
+  // layer for the ripple.
+  button()->SetVisible(true);
+  EXPECT_TRUE(button()->visible());
+
+  // No expected change when just making the button visible.
+  EXPECT_FALSE(button()->layer());
+  EXPECT_EQ(0, button()->ink_drop_layer_add_count());
+  EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
+
+  ui::test::EventGenerator generator(widget()->GetNativeWindow());
+  generator.PressLeftButton();
+  EXPECT_TRUE(button()->layer());
+  EXPECT_EQ(1, button()->ink_drop_layer_add_count());
+
+  // The animation is running here, so the layer won't be removed.
+  EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
+
+  // This test doesn't wait for the animation to finish.
+}
+
 // Todo(karandeepb): On Mac, a button should get clicked on a Space key press
 // (and not release). Modify this test after fixing crbug.com/607429.
 // Test that Space Key behaves correctly on a focused button.
 TEST_F(CustomButtonTest, ClickOnSpace) {
   // Give focus to the button.
   button()->SetFocusForPlatform();
   button()->RequestFocus();
   EXPECT_EQ(button(), widget()->GetFocusManager()->GetFocusedView());
 
   ui::KeyEvent space_press(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, ui::EF_NONE);
   SimulateKeyEvent(&space_press);
   EXPECT_EQ(CustomButton::STATE_PRESSED, button()->state());
   EXPECT_FALSE(button()->pressed());
 
   ui::KeyEvent space_release(ui::ET_KEY_RELEASED, ui::VKEY_SPACE, ui::EF_NONE);
   SimulateKeyEvent(&space_release);
   EXPECT_EQ(CustomButton::STATE_NORMAL, button()->state());
   EXPECT_TRUE(button()->pressed());
 }
 
 }  // namespace views