CrOS: Handle focus rotation between shelf and status area

Note that the main logic to handle focus transitions from status area
--> shelf was added in CL 1475149. This makes some small tweaks and
mainly adds the logic for transitions in the other direction.

This is a little more involved because we need to handle situations
where the overflow bubble is showing.

This also adds some extensive tests covering focus-cycling between all
these elements.

You can see a screencast of what this looks like in
    https://bugs.chromium.org/p/chromium/issues/detail?id=753409#c34

Bug: 753409
Change-Id: Ia903c27f54226b4d67cf796d2d076f9da72225a9
Reviewed-on: https://chromium-review.googlesource.com/c/1477464
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634723}

ash/shelf/login_shelf_view.cc

@@ -430,13 +430,11 @@ void LoginShelfView::AboutToRequestFocusFromTabTraversal(bool reverse) {
     }
   } else {
     // Focus goes to status area.
-    Shelf::ForWindow(GetWidget()->GetNativeWindow())
-        ->GetStatusAreaWidget()
-        ->status_area_widget_delegate()
+    StatusAreaWidget* status_area_widget =
+        Shelf::ForWindow(GetWidget()->GetNativeWindow())->GetStatusAreaWidget();
+    status_area_widget->status_area_widget_delegate()
         ->set_default_last_focusable_child(reverse);
-    Shell::Get()->focus_cycler()->FocusWidget(
-        Shelf::ForWindow(GetWidget()->GetNativeWindow())
-            ->GetStatusAreaWidget());
+    Shell::Get()->focus_cycler()->FocusWidget(status_area_widget);
   }
 }
 

ash/shelf/shelf_button.cc

@@ -54,6 +54,10 @@ bool ShelfButton::OnMouseDragged(const ui::MouseEvent& event) {
   return true;
 }
 
+void ShelfButton::AboutToRequestFocusFromTabTraversal(bool reverse) {
+  shelf_view_->OnShelfButtonAboutToRequestFocusFromTabTraversal(this, reverse);
+}
+
 // Do not remove this function to avoid unnecessary ChromeVox announcement
 // triggered by Button::GetAccessibleNodeData. (See https://crbug.com/932200)
 void ShelfButton::GetAccessibleNodeData(ui::AXNodeData* node_data) {

ash/shelf/shelf_button.h

@@ -23,6 +23,7 @@ class ASH_EXPORT ShelfButton : public views::Button {
   void OnMouseReleased(const ui::MouseEvent& event) override;
   void OnMouseCaptureLost() override;
   bool OnMouseDragged(const ui::MouseEvent& event) override;
+  void AboutToRequestFocusFromTabTraversal(bool reverse) override;
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
   bool GetTooltipText(const gfx::Point& p,
                       base::string16* tooltip) const override;

ash/shelf/shelf_view.cc

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "ash/drag_drop/drag_image_view.h"
+#include "ash/focus_cycler.h"
 #include "ash/metrics/user_metrics_recorder.h"
 #include "ash/public/cpp/ash_constants.h"
 #include "ash/public/cpp/shelf_item_delegate.h"
@@ -33,6 +34,7 @@
 #include "ash/system/model/system_tray_model.h"
 #include "ash/system/model/virtual_keyboard_model.h"
 #include "ash/system/status_area_widget.h"
+#include "ash/system/status_area_widget_delegate.h"
 #include "ash/wm/mru_window_tracker.h"
 #include "ash/wm/root_window_finder.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
@@ -204,7 +206,7 @@ class ShelfFocusSearch : public views::FocusSearch {
       new_index = 0;
 
     if (new_index >= overflow_cutoff)
-      shelf_view_->shelf_widget()->set_activated_from_overflow_bubble(true);
+      shelf_view_->shelf_widget()->set_activated_from_other_widget(true);
     return focusable_views[new_index];
   }
 
@@ -816,13 +818,44 @@ const std::vector<aura::Window*> ShelfView::GetOpenWindowsForShelfView(
   return open_windows;
 }
 
+views::View* ShelfView::FindFirstFocusableChild() {
+  if (is_overflow_mode())
+    return main_shelf()->FindFirstFocusableChild();
+  return view_model_->view_at(first_visible_index());
+}
+
+views::View* ShelfView::FindLastFocusableChild() {
+  if (is_showing_overflow_bubble())
+    return overflow_shelf()->FindLastFocusableChild();
+  return overflow_button_->visible()
+             ? overflow_button_
+             : view_model_->view_at(last_visible_index());
+}
+
 views::View* ShelfView::FindFirstOrLastFocusableChild(bool last) {
-  if (last) {
-    return overflow_button_->visible()
-               ? overflow_button_
-               : view_model_->view_at(last_visible_index());
-  } else {
-    return view_model_->view_at(first_visible_index());
+  return last ? FindLastFocusableChild() : FindFirstFocusableChild();
+}
+
+void ShelfView::OnShelfButtonAboutToRequestFocusFromTabTraversal(
+    ShelfButton* button,
+    bool reverse) {
+  if (is_overflow_mode()) {
+    main_shelf()->OnShelfButtonAboutToRequestFocusFromTabTraversal(button,
+                                                                   reverse);
+    return;
+  }
+
+  // The logic here seems backwards, but is actually correct. For instance if
+  // the ShelfView's internal focus cycling logic attemmpts to focus the first
+  // child (e.g. app list button) after hitting Tab, we intercept that and
+  // instead, advance through to the status area.
+  if ((reverse && button == FindLastFocusableChild()) ||
+      (!reverse && button == FindFirstFocusableChild())) {
+    StatusAreaWidget* status_area_widget =
+        Shelf::ForWindow(GetWidget()->GetNativeWindow())->GetStatusAreaWidget();
+    status_area_widget->status_area_widget_delegate()
+        ->set_default_last_focusable_child(reverse);
+    Shell::Get()->focus_cycler()->FocusWidget(status_area_widget);
   }
 }
 

ash/shelf/shelf_view.h

@@ -54,6 +54,7 @@ class OverflowButton;
 class ScopedRootWindowForNewWindows;
 class Shelf;
 class ShelfAppButton;
+class ShelfButton;
 class ShelfModel;
 struct ShelfItem;
 class ShelfMenuModelAdapter;
@@ -264,7 +265,20 @@ class ASH_EXPORT ShelfView : public views::View,
   const std::vector<aura::Window*> GetOpenWindowsForShelfView(
       views::View* view);
 
+  // The three methods below return the first or last focusable child of the
+  // set including both the main shelf and the overflow shelf it it's showing.
+  // - The first focusable child is either the app list button, or the back
+  //   button in tablet mode.
+  // - The last focusable child can be either 1) the last app icon on the main
+  //   shelf if there aren't enough apps to overflow, 2) the overflow button
+  //   if it's visible but the overflow bubble isn't showing, or 3) the last
+  //   app icon in the overflow bubble if it's showing.
   views::View* FindFirstOrLastFocusableChild(bool last);
+  views::View* FindFirstFocusableChild();
+  views::View* FindLastFocusableChild();
+
+  void OnShelfButtonAboutToRequestFocusFromTabTraversal(ShelfButton* button,
+                                                        bool reverse);
 
   // Return the view model for test purposes.
   const views::ViewModel* view_model_for_test() const {
@@ -292,6 +306,9 @@ class ASH_EXPORT ShelfView : public views::View,
   //     small resolution screen, the overflow bubble can show the app list
   //     button.
   bool is_overflow_mode() const { return overflow_mode_; }
+  bool is_showing_overflow_bubble() const {
+    return overflow_bubble_ && overflow_bubble_->IsShowing();
+  }
 
   int first_visible_index() const { return first_visible_index_; }
   int last_visible_index() const { return last_visible_index_; }

ash/shelf/shelf_view_unittest.cc

@@ -90,6 +90,16 @@ int64_t GetPrimaryDisplayId() {
   return display::Screen::GetScreen()->GetPrimaryDisplay().id();
 }
 
+void ExpectFocused(views::View* view) {
+  EXPECT_TRUE(view->GetWidget()->IsActive());
+  EXPECT_TRUE(view->Contains(view->GetFocusManager()->GetFocusedView()));
+}
+
+void ExpectNotFocused(views::View* view) {
+  EXPECT_FALSE(view->GetWidget()->IsActive());
+  EXPECT_FALSE(view->Contains(view->GetFocusManager()->GetFocusedView()));
+}
+
 class TestShelfObserver : public ShelfObserver {
  public:
   explicit TestShelfObserver(Shelf* shelf) : shelf_(shelf) {
@@ -3450,8 +3460,13 @@ class ShelfViewFocusTest : public ShelfViewTest {
     AddAppShortcut();
     AddAppShortcut();
 
-    // Focus the shelf.
     Shelf* shelf = Shelf::ForWindow(Shell::GetPrimaryRootWindow());
+    gfx::NativeWindow window = shelf->shelf_widget()->GetNativeWindow();
+    status_area_ = RootWindowController::ForWindow(window)
+                       ->GetStatusAreaWidget()
+                       ->GetContentsView();
+
+    // Focus the shelf.
     Shell::Get()->focus_cycler()->FocusWidget(shelf->shelf_widget());
   }
 
@@ -3466,6 +3481,9 @@ class ShelfViewFocusTest : public ShelfViewTest {
                        ui::EventFlags::EF_SHIFT_DOWN);
   }
 
+ protected:
+  views::View* status_area_ = nullptr;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ShelfViewFocusTest);
 };
@@ -3496,19 +3514,16 @@ TEST_F(ShelfViewFocusTest, ForwardCycling) {
   DoTab();
   DoTab();
   EXPECT_TRUE(test_api_->GetViewAt(4)->HasFocus());
-
-  // The last element is currently focused so pressing tab once should advance
-  // focus to the first element.
-  DoTab();
-  EXPECT_TRUE(test_api_->GetViewAt(1)->HasFocus());
 }
 
 // Tests that the expected views have focus when cycling backwards through shelf
 // items with shift tab.
 TEST_F(ShelfViewFocusTest, BackwardCycling) {
-  // The first element is currently focused so pressing shift tab once should
-  // advance focus to the last element.
-  DoShiftTab();
+  // The first element is currently focused. Let's advance to the last element
+  // first.
+  DoTab();
+  DoTab();
+  DoTab();
   EXPECT_TRUE(test_api_->GetViewAt(4)->HasFocus());
 
   // Pressing shift tab once should advance focus to the previous element.
@@ -3526,6 +3541,41 @@ TEST_F(ShelfViewFocusTest, OverflowNotActivatedWhenOpened) {
   EXPECT_TRUE(::wm::IsActiveWindow(window.get()));
 }
 
+// Verifies that focus moves as expected between the shelf and the status area.
+TEST_F(ShelfViewFocusTest, FocusCyclingBetweenShelfAndStatusWidget) {
+  // The first element of the shelf is focused at start.
+
+  // Focus the next few elements.
+  DoTab();
+  EXPECT_TRUE(test_api_->GetViewAt(2)->HasFocus());
+  DoTab();
+  EXPECT_TRUE(test_api_->GetViewAt(3)->HasFocus());
+  DoTab();
+  EXPECT_TRUE(test_api_->GetViewAt(4)->HasFocus());
+
+  // This is the last element. Tabbing once more should go into the status
+  // area.
+  DoTab();
+  ExpectNotFocused(shelf_view_);
+  ExpectFocused(status_area_);
+
+  // Shift-tab: we should be back at the last element in the shelf.
+  DoShiftTab();
+  EXPECT_TRUE(test_api_->GetViewAt(4)->HasFocus());
+  ExpectNotFocused(status_area_);
+
+  // Go into the status area again.
+  DoTab();
+  ExpectNotFocused(shelf_view_);
+  ExpectFocused(status_area_);
+
+  // And keep going forward, now we should be cycling back to the first shelf
+  // element.
+  DoTab();
+  EXPECT_TRUE(test_api_->GetViewAt(1)->HasFocus());
+  ExpectNotFocused(status_area_);
+}
+
 class ShelfViewOverflowFocusTest : public ShelfViewFocusTest {
  public:
   ShelfViewOverflowFocusTest() = default;
@@ -3598,16 +3648,13 @@ TEST_F(ShelfViewOverflowFocusTest, ForwardCycling) {
   // Focus the overflow button.
   DoTab();
   EXPECT_TRUE(test_api_->overflow_button()->HasFocus());
-
-  DoTab();
-  EXPECT_TRUE(test_api_->GetViewAt(1)->HasFocus());
 }
 
 // Tests that when cycling through the items with shift tab, the items in the
 // overflow shelf are ignored because it is not visible.
 TEST_F(ShelfViewOverflowFocusTest, BackwardCycling) {
-  DoShiftTab();
-  EXPECT_TRUE(test_api_->overflow_button()->HasFocus());
+  while (!test_api_->overflow_button()->HasFocus())
+    DoTab();
 
   DoShiftTab();
   EXPECT_TRUE(test_api_->GetViewAt(last_item_on_main_shelf_index_)->HasFocus());
@@ -3634,20 +3681,6 @@ TEST_F(ShelfViewOverflowFocusTest, ForwardCyclingWithBubbleOpen) {
   const int first_index_overflow_shelf = last_item_on_main_shelf_index_ + 1;
   EXPECT_TRUE(overflow_shelf_test_api_->GetViewAt(first_index_overflow_shelf)
                   ->HasFocus());
-
-  // Focus the last item on the overflow shelf.
-  test_api_->overflow_bubble()
-      ->bubble_view()
-      ->GetWidget()
-      ->GetFocusManager()
-      ->SetFocusedView(overflow_shelf_test_api_->GetViewAt(
-          overflow_shelf_test_api_->GetLastVisibleIndex()));
-
-  // Tests that after pressing tab once more, the main shelf widget now is
-  // active, and the first item on the main shelf has focus.
-  DoTab();
-  EXPECT_TRUE(shelf_view_->shelf_widget()->IsActive());
-  EXPECT_TRUE(test_api_->GetViewAt(1)->HasFocus());
 }
 
 // Tests that backwards cycling through elements with shift tab works as
@@ -3655,23 +3688,11 @@ TEST_F(ShelfViewOverflowFocusTest, ForwardCyclingWithBubbleOpen) {
 TEST_F(ShelfViewOverflowFocusTest, BackwardCyclingWithBubbleOpen) {
   OpenOverflow();
 
-  // Tests that after pressing shift tab once, the overflow shelf bubble is
-  // active and the last item on the overflow shelf has focus.
-  DoShiftTab();
-  EXPECT_TRUE(
-      test_api_->overflow_bubble()->bubble_view()->GetWidget()->IsActive());
-  const int first_index_overflow_shelf = last_item_on_main_shelf_index_ + 1;
-  EXPECT_TRUE(overflow_shelf_test_api_
-                  ->GetViewAt(overflow_shelf_test_api_->GetLastVisibleIndex())
-                  ->HasFocus());
-
   // Focus the first item on the overflow shelf.
-  test_api_->overflow_bubble()
-      ->bubble_view()
-      ->GetWidget()
-      ->GetFocusManager()
-      ->SetFocusedView(
-          overflow_shelf_test_api_->GetViewAt(first_index_overflow_shelf));
+  while (!test_api_->overflow_bubble()->bubble_view()->GetWidget()->IsActive())
+    DoTab();
+  EXPECT_FALSE(shelf_view_->shelf_widget()->IsActive());
+  EXPECT_FALSE(test_api_->overflow_button()->HasFocus());
 
   // Tests that after pressing shift tab once, the main shelf is active and
   // the overflow button has focus.
@@ -3684,4 +3705,60 @@ TEST_F(ShelfViewOverflowFocusTest, BackwardCyclingWithBubbleOpen) {
   EXPECT_TRUE(test_api_->GetViewAt(last_item_on_main_shelf_index_)->HasFocus());
 }
 
+// Verifies that focus moves as expected between the shelf and the status area
+// when the overflow bubble is showing.
+TEST_F(ShelfViewOverflowFocusTest, FocusCyclingBetweenShelfAndStatusWidget) {
+  OpenOverflow();
+  const int first_index_overflow_shelf = last_item_on_main_shelf_index_ + 1;
+
+  // We start with the first shelf item focused. Shift-tab should focus the
+  // status area.
+  DoShiftTab();
+  ExpectNotFocused(shelf_view_);
+  ExpectFocused(status_area_);
+
+  // Focus the shelf again.
+  DoTab();
+  ExpectFocused(shelf_view_);
+  EXPECT_TRUE(test_api_->GetViewAt(1)->HasFocus());
+  ExpectNotFocused(status_area_);
+
+  // Now advance to the last item on the main shelf.
+  while (!test_api_->GetViewAt(last_item_on_main_shelf_index_)->HasFocus())
+    DoTab();
+  ExpectNotFocused(status_area_);
+
+  // Focus the overflow button
+  DoTab();
+  EXPECT_TRUE(test_api_->overflow_button()->HasFocus());
+
+  // Tab into the overflow bubble.
+  DoTab();
+  EXPECT_TRUE(overflow_shelf_test_api_->GetViewAt(first_index_overflow_shelf)
+                  ->HasFocus());
+
+  // Back onto the overflow button itself.
+  DoShiftTab();
+  EXPECT_TRUE(test_api_->overflow_button()->HasFocus());
+
+  // Now advance until we get to the status area.
+  while (!status_area_->GetWidget()->IsActive())
+    DoTab();
+
+  // Go back once, we should be in the overflow bubble again.
+  DoShiftTab();
+  ExpectNotFocused(status_area_);
+  ExpectFocused(test_api_->overflow_bubble()->bubble_view());
+
+  // Go into the status area again.
+  DoTab();
+  ExpectFocused(status_area_);
+
+  // Now advance until the status area isn't focused anymore.
+  while (status_area_->GetWidget()->IsActive())
+    DoTab();
+  // This should have brought focus to the first element on the shelf.
+  EXPECT_TRUE(test_api_->GetViewAt(1)->HasFocus());
+}
+
 }  // namespace ash

ash/shelf/shelf_widget.cc

@@ -483,15 +483,24 @@ void ShelfWidget::set_default_last_focusable_child(
       default_last_focusable_child);
 }
 
+void ShelfWidget::FocusFirstOrLastFocusableChild(bool last) {
+  // This is only ever called during an active session.
+  if (!shelf_view_->visible())
+    return;
+  views::View* to_focus = shelf_view_->FindFirstOrLastFocusableChild(last);
+
+  Shell::Get()->focus_cycler()->FocusWidget(to_focus->GetWidget());
+  to_focus->GetFocusManager()->SetFocusedView(to_focus);
+}
+
 void ShelfWidget::OnWidgetActivationChanged(views::Widget* widget,
                                             bool active) {
   if (active) {
     // Do not focus the default element if the widget activation came from the
-    // overflow bubble focus cycling. The setter of
-    // |activated_from_overflow_bubble_| should handle focusing the correct
-    // view.
-    if (activated_from_overflow_bubble_) {
-      activated_from_overflow_bubble_ = false;
+    // another widget's focus cycling. The setter of
+    // |activated_from_other_widget_| should handle focusing the correct view.
+    if (activated_from_other_widget_) {
+      activated_from_other_widget_ = false;
       return;
     }
     delegate_view_->SetPaneFocusAndFocusDefault();

ash/shelf/shelf_widget.h

@@ -103,6 +103,10 @@ class ASH_EXPORT ShelfWidget : public views::Widget,
 
   void set_default_last_focusable_child(bool default_last_focusable_child);
 
+  // Finds the first or last focusable child of the set (main shelf + overflow)
+  // and focuses it.
+  void FocusFirstOrLastFocusableChild(bool last);
+
   // Overridden from views::WidgetObserver:
   void OnWidgetActivationChanged(views::Widget* widget, bool active) override;
 
@@ -127,8 +131,8 @@ class ASH_EXPORT ShelfWidget : public views::Widget,
     return &background_animator_;
   }
 
-  void set_activated_from_overflow_bubble(bool val) {
-    activated_from_overflow_bubble_ = val;
+  void set_activated_from_other_widget(bool val) {
+    activated_from_other_widget_ = val;
   }
 
  private:
@@ -166,10 +170,10 @@ class ASH_EXPORT ShelfWidget : public views::Widget,
   // Owned by the views hierarchy.
   LoginShelfView* const login_shelf_view_;
 
-  // Set to true when the widget is activated from the shelf overflow bubble.
-  // Do not focus the default element in this case. This should be set when
-  // cycling focus from the overflow bubble to the main shelf.
-  bool activated_from_overflow_bubble_ = false;
+  // Set to true when the widget is activated from another widget. Do not
+  // focus the default element in this case. This should be set when
+  // cycling focus from another widget to the shelf.
+  bool activated_from_other_widget_ = false;
 
   ScopedSessionObserver scoped_session_observer_;
 

ash/system/tray/tray_background_view.cc

@@ -270,21 +270,34 @@ void TrayBackgroundView::AboutToRequestFocusFromTabTraversal(bool reverse) {
   if (!delegate || !delegate->ShouldFocusOut(reverse))
     return;
 
-  // At this point, we know we should focus out of the status widget.
-  // If we're not using a views-based shelf, delegate to system tray focus
-  // observers to decide where the focus goes next.
+  // At this point, we know we should focus out of the status widget. It
+  // remains to be determined whether we should bring focus to the shelf, or
+  // whether we should delegate to system tray focus observers to decide
+  // where the focus goes next.
+  bool should_focus_shelf = true;
+
   if (!ShelfWidget::IsUsingViewsShelf()) {
-    Shell::Get()->system_tray_notifier()->NotifyFocusOut(reverse);
+    // Never bring the focus to the shelf if it's not a views-based shelf as
+    // it is visually not on par with the status widget.
     return;
   }
 
   // If we are using a views-based shelf:
-  // * If we're going in reverse, always focus the shelf.
-  // * If we're going forward, focus the shelf in an active session, and let
-  //   the system tray focus observers focus the lock/login view otherwise.
-  if (reverse) {
+  // * If we're in an active session, always bring focus to the shelf whether
+  //   we are going in reverse or not.
+  // * Otherwise (login/lock screen, OOBE), bring focus to the shelf only
+  //   if we're going in reverse; if we're going forward, let the system tray
+  //   focus observers focus the lock/login view.
+  if (shelf->shelf_widget()->login_shelf_view()->visible()) {
+    // Login/lock screen or OOBE.
+    should_focus_shelf = reverse;
+  }
+
+  if (should_focus_shelf) {
     shelf->shelf_widget()->set_default_last_focusable_child(reverse);
+    shelf->shelf_widget()->set_activated_from_other_widget(true);
     Shell::Get()->focus_cycler()->FocusWidget(shelf->shelf_widget());
+    shelf->shelf_widget()->FocusFirstOrLastFocusableChild(reverse);
   } else {
     Shell::Get()->system_tray_notifier()->NotifyFocusOut(reverse);
   }