cros: Implement improved stylus mode exiting.

This CL does the following:
1) If we choose a tool that is a "mode", the palette tray stays active.
2) If a tool that is a "mode" is active, tapping the palette tray deactives the tool.
3) Remove the checkmarks (because they can't be seen anymore with #2).
4) Add a seperator between tools that are "actions" and tools that are "modes".

Test: ash_unittests --gtest_filter="PaletteTrayTest.*"
Bug: 742524
Change-Id: I70d6e07a460250e7a22059973de036146dd753a6
Reviewed-on: https://chromium-review.googlesource.com/572066
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488108}

ash/BUILD.gn

@@ -1227,6 +1227,7 @@ source_set("common_unittests") {
     "system/palette/mock_palette_tool_delegate.cc",
     "system/palette/mock_palette_tool_delegate.h",
     "system/palette/palette_tool_manager_unittest.cc",
+    "system/palette/palette_tray_unittest.cc",
     "system/palette/tools/create_note_unittest.cc",
     "system/palette/tools/metalayer_unittest.cc",
     "system/palette/tools/screenshot_unittest.cc",

ash/system/palette/common_palette_tool.cc

@@ -49,30 +49,23 @@ void CommonPaletteTool::OnViewDestroyed() {
 void CommonPaletteTool::OnEnable() {
   PaletteTool::OnEnable();
   start_time_ = base::TimeTicks::Now();
-
-  if (highlight_view_)
-    TrayPopupUtils::UpdateCheckMarkVisibility(highlight_view_, true);
 }
 
 void CommonPaletteTool::OnDisable() {
   PaletteTool::OnDisable();
   AddHistogramTimes(GetToolId(), base::TimeTicks::Now() - start_time_);
-
-  if (highlight_view_)
-    TrayPopupUtils::UpdateCheckMarkVisibility(highlight_view_, false);
 }
 
 void CommonPaletteTool::OnViewClicked(views::View* sender) {
+  // The tool should always be disabled when we click it because the bubble
+  // which houses this view is automatically closed when the tool is first
+  // enabled. Then, to open the bubble again we have to click on the palette
+  // tray twice, and the first click will disable any active tools.
+  DCHECK(!enabled());
+
   delegate()->RecordPaletteOptionsUsage(
       PaletteToolIdToPaletteTrayOptions(GetToolId()));
-  if (enabled()) {
-    delegate()->DisableTool(GetToolId());
-    delegate()->RecordPaletteModeCancellation(
-        PaletteToolIdToPaletteModeCancelType(GetToolId(),
-                                             false /*is_switched*/));
-  } else {
-    delegate()->EnableTool(GetToolId());
-  }
+  delegate()->EnableTool(GetToolId());
 }
 
 views::View* CommonPaletteTool::CreateDefaultView(const base::string16& name) {
@@ -80,7 +73,6 @@ views::View* CommonPaletteTool::CreateDefaultView(const base::string16& name) {
       CreateVectorIcon(GetPaletteIcon(), kMenuIconSize, gfx::kChromeIconGrey);
   highlight_view_ = new HoverHighlightView(this);
   highlight_view_->AddIconAndLabel(icon, name);
-  TrayPopupUtils::InitializeAsCheckableRow(highlight_view_, enabled());
   return highlight_view_;
 }
 

ash/system/palette/palette_tool_manager.cc

@@ -82,18 +82,21 @@ const gfx::VectorIcon& PaletteToolManager::GetActiveTrayIcon(
   return tool->GetActiveTrayIcon();
 }
 
-std::vector<PaletteToolView> PaletteToolManager::CreateViews() {
+std::vector<PaletteToolView> PaletteToolManager::CreateViewsForGroup(
+    PaletteGroup group) {
   std::vector<PaletteToolView> views;
-  views.reserve(tools_.size());
 
-  for (size_t i = 0; i < tools_.size(); ++i) {
-    views::View* tool_view = tools_[i]->CreateView();
+  for (const auto& tool : tools_) {
+    if (tool->GetGroup() != group)
+      continue;
+
+    views::View* tool_view = tool->CreateView();
     if (!tool_view)
       continue;
 
     PaletteToolView view;
-    view.group = tools_[i]->GetGroup();
-    view.tool_id = tools_[i]->GetToolId();
+    view.group = tool->GetGroup();
+    view.tool_id = tool->GetToolId();
     view.view = tool_view;
     views.push_back(view);
   }

ash/system/palette/palette_tool_manager.h

@@ -86,8 +86,8 @@ class ASH_EXPORT PaletteToolManager : public PaletteTool::Delegate {
   // not available.
   const gfx::VectorIcon& GetActiveTrayIcon(PaletteToolId tool_id) const;
 
-  // Create views for all of the registered tools.
-  std::vector<PaletteToolView> CreateViews();
+  // Create views for all of the registered mode tools with group |group|.
+  std::vector<PaletteToolView> CreateViewsForGroup(PaletteGroup group);
 
   // Called when the views returned by CreateViews have been destroyed. This
   // should clear any (now) stale references.

ash/system/palette/palette_tray.cc

@@ -234,7 +234,9 @@ void PaletteTray::OnStylusStateChanged(ui::StylusState stylus_state) {
 
 void PaletteTray::BubbleViewDestroyed() {
   palette_tool_manager_->NotifyViewsDestroyed();
-  SetIsActive(false);
+  // The tray button remains active if the current active tool is a mode.
+  SetIsActive(palette_tool_manager_->GetActiveTool(PaletteGroup::MODE) !=
+              PaletteToolId::NONE);
 }
 
 void PaletteTray::OnMouseEnteredView() {}
@@ -334,6 +336,19 @@ bool PaletteTray::PerformAction(const ui::Event& event) {
     return true;
   }
 
+  // Deactivate the active tool if there is one.
+  PaletteToolId active_tool_id =
+      palette_tool_manager_->GetActiveTool(PaletteGroup::MODE);
+  if (active_tool_id != PaletteToolId::NONE) {
+    palette_tool_manager_->DeactivateTool(active_tool_id);
+    // TODO(sammiequon): Investigate whether we should removed |is_switched|
+    // from PaletteToolIdToPaletteModeCancelType.
+    RecordPaletteModeCancellation(PaletteToolIdToPaletteModeCancelType(
+        active_tool_id, false /*is_switched*/));
+    SetIsActive(false);
+    return true;
+  }
+
   ShowBubble();
   return true;
 }
@@ -373,18 +388,33 @@ void PaletteTray::ShowBubble() {
       gfx::Insets(0, kPaddingBetweenTitleAndLeftEdge, 0, 0)));
   bubble_view->AddChildView(title_view);
 
-  // Add horizontal separator.
-  views::Separator* separator = new views::Separator();
-  separator->SetColor(kPaletteSeparatorColor);
-  separator->SetBorder(views::CreateEmptyBorder(gfx::Insets(
-      kPaddingBetweenTitleAndSeparator, 0, kMenuSeparatorVerticalPadding, 0)));
-  bubble_view->AddChildView(separator);
+  // Function for creating a separator.
+  auto build_separator = []() {
+    auto* separator = new views::Separator();
+    separator->SetColor(kPaletteSeparatorColor);
+    separator->SetBorder(views::CreateEmptyBorder(
+        gfx::Insets(kPaddingBetweenTitleAndSeparator, 0,
+                    kMenuSeparatorVerticalPadding, 0)));
+    return separator;
+  };
+
+  // Add horizontal separator between the title and the tools.
+  bubble_view->AddChildView(build_separator());
 
   // Add palette tools.
   // TODO(tdanderson|jdufault): Use SystemMenuButton to get the material design
   // ripples.
-  std::vector<PaletteToolView> views = palette_tool_manager_->CreateViews();
-  for (const PaletteToolView& view : views)
+  std::vector<PaletteToolView> action_views =
+      palette_tool_manager_->CreateViewsForGroup(PaletteGroup::ACTION);
+  for (const PaletteToolView& view : action_views)
+    bubble_view->AddChildView(view.view);
+
+  // Add horizontal separator between action tools and mode tools.
+  bubble_view->AddChildView(build_separator());
+
+  std::vector<PaletteToolView> mode_views =
+      palette_tool_manager_->CreateViewsForGroup(PaletteGroup::MODE);
+  for (const PaletteToolView& view : mode_views)
     bubble_view->AddChildView(view.view);
 
   // Show the bubble.
@@ -399,7 +429,7 @@ views::TrayBubbleView* PaletteTray::GetBubbleView() {
 void PaletteTray::UpdateTrayIcon() {
   icon_->SetImage(CreateVectorIcon(
       palette_tool_manager_->GetActiveTrayIcon(
-          palette_tool_manager_->GetActiveTool(ash::PaletteGroup::MODE)),
+          palette_tool_manager_->GetActiveTool(PaletteGroup::MODE)),
       kTrayIconSize, kShelfIconColor));
 }
 
@@ -419,4 +449,12 @@ void PaletteTray::UpdateIconVisibility() {
              ShouldShowOnDisplay(this) && IsInUserSession());
 }
 
+// TestApi. For testing purposes.
+PaletteTray::TestApi::TestApi(PaletteTray* palette_tray)
+    : palette_tray_(palette_tray) {
+  DCHECK(palette_tray_);
+}
+
+PaletteTray::TestApi::~TestApi() {}
+
 }  // namespace ash

ash/system/palette/palette_tray.h

@@ -27,8 +27,8 @@ class ImageView;
 
 namespace ash {
 
-class TrayBubbleWrapper;
 class PaletteToolManager;
+class TrayBubbleWrapper;
 
 // The PaletteTray shows the palette in the bottom area of the screen. This
 // class also controls the lifetime for all of the tools available in the
@@ -40,6 +40,26 @@ class ASH_EXPORT PaletteTray : public TrayBackgroundView,
                                public PaletteToolManager::Delegate,
                                public ui::InputDeviceEventObserver {
  public:
+  // For testing.
+  class TestApi {
+   public:
+    explicit TestApi(PaletteTray* palette_tray);
+    ~TestApi();
+
+    PaletteToolManager* GetPaletteToolManager() {
+      return palette_tray_->palette_tool_manager_.get();
+    }
+
+    TrayBubbleWrapper* GetTrayBubbleWrapper() {
+      return palette_tray_->bubble_.get();
+    }
+
+   private:
+    PaletteTray* palette_tray_ = nullptr;  // not owned
+
+    DISALLOW_COPY_AND_ASSIGN(TestApi);
+  };
+
   explicit PaletteTray(Shelf* shelf);
   ~PaletteTray() override;
 

ash/system/palette/palette_tray_unittest.cc

+// Copyright 2017 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 "ash/system/palette/palette_tray.h"
+
+#include "ash/ash_switches.h"
+#include "ash/shell_test_api.h"
+#include "ash/system/palette/test_palette_delegate.h"
+#include "ash/system/status_area_widget.h"
+#include "ash/system/status_area_widget_test_helper.h"
+#include "ash/test/ash_test_base.h"
+#include "base/command_line.h"
+#include "ui/events/event.h"
+
+namespace ash {
+
+class PaletteTrayTest : public AshTestBase {
+ public:
+  PaletteTrayTest() {}
+  ~PaletteTrayTest() override {}
+
+  void SetUp() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kAshForceEnableStylusTools);
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kAshEnablePaletteOnAllDisplays);
+
+    AshTestBase::SetUp();
+
+    palette_tray_ =
+        StatusAreaWidgetTestHelper::GetStatusAreaWidget()->palette_tray();
+    test_api_ = base::MakeUnique<PaletteTray::TestApi>(palette_tray_);
+
+    ShellTestApi().SetPaletteDelegate(base::MakeUnique<TestPaletteDelegate>());
+  }
+
+  // Performs a tap on the palette tray button.
+  void PerformTap() {
+    ui::GestureEvent tap(0, 0, 0, base::TimeTicks(),
+                         ui::GestureEventDetails(ui::ET_GESTURE_TAP));
+    palette_tray_->PerformAction(tap);
+  }
+
+ protected:
+  PaletteTray* palette_tray_ = nullptr;  // not owned
+
+  std::unique_ptr<PaletteTray::TestApi> test_api_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PaletteTrayTest);
+};
+
+// Verify the palette tray button exists and is visible with the flags we added
+// during setup.
+TEST_F(PaletteTrayTest, PaletteTrayIsVisible) {
+  ASSERT_TRUE(palette_tray_);
+  EXPECT_TRUE(palette_tray_->visible());
+}
+
+// Verify taps on the palette tray button results in expected behaviour.
+TEST_F(PaletteTrayTest, PaletteTrayWorkflow) {
+  // Verify the palette tray button is not active, and the palette tray bubble
+  // is not shown initially.
+  EXPECT_FALSE(palette_tray_->is_active());
+  EXPECT_FALSE(test_api_->GetTrayBubbleWrapper());
+
+  // Verify that by tapping the palette tray button, the button will become
+  // active and the palette tray bubble will be open.
+  PerformTap();
+  EXPECT_TRUE(palette_tray_->is_active());
+  EXPECT_TRUE(test_api_->GetTrayBubbleWrapper());
+
+  // Verify that activating a mode tool will close the palette tray bubble, but
+  // leave the palette tray button active.
+  test_api_->GetPaletteToolManager()->ActivateTool(
+      PaletteToolId::LASER_POINTER);
+  EXPECT_TRUE(test_api_->GetPaletteToolManager()->IsToolActive(
+      PaletteToolId::LASER_POINTER));
+  EXPECT_TRUE(palette_tray_->is_active());
+  EXPECT_FALSE(test_api_->GetTrayBubbleWrapper());
+
+  // Verify that tapping the palette tray while a tool is active will deactivate
+  // the tool, and the palette tray button will not be active.
+  PerformTap();
+  EXPECT_FALSE(palette_tray_->is_active());
+  EXPECT_FALSE(test_api_->GetPaletteToolManager()->IsToolActive(
+      PaletteToolId::LASER_POINTER));
+
+  // Verify that activating a action tool will close the palette tray bubble and
+  // the palette tray button is will not be active.
+  PerformTap();
+  ASSERT_TRUE(test_api_->GetTrayBubbleWrapper());
+  test_api_->GetPaletteToolManager()->ActivateTool(
+      PaletteToolId::CAPTURE_SCREEN);
+  EXPECT_FALSE(test_api_->GetPaletteToolManager()->IsToolActive(
+      PaletteToolId::CAPTURE_SCREEN));
+  // Wait for the tray bubble widget to close.
+  RunAllPendingInMessageLoop();
+  EXPECT_FALSE(test_api_->GetTrayBubbleWrapper());
+  EXPECT_FALSE(palette_tray_->is_active());
+}
+
+}  // namespace ash