Fix bug where tab hover card keyboard navigation did not work on Mac.

When keyboard navigating the tab strip on Mac pane_has_focus is not set to true so key events were hiding the tab hover cards when they shouldnt have been. This change instead ignores hiding cards on when updating the hover card if the tab strip or one of its children (excluding the new tab button) has focus.

Bug: 910739, 974896
Change-Id: I306543684f448fa3e4ceac0f346a5b833f334b7e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1650213
Commit-Queue: Caroline Rising <corising@chromium.org>
Reviewed-by: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669897}

chrome/browser/ui/views/tabs/tab.cc

@@ -17,6 +17,7 @@
 #include "base/macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/numerics/ranges.h"
+#include "base/scoped_observer.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
@@ -112,6 +113,42 @@ int Center(int size, int item_size) {
 
 }  // namespace
 
+// Helper class that observes the tab's close button.
+class Tab::TabCloseButtonObserver : public views::ViewObserver {
+ public:
+  explicit TabCloseButtonObserver(Tab* tab,
+                                  views::View* close_button,
+                                  TabController* controller)
+      : tab_(tab), close_button_(close_button), controller_(controller) {
+    DCHECK(close_button_);
+    tab_close_button_observer_.Add(close_button_);
+  }
+
+  ~TabCloseButtonObserver() override {
+    tab_close_button_observer_.Remove(close_button_);
+  }
+
+ private:
+  void OnViewFocused(views::View* observed_view) override {
+    controller_->UpdateHoverCard(tab_, /* should_show */ true);
+  }
+
+  void OnViewBlurred(views::View* observed_view) override {
+    // Only hide hover card if not keyboard navigating.
+    if (!controller_->IsFocusInTabs())
+      controller_->UpdateHoverCard(nullptr, /* should_show */ false);
+  }
+
+  ScopedObserver<views::View, views::ViewObserver> tab_close_button_observer_{
+      this};
+
+  Tab* tab_;
+  views::View* close_button_;
+  TabController* controller_;
+
+  DISALLOW_COPY_AND_ASSIGN(TabCloseButtonObserver);
+};
+
 // Tab -------------------------------------------------------------------------
 
 // static
@@ -156,7 +193,9 @@ Tab::Tab(TabController* controller)
       this, base::BindRepeating(&TabController::OnMouseEventInTab,
                                 base::Unretained(controller_)));
   AddChildView(close_button_);
-  close_button_->AddObserver(this);
+
+  tab_close_button_observer_ = std::make_unique<TabCloseButtonObserver>(
+      this, close_button_, controller_);
 
   set_context_menu_controller(this);
 
@@ -168,7 +207,8 @@ Tab::Tab(TabController* controller)
 }
 
 Tab::~Tab() {
-  close_button_->RemoveObserver(this);
+  // Observer must be unregistered before child views are destroyed.
+  tab_close_button_observer_.reset();
   controller_->UpdateHoverCard(this, /* should_show */ false);
 }
 
@@ -597,16 +637,17 @@ void Tab::AddedToWidget() {
 }
 
 void Tab::OnFocus() {
-  controller_->UpdateHoverCard(this, /* should_show */ true);
   View::OnFocus();
+  controller_->UpdateHoverCard(this, /* should_show */ true);
 }
 
-void Tab::OnThemeChanged() {
-  UpdateForegroundColors();
+void Tab::OnBlur() {
+  View::OnBlur();
+  controller_->UpdateHoverCard(nullptr, /* should_show */ false);
 }
 
-void Tab::OnViewFocused(views::View* observed_view) {
-  controller_->UpdateHoverCard(this, /* should_show */ true);
+void Tab::OnThemeChanged() {
+  UpdateForegroundColors();
 }
 
 void Tab::SetClosing(bool closing) {

chrome/browser/ui/views/tabs/tab.h

@@ -99,11 +99,9 @@ class Tab : public gfx::AnimationDelegate,
   void OnPaint(gfx::Canvas* canvas) override;
   void AddedToWidget() override;
   void OnFocus() override;
+  void OnBlur() override;
   void OnThemeChanged() override;
 
-  // views::ViewObserver:
-  void OnViewFocused(views::View* observed_view) override;
-
   TabController* controller() const { return controller_; }
 
   // Used to set/check whether this Tab is being animated closed.
@@ -188,6 +186,7 @@ class Tab : public gfx::AnimationDelegate,
                                        TabAlertState alert_state);
 
  private:
+  class TabCloseButtonObserver;
   friend class AlertIndicatorTest;
   friend class TabTest;
   friend class TabStripTest;
@@ -195,6 +194,8 @@ class Tab : public gfx::AnimationDelegate,
   FRIEND_TEST_ALL_PREFIXES(TabStripTest,
                            TabCloseButtonVisibilityWhenNotStacked);
   FRIEND_TEST_ALL_PREFIXES(TabTest, TitleTextHasSufficientContrast);
+  FRIEND_TEST_ALL_PREFIXES(TabHoverCardBubbleViewBrowserTest,
+                           WidgetVisibleOnTabCloseButtonFocusAfterTabFocus);
 
   // Invoked from Layout to adjust the position of the favicon or alert
   // indicator for pinned tabs. The visual_width parameter is how wide the
@@ -289,6 +290,8 @@ class Tab : public gfx::AnimationDelegate,
   // the view bounds.
   bool mouse_hovered_ = false;
 
+  std::unique_ptr<TabCloseButtonObserver> tab_close_button_observer_;
+
   // Focus ring for accessibility.
   std::unique_ptr<views::FocusRing> focus_ring_;
 

chrome/browser/ui/views/tabs/tab_controller.h

@@ -76,6 +76,9 @@ class TabController {
   virtual bool IsFirstVisibleTab(const Tab* tab) const = 0;
   virtual bool IsLastVisibleTab(const Tab* tab) const = 0;
 
+  // Returns true if any tab or one of its children has focus.
+  virtual bool IsFocusInTabs() const = 0;
+
   // Potentially starts a drag for the specified Tab.
   virtual void MaybeStartDrag(
       Tab* tab,

chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.cc

@@ -377,8 +377,15 @@ void TabHoverCardBubbleView::UpdateAndShow(Tab* tab) {
                                kMaxHoverCardReshowTimeDelta,
                                kHoverCardHistogramBucketCount);
   }
-  bool show_immediately = !last_visible_timestamp_.is_null() &&
-                          elapsed_time <= kShowWithoutDelayTimeBuffer;
+  bool within_delay_time_buffer = !last_visible_timestamp_.is_null() &&
+                                  elapsed_time <= kShowWithoutDelayTimeBuffer;
+  // Hover cards should be shown without delay if triggered within the time
+  // buffer or if the tab or its children have focus which indicates that the
+  // tab is keyboard focused.
+  const views::FocusManager* tab_focus_manager = tab->GetFocusManager();
+  bool show_immediately =
+      within_delay_time_buffer || tab->HasFocus() ||
+      (tab_focus_manager && tab->Contains(tab_focus_manager->GetFocusedView()));
 
   fade_animation_delegate_->CancelFadeOut();
 
@@ -404,8 +411,7 @@ void TabHoverCardBubbleView::UpdateAndShow(Tab* tab) {
   }
 
   if (!widget_->IsVisible()) {
-    if (disable_animations_for_testing_ || show_immediately ||
-        tab->HasFocus()) {
+    if (disable_animations_for_testing_ || show_immediately) {
       widget_->SetOpacity(1.0f);
       widget_->Show();
     } else {

chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/ui_features.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
+#include "chrome/browser/ui/views/tabs/tab_close_button.h"
 #include "chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/grit/generated_resources.h"
@@ -183,6 +184,79 @@ IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
   EXPECT_FALSE(widget->IsVisible());
 }
 
+// Verify hover card is visible when tab is focused.
+IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
+                       WidgetVisibleOnTabFocus) {
+  TabStrip* tab_strip =
+      BrowserView::GetBrowserViewForBrowser(browser())->tabstrip();
+  Tab* tab = tab_strip->tab_at(0);
+  tab_strip->GetFocusManager()->SetFocusedView(tab);
+  TabHoverCardBubbleView* hover_card = GetHoverCard(tab_strip);
+  Widget* widget = GetHoverCardWidget(hover_card);
+  HoverCardVisibleWaiter waiter(widget);
+  waiter.Wait();
+  EXPECT_TRUE(widget != nullptr);
+  EXPECT_TRUE(widget->IsVisible());
+}
+
+// Verify hover card is visible when focus moves from the tab to tab close
+// button.
+IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
+                       WidgetVisibleOnTabCloseButtonFocusAfterTabFocus) {
+  TabStrip* tab_strip =
+      BrowserView::GetBrowserViewForBrowser(browser())->tabstrip();
+  Tab* tab = tab_strip->tab_at(0);
+  tab_strip->GetFocusManager()->SetFocusedView(tab);
+  TabHoverCardBubbleView* hover_card = GetHoverCard(tab_strip);
+  Widget* widget = GetHoverCardWidget(hover_card);
+  HoverCardVisibleWaiter waiter(widget);
+  waiter.Wait();
+  EXPECT_TRUE(widget != nullptr);
+  EXPECT_TRUE(widget->IsVisible());
+  tab_strip->GetFocusManager()->SetFocusedView(tab->close_button_);
+  waiter.Wait();
+  EXPECT_TRUE(widget != nullptr);
+  EXPECT_TRUE(widget->IsVisible());
+}
+
+// Verify hover card is visible when tab is focused and a key is pressed.
+IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
+                       WidgetVisibleOnKeyPressAfterTabFocus) {
+  TabStrip* tab_strip =
+      BrowserView::GetBrowserViewForBrowser(browser())->tabstrip();
+  Tab* tab = tab_strip->tab_at(0);
+  tab_strip->GetFocusManager()->SetFocusedView(tab);
+  TabHoverCardBubbleView* hover_card = GetHoverCard(tab_strip);
+  Widget* widget = GetHoverCardWidget(hover_card);
+  HoverCardVisibleWaiter waiter(widget);
+  waiter.Wait();
+  EXPECT_TRUE(widget != nullptr);
+  EXPECT_TRUE(widget->IsVisible());
+
+  ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_SPACE, 0);
+  tab->OnKeyPressed(key_event);
+  EXPECT_TRUE(widget->IsVisible());
+}
+
+// Verify hover card is not visible when tab is focused and the mouse is
+// pressed.
+IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
+                       WidgetNotVisibleOnMousePressAfterTabFocus) {
+  TabStrip* tab_strip =
+      BrowserView::GetBrowserViewForBrowser(browser())->tabstrip();
+  Tab* tab = tab_strip->tab_at(0);
+  tab_strip->GetFocusManager()->SetFocusedView(tab);
+  TabHoverCardBubbleView* hover_card = GetHoverCard(tab_strip);
+  Widget* widget = GetHoverCardWidget(hover_card);
+  HoverCardVisibleWaiter waiter(widget);
+  waiter.Wait();
+  EXPECT_TRUE(widget != nullptr);
+  EXPECT_TRUE(widget->IsVisible());
+
+  ClickMouseOnTab();
+  EXPECT_FALSE(widget->IsVisible());
+}
+
 // Verify hover card is not visible after clicking on a tab.
 IN_PROC_BROWSER_TEST_F(TabHoverCardBubbleViewBrowserTest,
                        WidgetNotVisibleOnClick) {

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -147,8 +147,8 @@ class TabHoverCardEventSniffer : public ui::EventHandler {
  protected:
   // ui::EventTarget:
   void OnKeyEvent(ui::KeyEvent* event) override {
-    if (!tab_strip_->pane_has_focus())
-      hover_card_->FadeOutToHide();
+    if (!tab_strip_->IsFocusInTabs())
+      tab_strip_->UpdateHoverCard(nullptr, false);
   }
 
   void OnMouseEvent(ui::MouseEvent* event) override {
@@ -1525,6 +1525,11 @@ bool TabStrip::IsLastVisibleTab(const Tab* tab) const {
   return GetLastVisibleTab() == tab;
 }
 
+bool TabStrip::IsFocusInTabs() const {
+  return GetFocusManager() && Contains(GetFocusManager()->GetFocusedView()) &&
+         GetFocusManager()->GetFocusedView() != new_tab_button_;
+}
+
 void TabStrip::MaybeStartDrag(
     Tab* tab,
     const ui::LocatedEvent& event,

chrome/browser/ui/views/tabs/tab_strip.h

@@ -239,6 +239,7 @@ class TabStrip : public views::AccessiblePaneView,
   bool IsTabPinned(const Tab* tab) const override;
   bool IsFirstVisibleTab(const Tab* tab) const override;
   bool IsLastVisibleTab(const Tab* tab) const override;
+  bool IsFocusInTabs() const override;
   void MaybeStartDrag(
       Tab* tab,
       const ui::LocatedEvent& event,

chrome/browser/ui/views/tabs/tab_unittest.cc

@@ -64,6 +64,7 @@ class FakeTabController : public TabController {
   bool IsTabPinned(const Tab* tab) const override { return false; }
   bool IsFirstVisibleTab(const Tab* tab) const override { return false; }
   bool IsLastVisibleTab(const Tab* tab) const override { return false; }
+  bool IsFocusInTabs() const override { return false; }
   void MaybeStartDrag(
       Tab* tab,
       const ui::LocatedEvent& event,