Focus fixes for holding space.

Previously opening holding space via keyboard would keep focus on the
holding space tray and not be able to enter the holding space bubble.

Also, we weren't previously watching for destruction of the holding
space bubble widget so if it was closed without going through
HoldingSpaceTray::CloseBubble(), we got in a bad state. This could
happen if the user closed holding space via the ESC key. This has now
been fixed.

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

ash/system/holding_space/holding_space_tray.cc

@@ -104,6 +104,11 @@ void HoldingSpaceTray::HideBubble(const TrayBubbleView* bubble_view) {
   CloseBubble();
 }
 
+void HoldingSpaceTray::OnWidgetDestroying(views::Widget* widget) {
+  widget->RemoveObserver(this);
+  CloseBubble();
+}
+
 void HoldingSpaceTray::AnchorUpdated() {
   if (bubble_)
     bubble_->bubble_view()->UpdateBubble();
@@ -116,6 +121,15 @@ bool HoldingSpaceTray::PerformAction(const ui::Event& event) {
   }
 
   ShowBubble(event.IsMouseEvent() || event.IsGestureEvent());
+
+  // Activate the bubble for a11y or if it was shown via keypress. Otherwise
+  // focus will remain on the tray when it should enter the bubble.
+  if (event.IsKeyEvent() || (event.flags() & ui::EF_TOUCH_ACCESSIBILITY)) {
+    DCHECK(bubble_ && bubble_->GetBubbleWidget());
+    bubble_->GetBubbleWidget()->widget_delegate()->SetCanActivate(true);
+    bubble_->GetBubbleWidget()->Activate();
+  }
+
   return true;
 }
 
@@ -125,6 +139,15 @@ void HoldingSpaceTray::UpdateAfterLoginStatusChange() {
 }
 
 void HoldingSpaceTray::CloseBubble() {
+  if (!bubble_)
+    return;
+
+  // If the call to `CloseBubble()` originated from `OnWidgetDestroying()`, as
+  // would be the case when closing due to ESC key press, the bubble widget will
+  // have already been destroyed.
+  if (bubble_->GetBubbleWidget())
+    bubble_->GetBubbleWidget()->RemoveObserver(this);
+
   bubble_.reset();
   SetIsActive(false);
 }
@@ -173,6 +196,12 @@ void HoldingSpaceTray::ShowBubble(bool show_by_click) {
   bubble_->GetBubbleWidget()->non_client_view()->frame_view()->SetVisible(
       false);
 
+  // Observe the bubble widget so that we can do proper clean up when it is
+  // being destroyed. If destruction is due to a call to `CloseBubble()` we will
+  // have already cleaned up state but there are cases where the bubble widget
+  // is destroyed independent of a call to `CloseBubble()`, e.g. ESC key press.
+  bubble_->GetBubbleWidget()->AddObserver(this);
+
   SetIsActive(true);
 }
 

ash/system/holding_space/holding_space_tray.h

@@ -11,6 +11,7 @@
 #include "ash/system/holding_space/holding_space_item_view_delegate.h"
 #include "ash/system/tray/tray_background_view.h"
 #include "base/memory/weak_ptr.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace gfx {
 class Point;
@@ -29,7 +30,8 @@ class TrayBubbleWrapper;
 // The HoldingSpaceTray shows the tray button in the bottom area of the screen.
 // This class also controls the lifetime for all of the tools available in the
 // palette. HoldingSpaceTray has one instance per-display.
-class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView {
+class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView,
+                                    public views::WidgetObserver {
  public:
   explicit HoldingSpaceTray(Shelf* shelf);
   HoldingSpaceTray(const HoldingSpaceTray& other) = delete;
@@ -59,6 +61,9 @@ class ASH_EXPORT HoldingSpaceTray : public TrayBackgroundView {
   bool ShouldEnableExtraKeyboardAccessibility() override;
   void HideBubble(const TrayBubbleView* bubble_view) override;
 
+  // views::WidgetObserver:
+  void OnWidgetDestroying(views::Widget* widget) override;
+
   // The singleton delegate for `HoldingSpaceItemView`s that implements support
   // for context menu, drag-and-drop, and multiple selection.
   HoldingSpaceItemViewDelegate delegate_;