Open holding space items when pressing RETURN key.

This CL also adds focus ring for indication of focus.

Bug: 1129694
Change-Id: I0140b01dd6d6ef758176b942622d222005a5710e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424824Reviewed-by: Ahmed Mehfooz <amehfooz@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#809524}

ash/public/cpp/holding_space/holding_space_constants.h

@@ -26,13 +26,13 @@ constexpr int kHoldingSpaceContainerChildSpacing = 16;
 constexpr int kHoldingSpaceContainerSpacing = 8;
 constexpr gfx::Insets kHoldingSpaceChipPadding(8);
 constexpr int kHoldingSpaceChipChildSpacing = 8;
-constexpr int kHoldingSpaceChipCornerRadius = 8;
 constexpr int kHoldingSpaceChipHeight = 40;
 constexpr int kHoldingSpaceChipIconSize = 24;
 constexpr int kHoldingSpaceChipWidth = 160;
 constexpr int kHoldingSpaceChipsPerRow = 2;
 constexpr int kHoldingSpaceColumnSpacing = 8;
 constexpr int kHoldingSpaceColumnWidth = 160;
+constexpr int kHoldingSpaceCornerRadius = 8;
 constexpr int kHoldingSpaceRowSpacing = 8;
 constexpr int kHoldingSpaceScreenshotSpacing = 8;
 constexpr gfx::Size kHoldingSpaceScreenshotSize(104, 80);

ash/system/holding_space/holding_space_item_chip_view.cc

@@ -23,7 +23,6 @@
 #include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/image_button.h"
-#include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/metadata/metadata_impl_macros.h"
@@ -39,8 +38,8 @@ HoldingSpaceItemChipView::HoldingSpaceItemChipView(const HoldingSpaceItem* item)
 
   SetPreferredSize(gfx::Size(kHoldingSpaceChipWidth, kHoldingSpaceChipHeight));
 
-  image_ =
-      AddChildView(std::make_unique<tray::RoundedImageView>(kTrayItemSize / 2));
+  image_ = AddChildView(
+      std::make_unique<tray::RoundedImageView>(kHoldingSpaceChipIconSize / 2));
 
   label_ = AddChildView(std::make_unique<views::Label>(item->text()));
   label_->SetElideBehavior(gfx::ELIDE_MIDDLE);
@@ -54,17 +53,13 @@ HoldingSpaceItemChipView::HoldingSpaceItemChipView(const HoldingSpaceItem* item)
   SetBackground(views::CreateRoundedRectBackground(
       AshColorProvider::Get()->GetControlsLayerColor(
           AshColorProvider::ControlsLayerType::kControlBackgroundColorInactive),
-      kHoldingSpaceChipCornerRadius));
+      kHoldingSpaceCornerRadius));
 
   SetInkDropMode(InkDropMode::ON_NO_GESTURE_HANDLER);
   SetInkDropVisibleOpacity(
       ShelfConfig::Get()->GetInkDropRippleAttributes().inkdrop_opacity);
   SetNotifyEnterExitOnChild(true);
 
-  // Ink drop layers should be clipped to match the corner radius of this view.
-  views::InstallRoundRectHighlightPathGenerator(this, gfx::Insets(),
-                                                kHoldingSpaceChipCornerRadius);
-
   // Subscribe to be notified of changes to `item_`'s image.
   image_subscription_ =
       item->image().AddImageSkiaChangedCallback(base::BindRepeating(

ash/system/holding_space/holding_space_item_screenshot_view.cc

@@ -20,20 +20,20 @@ HoldingSpaceItemScreenshotView::HoldingSpaceItemScreenshotView(
     : HoldingSpaceItemView(item) {
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
-  image_ =
-      AddChildView(std::make_unique<tray::RoundedImageView>(kTrayItemSize / 2));
+  image_ = AddChildView(
+      std::make_unique<tray::RoundedImageView>(kHoldingSpaceCornerRadius));
 
   // Subscribe to be notified of changes to `item_`'s image.
-  image_subscription_ =
-      item->image().AddImageSkiaChangedCallback(base::BindRepeating(
-          &HoldingSpaceItemScreenshotView::Update, base::Unretained(this)));
+  image_subscription_ = item->image().AddImageSkiaChangedCallback(
+      base::BindRepeating(&HoldingSpaceItemScreenshotView::UpdateImage,
+                          base::Unretained(this)));
 
-  Update();
+  UpdateImage();
 }
 
 HoldingSpaceItemScreenshotView::~HoldingSpaceItemScreenshotView() = default;
 
-void HoldingSpaceItemScreenshotView::Update() {
+void HoldingSpaceItemScreenshotView::UpdateImage() {
   image_->SetImage(item()->image().image_skia(), kHoldingSpaceScreenshotSize);
 }
 

ash/system/holding_space/holding_space_item_screenshot_view.h

@@ -30,7 +30,7 @@ class ASH_EXPORT HoldingSpaceItemScreenshotView : public HoldingSpaceItemView {
   ~HoldingSpaceItemScreenshotView() override;
 
  private:
-  void Update();
+  void UpdateImage();
 
   tray::RoundedImageView* image_ = nullptr;
 

ash/system/holding_space/holding_space_item_view.cc

@@ -16,6 +16,8 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/text_constants.h"
 #include "ui/views/accessibility/view_accessibility.h"
+#include "ui/views/controls/focus_ring.h"
+#include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/controls/menu/menu_runner.h"
 
 namespace ash {
@@ -42,7 +44,14 @@ HoldingSpaceItemView::HoldingSpaceItemView(const HoldingSpaceItem* item)
   layer()->SetFillsBoundsOpaquely(false);
 
   GetViewAccessibility().OverrideName(item->text());
+
   SetFocusBehavior(FocusBehavior::ALWAYS);
+  views::FocusRing* focus_ring = views::FocusRing::Install(this);
+  focus_ring->SetColor(ShelfConfig::Get()->shelf_focus_border_color());
+
+  // Focus ring and ink drop layers should match the corner radius of this view.
+  views::InstallRoundRectHighlightPathGenerator(this, gfx::Insets(),
+                                                kHoldingSpaceCornerRadius);
 }
 
 HoldingSpaceItemView::~HoldingSpaceItemView() = default;
@@ -60,6 +69,14 @@ void HoldingSpaceItemView::OnGestureEvent(ui::GestureEvent* event) {
     OpenItem(*item());
 }
 
+bool HoldingSpaceItemView::OnKeyPressed(const ui::KeyEvent& event) {
+  if (event.key_code() == ui::KeyboardCode::VKEY_RETURN) {
+    OpenItem(*item());
+    return true;
+  }
+  return false;
+}
+
 bool HoldingSpaceItemView::OnMousePressed(const ui::MouseEvent& event) {
   if (event.flags() & ui::EF_IS_DOUBLE_CLICK) {
     OpenItem(*item());

ash/system/holding_space/holding_space_item_view.h

@@ -28,6 +28,7 @@ class ASH_EXPORT HoldingSpaceItemView : public views::InkDropHostView {
   int GetDragOperations(const gfx::Point& point) override;
   SkColor GetInkDropBaseColor() const override;
   void OnGestureEvent(ui::GestureEvent* event) override;
+  bool OnKeyPressed(const ui::KeyEvent& event) override;
   bool OnMousePressed(const ui::MouseEvent& event) override;
   void WriteDragData(const gfx::Point& point, ui::OSExchangeData*) override;
 

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

@@ -203,6 +203,11 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceUiBrowserTest, OpenItem) {
   std::vector<base::OnceCallback<void(const views::View*)>> user_interactions;
   user_interactions.push_back(base::BindOnce(&DoubleClick));
   user_interactions.push_back(base::BindOnce(&GestureTap));
+  user_interactions.push_back(base::BindOnce([](const views::View* view) {
+    while (!view->HasFocus())
+      PressAndReleaseKey(ui::KeyboardCode::VKEY_TAB);
+    PressAndReleaseKey(ui::KeyboardCode::VKEY_RETURN);
+  }));
 
   for (auto& user_interaction : user_interactions) {
     // Show holding space UI and verify a holding space item view exists.