Update placeholder for pinned files section.

The placeholder text needs to be updated and a chip added which allows
the user to navigate to the Files app. Note that the UX spec has not yet
been delivered so font sizes, spacing, etc. are subject to change. Also
note that the Files app icon still needs to be plugged in.

This CL also fixes some focus rings which were incorrectly colored.

Screenshot: http://shortn/_0FB67SM4PA

Bug: 1159527
Change-Id: I88e1e7cbc90178f558d2d3490bbd10a358f72c70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2596218
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837855}

ash/ash_strings.grd

@@ -1012,7 +1012,10 @@ This file contains the strings for ash.
         Pinned
       </message>
       <message name="IDS_ASH_HOLDING_SPACE_PINNED_EMPTY_PROMPT" desc="Prompt shown in the pinned files area of the holding space bubble if the user has no pinned files.">
-        Quickly access your important files. Right-click or touch &amp; hold a file to pin it.
+        You can pin your important files here. Open Files app to get started.
+      </message>
+      <message name="IDS_ASH_HOLDING_SPACE_PINNED_FILES_APP_CHIP_TEXT" desc="Text for chip which opens the Files app shown in the pinned files area of the holding space bubble if the user has no pinned files.">
+        Open Files
       </message>
       <message name="IDS_ASH_HOLDING_SPACE_DOWNLOADS_TITLE" desc="Title of the downloads area in the holding space bubble.">
         Downloads

ash/ash_strings_grd/IDS_ASH_HOLDING_SPACE_PINNED_EMPTY_PROMPT.png.sha1

-0ad404e14ac69a7254a868558f7ca5c89489c141
+5564e74f519fc35d835da6581b9e847bdcbe233c
\ No newline at end of file

ash/ash_strings_grd/IDS_ASH_HOLDING_SPACE_PINNED_FILES_APP_CHIP_TEXT.png.sha1

+5ff162a5e6078da380edd7cec97eca7e94e388e9
\ No newline at end of file

ash/public/cpp/holding_space/holding_space_client.h

@@ -45,6 +45,10 @@ class ASH_PUBLIC_EXPORT HoldingSpaceClient {
   virtual void OpenItems(const std::vector<const HoldingSpaceItem*>& items,
                          SuccessCallback callback) = 0;
 
+  // Attempts to open the My Files folder.
+  // Success is returned via the supplied `callback`.
+  virtual void OpenMyFiles(SuccessCallback callback) = 0;
+
   // Attempts to show the specified holding space `item` in its folder.
   // Success is returned via the supplied `callback`.
   virtual void ShowItemInFolder(const HoldingSpaceItem& item,

ash/public/cpp/holding_space/holding_space_constants.h

@@ -55,12 +55,13 @@ enum HoldingSpaceCommandId {
 };
 
 // View IDs.
-constexpr int kHoldingSpaceItemPinButtonId = 1;
-constexpr int kHoldingSpacePinnedFilesBubbleId = 2;
-constexpr int kHoldingSpaceRecentFilesBubbleId = 3;
-constexpr int kHoldingSpaceScreenCapturePlayIconId = 4;
-constexpr int kHoldingSpaceTrayDefaultIconId = 5;
-constexpr int kHoldingSpaceTrayPreviewsIconId = 6;
+constexpr int kHoldingSpaceFilesAppChipId = 1;
+constexpr int kHoldingSpaceItemPinButtonId = 2;
+constexpr int kHoldingSpacePinnedFilesBubbleId = 3;
+constexpr int kHoldingSpaceRecentFilesBubbleId = 4;
+constexpr int kHoldingSpaceScreenCapturePlayIconId = 5;
+constexpr int kHoldingSpaceTrayDefaultIconId = 6;
+constexpr int kHoldingSpaceTrayPreviewsIconId = 7;
 
 // The maximum allowed age for files restored into the holding space model.
 // Note that this is not enforced for pinned items.

ash/public/cpp/holding_space/holding_space_test_api.h

@@ -69,6 +69,9 @@ class ASH_EXPORT HoldingSpaceTestApi {
   // which displays previews of most recent items added to holding space.
   views::View* GetPreviewsTrayIcon();
 
+  // Returns the pinned files bubble.
+  views::View* GetPinnedFilesBubble();
+
   // Returns whether the pinned files bubble is shown.
   bool PinnedFilesBubbleShown() const;
 

ash/system/holding_space/downloads_section.cc

@@ -38,6 +38,11 @@ class Header : public views::Button {
     SetCallback(
         base::BindRepeating(&Header::OnPressed, base::Unretained(this)));
 
+    // Focus ring.
+    AshColorProvider* const ash_color_provider = AshColorProvider::Get();
+    focus_ring()->SetColor(ash_color_provider->GetControlsLayerColor(
+        AshColorProvider::ControlsLayerType::kFocusRingColor));
+
     auto* layout = SetLayoutManager(std::make_unique<views::BoxLayout>(
         views::BoxLayout::Orientation::kHorizontal, gfx::Insets(),
         kHoldingSpaceDownloadsHeaderSpacing));
@@ -45,7 +50,7 @@ class Header : public views::Button {
     // Label.
     auto* label = AddChildView(std::make_unique<views::Label>(
         l10n_util::GetStringUTF16(IDS_ASH_HOLDING_SPACE_DOWNLOADS_TITLE)));
-    label->SetEnabledColor(AshColorProvider::Get()->GetContentLayerColor(
+    label->SetEnabledColor(ash_color_provider->GetContentLayerColor(
         AshColorProvider::ContentLayerType::kTextColorPrimary));
     label->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
     layout->SetFlexForView(label, 1);
@@ -58,7 +63,7 @@ class Header : public views::Button {
     chevron->SetFlipCanvasOnPaintForRTLUI(true);
     chevron->SetImage(gfx::CreateVectorIcon(
         kChevronRightIcon, kHoldingSpaceDownloadsChevronIconSize,
-        AshColorProvider::Get()->GetContentLayerColor(
+        ash_color_provider->GetContentLayerColor(
             AshColorProvider::ContentLayerType::kIconColorPrimary)));
   }
 

ash/system/holding_space/holding_space_item_views_section.cc

@@ -271,8 +271,7 @@ void HoldingSpaceItemViewsSection::MaybeAnimateOut() {
 void HoldingSpaceItemViewsSection::AnimateIn(
     ui::LayerAnimationObserver* observer) {
   if (views_by_item_id_.empty() && placeholder_) {
-    DCHECK(!placeholder_->GetVisible());
-    placeholder_->SetVisible(true);
+    DCHECK(placeholder_->GetVisible());
     return;
   }
   for (auto& view_by_item_id : views_by_item_id_)

ash/system/holding_space/holding_space_test_api.cc

@@ -140,6 +140,13 @@ views::View* HoldingSpaceTestApi::GetPreviewsTrayIcon() {
   return holding_space_tray_->GetViewByID(kHoldingSpaceTrayPreviewsIconId);
 }
 
+views::View* HoldingSpaceTestApi::GetPinnedFilesBubble() {
+  if (!holding_space_tray_->GetBubbleView())
+    return nullptr;
+  return holding_space_tray_->GetBubbleView()->GetViewByID(
+      kHoldingSpacePinnedFilesBubbleId);
+}
+
 bool HoldingSpaceTestApi::PinnedFilesBubbleShown() const {
   if (!holding_space_tray_->GetBubbleView())
     return false;

ash/system/holding_space/holding_space_tray_unittest.cc

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "ash/public/cpp/ash_features.h"
+#include "ash/public/cpp/holding_space/holding_space_client.h"
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_image.h"
@@ -21,6 +22,7 @@
 #include "base/strings/strcat.h"
 #include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "url/gurl.h"
@@ -31,13 +33,62 @@ namespace {
 
 constexpr char kTestUser[] = "user@test";
 
+// Helpers ---------------------------------------------------------------------
+
+void Click(views::View* view) {
+  auto* root_window = view->GetWidget()->GetNativeWindow()->GetRootWindow();
+  ui::test::EventGenerator event_generator(root_window);
+  event_generator.MoveMouseTo(view->GetBoundsInScreen().CenterPoint());
+  event_generator.ClickLeftButton();
+}
+
 std::unique_ptr<HoldingSpaceImage> CreateStubHoldingSpaceImage() {
   return std::make_unique<HoldingSpaceImage>(
       gfx::ImageSkia(), /*async_bitmap_resolver=*/base::DoNothing());
 }
 
+// Mocks -----------------------------------------------------------------------
+
+class MockHoldingSpaceClient : public HoldingSpaceClient {
+ public:
+  // HoldingSpaceClient:
+  MOCK_METHOD(void,
+              AddScreenshot,
+              (const base::FilePath& file_path),
+              (override));
+  MOCK_METHOD(void,
+              AddScreenRecording,
+              (const base::FilePath& file_path),
+              (override));
+  MOCK_METHOD(void,
+              CopyImageToClipboard,
+              (const HoldingSpaceItem& item, SuccessCallback callback),
+              (override));
+  MOCK_METHOD(void, OpenDownloads, (SuccessCallback callback), (override));
+  MOCK_METHOD(void, OpenMyFiles, (SuccessCallback callback), (override));
+  MOCK_METHOD(void,
+              OpenItems,
+              (const std::vector<const HoldingSpaceItem*>& items,
+               SuccessCallback callback),
+              (override));
+  MOCK_METHOD(void,
+              ShowItemInFolder,
+              (const HoldingSpaceItem& item, SuccessCallback callback),
+              (override));
+  MOCK_METHOD(void,
+              PinItems,
+              (const std::vector<const HoldingSpaceItem*>& items),
+              (override));
+  MOCK_METHOD(void,
+              UnpinItems,
+              (const std::vector<const HoldingSpaceItem*>& items),
+              (override));
+};
+
 }  // namespace
 
+// HoldingSpaceTrayTest --------------------------------------------------------
+
 // Parameterized by whether the previews feature is enabled.
 class HoldingSpaceTrayTest : public AshTestBase,
                              public testing::WithParamInterface<bool> {
@@ -63,7 +114,7 @@ class HoldingSpaceTrayTest : public AshTestBase,
     test_api_ = std::make_unique<HoldingSpaceTestApi>();
     AccountId user_account = AccountId::FromUserEmail(kTestUser);
     HoldingSpaceController::Get()->RegisterClientAndModelForUser(
-        user_account, /*client=*/nullptr, model());
+        user_account, client(), model());
     GetSessionControllerClient()->AddUserSession(kTestUser);
     holding_space_prefs::MarkTimeOfFirstAvailability(
         GetSessionControllerClient()->GetUserPrefService(user_account));
@@ -138,14 +189,21 @@ class HoldingSpaceTrayTest : public AshTestBase,
 
   HoldingSpaceTestApi* test_api() { return test_api_.get(); }
 
+  testing::NiceMock<MockHoldingSpaceClient>* client() {
+    return &holding_space_client_;
+  }
+
   HoldingSpaceModel* model() { return &holding_space_model_; }
 
  private:
   std::unique_ptr<HoldingSpaceTestApi> test_api_;
+  testing::NiceMock<MockHoldingSpaceClient> holding_space_client_;
   HoldingSpaceModel holding_space_model_;
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+// Tests -----------------------------------------------------------------------
+
 TEST_P(HoldingSpaceTrayTest, ShowTrayButtonOnFirstUse) {
   StartSession(/*pre_mark_time_of_first_add=*/false);
 
@@ -1343,6 +1401,38 @@ TEST_P(HoldingSpaceTrayTest, PlayIconForScreenRecordings) {
       kHoldingSpaceScreenCapturePlayIconId));
 }
 
+// Until the user has pinned an item, a placeholder should exist in the pinned
+// files bubble which contains a chip to open the Files app.
+TEST_P(HoldingSpaceTrayTest, PlaceholderContainsFilesAppChip) {
+  StartSession(/*pre_mark_time_of_first_add=*/false);
+
+  // The tray button should *not* be shown for users that have never added
+  // anything to the holding space.
+  EXPECT_FALSE(test_api()->IsShowingInShelf());
+
+  // Add a download item. This should cause the tray button to show.
+  AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake"));
+  MarkTimeOfFirstAdd();
+  EXPECT_TRUE(test_api()->IsShowingInShelf());
+
+  // Show the bubble. Both the pinned files and recent files child bubbles
+  // should be shown.
+  test_api()->Show();
+  EXPECT_TRUE(test_api()->PinnedFilesBubbleShown());
+  EXPECT_TRUE(test_api()->RecentFilesBubbleShown());
+
+  // A chip to open the Files app should exist in the pinned files bubble.
+  views::View* pinned_files_bubble = test_api()->GetPinnedFilesBubble();
+  ASSERT_TRUE(pinned_files_bubble);
+  views::View* files_app_chip =
+      pinned_files_bubble->GetViewByID(kHoldingSpaceFilesAppChipId);
+  ASSERT_TRUE(files_app_chip);
+
+  // Click the chip and expect a call to open the Files app.
+  EXPECT_CALL(*client(), OpenMyFiles);
+  Click(files_app_chip);
+}
+
 INSTANTIATE_TEST_SUITE_P(All, HoldingSpaceTrayTest, testing::Bool());
 
 }  // namespace ash

ash/system/holding_space/pinned_files_section.cc

@@ -4,7 +4,9 @@
 
 #include "ash/system/holding_space/pinned_files_section.h"
 
+#include "ash/public/cpp/holding_space/holding_space_client.h"
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
+#include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/holding_space/holding_space_prefs.h"
 #include "ash/session/session_controller_impl.h"
@@ -17,12 +19,24 @@
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/views/accessibility/view_accessibility.h"
+#include "ui/views/background.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/highlight_path_generator.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
+#include "ui/views/layout/box_layout.h"
 
 namespace ash {
 
 namespace {
 
+// Appearance.
+constexpr int kFilesAppChipChildSpacing = 16;
+constexpr int kFilesAppChipHeight = 32;
+constexpr int kFilesAppChipIconHeight = 16;
+constexpr gfx::Insets kFilesAppChipInsets(0, 8, 0, 16);
+constexpr int kPlaceholderChildSpacing = 16;
+
 // Helpers ---------------------------------------------------------------------
 
 // Returns whether the active user has ever pinned a file to holding space.
@@ -35,6 +49,88 @@ bool HasEverPinnedHoldingSpaceItem() {
              : false;
 }
 
+// FilesAppChip ----------------------------------------------------------------
+
+class FilesAppChip : public views::Button {
+ public:
+  FilesAppChip() { Init(); }
+  FilesAppChip(const FilesAppChip&) = delete;
+  FilesAppChip& operator=(const FilesAppChip&) = delete;
+  ~FilesAppChip() override = default;
+
+ private:
+  // views::Button:
+  gfx::Size CalculatePreferredSize() const override {
+    const int width = views::Button::CalculatePreferredSize().width();
+    return gfx::Size(width, GetHeightForWidth(width));
+  }
+
+  int GetHeightForWidth(int width) const override {
+    return kFilesAppChipHeight;
+  }
+
+  void Init() {
+    SetAccessibleName(l10n_util::GetStringUTF16(
+        IDS_ASH_HOLDING_SPACE_PINNED_FILES_APP_CHIP_TEXT));
+    SetCallback(
+        base::BindRepeating(&FilesAppChip::OnPressed, base::Unretained(this)));
+    SetID(kHoldingSpaceFilesAppChipId);
+
+    // Background.
+    AshColorProvider* const ash_color_provider = AshColorProvider::Get();
+    SetBackground(views::CreateRoundedRectBackground(
+        ash_color_provider->GetControlsLayerColor(
+            AshColorProvider::ControlsLayerType::
+                kControlBackgroundColorInactive),
+        kFilesAppChipHeight / 2));
+
+    // Focus ring.
+    focus_ring()->SetColor(ash_color_provider->GetControlsLayerColor(
+        AshColorProvider::ControlsLayerType::kFocusRingColor));
+
+    // Ink drop.
+    const AshColorProvider::RippleAttributes ripple_attributes =
+        ash_color_provider->GetRippleAttributes();
+    SetInkDropMode(InkDropMode::ON);
+    SetInkDropBaseColor(ripple_attributes.base_color);
+    SetInkDropVisibleOpacity(ripple_attributes.inkdrop_opacity);
+    views::InstallRoundRectHighlightPathGenerator(this, gfx::Insets(),
+                                                  kFilesAppChipHeight / 2);
+
+    // Layout.
+    auto* layout = SetLayoutManager(std::make_unique<views::BoxLayout>(
+        views::BoxLayout::Orientation::kHorizontal, kFilesAppChipInsets,
+        kFilesAppChipChildSpacing));
+    layout->set_cross_axis_alignment(
+        views::BoxLayout::CrossAxisAlignment::kCenter);
+
+    // Icon.
+    auto* icon = AddChildView(std::make_unique<views::ImageView>());
+    icon->SetBackground(views::CreateRoundedRectBackground(
+        ash_color_provider->GetControlsLayerColor(
+            AshColorProvider::ControlsLayerType::
+                kControlBackgroundColorInactive),
+        kFilesAppChipIconHeight / 2));
+    icon->SetPreferredSize(
+        gfx::Size(kFilesAppChipIconHeight, kFilesAppChipIconHeight));
+
+    // Label.
+    auto* label = AddChildView(std::make_unique<views::Label>());
+    label->SetEnabledColor(ash_color_provider->GetContentLayerColor(
+        AshColorProvider::ContentLayerType::kTextColorPrimary));
+    label->SetText(l10n_util::GetStringUTF16(
+        IDS_ASH_HOLDING_SPACE_PINNED_FILES_APP_CHIP_TEXT));
+    layout->SetFlexForView(label, 1);
+
+    TrayPopupUtils::SetLabelFontList(
+        label, TrayPopupUtils::FontStyle::kDetailedViewLabel);
+  }
+
+  void OnPressed(const ui::Event& event) {
+    HoldingSpaceController::Get()->client()->OpenMyFiles(base::DoNothing());
+  }
+};
+
 }  // namespace
 
 // PinnedFilesSection ----------------------------------------------------------
@@ -83,17 +179,30 @@ std::unique_ptr<views::View> PinnedFilesSection::CreatePlaceholder() {
   if (HasEverPinnedHoldingSpaceItem())
     return nullptr;
 
-  auto placeholder = std::make_unique<views::Label>(
-      l10n_util::GetStringUTF16(IDS_ASH_HOLDING_SPACE_PINNED_EMPTY_PROMPT));
-  placeholder->SetEnabledColor(AshColorProvider::Get()->GetContentLayerColor(
-      AshColorProvider::ContentLayerType::kTextColorPrimary));
-  placeholder->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
-  placeholder->SetMultiLine(true);
+  auto placeholder = std::make_unique<views::View>();
   placeholder->SetPaintToLayer();
   placeholder->layer()->SetFillsBoundsOpaquely(false);
 
+  auto* layout =
+      placeholder->SetLayoutManager(std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kVertical, gfx::Insets(),
+          kPlaceholderChildSpacing));
+  layout->set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kStart);
+
+  // Prompt.
+  auto* prompt = placeholder->AddChildView(std::make_unique<views::Label>(
+      l10n_util::GetStringUTF16(IDS_ASH_HOLDING_SPACE_PINNED_EMPTY_PROMPT)));
+  prompt->SetEnabledColor(AshColorProvider::Get()->GetContentLayerColor(
+      AshColorProvider::ContentLayerType::kTextColorPrimary));
+  prompt->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
+  prompt->SetMultiLine(true);
+
   TrayPopupUtils::SetLabelFontList(
-      placeholder.get(), TrayPopupUtils::FontStyle::kDetailedViewLabel);
+      prompt, TrayPopupUtils::FontStyle::kDetailedViewLabel);
+
+  // Files app chip.
+  placeholder->AddChildView(std::make_unique<FilesAppChip>());
 
   return placeholder;
 }

chrome/browser/ui/ash/holding_space/holding_space_client_impl.cc

@@ -182,6 +182,23 @@ void HoldingSpaceClientImpl::OpenItems(
   }
 }
 
+void HoldingSpaceClientImpl::OpenMyFiles(SuccessCallback callback) {
+  auto file_path = file_manager::util::GetMyFilesFolderForProfile(profile_);
+  if (file_path.empty()) {
+    std::move(callback).Run(/*success=*/false);
+    return;
+  }
+  file_manager::util::OpenItem(
+      profile_, file_path, platform_util::OPEN_FOLDER,
+      base::BindOnce(
+          [](SuccessCallback callback,
+             platform_util::OpenOperationResult result) {
+            const bool success = result == platform_util::OPEN_SUCCEEDED;
+            std::move(callback).Run(success);
+          },
+          std::move(callback)));
+}
+
 void HoldingSpaceClientImpl::ShowItemInFolder(const HoldingSpaceItem& item,
                                               SuccessCallback callback) {
   holding_space_metrics::RecordItemAction(

chrome/browser/ui/ash/holding_space/holding_space_client_impl.h

@@ -31,6 +31,7 @@ class HoldingSpaceClientImpl : public HoldingSpaceClient {
   void OpenDownloads(SuccessCallback callback) override;
   void OpenItems(const std::vector<const HoldingSpaceItem*>& items,
                  SuccessCallback callback) override;
+  void OpenMyFiles(SuccessCallback callback) override;
   void ShowItemInFolder(const HoldingSpaceItem&, SuccessCallback) override;
   void PinItems(const std::vector<const HoldingSpaceItem*>& items) override;
   void UnpinItems(const std::vector<const HoldingSpaceItem*>& items) override;

chrome/browser/ui/ash/holding_space/holding_space_client_impl_browsertest.cc

@@ -135,6 +135,23 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, DISABLED_OpenDownloads) {
   run_loop.Run();
 }
 
+// Verifies that `HoldingSpaceClient::OpenMyFiles()` works as intended.
+IN_PROC_BROWSER_TEST_F(HoldingSpaceClientImplTest, OpenMyFiles) {
+  ASSERT_TRUE(HoldingSpaceController::Get());
+
+  auto* holding_space_client = HoldingSpaceController::Get()->client();
+  ASSERT_TRUE(holding_space_client);
+
+  // We expect `HoldingSpaceClient::OpenMyFiles()` to succeed.
+  base::RunLoop run_loop;
+  holding_space_client->OpenMyFiles(
+      base::BindLambdaForTesting([&run_loop](bool success) {
+        EXPECT_TRUE(success);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+}
+
 // Verifies that `HoldingSpaceClient::OpenItems()` works as intended when
 // attempting to open holding space items backed by both non-existing and
 // existing files.