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}
diff --git a/chrome/browser/ui/views/tabs/tab.cc b/chrome/browser/ui/views/tabs/tab.cc
index c9a9b07e..656de71 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/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 @@
} // 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 @@
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() {
- 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,18 +637,19 @@
}
void Tab::OnFocus() {
- controller_->UpdateHoverCard(this, /* should_show */ true);
View::OnFocus();
+ controller_->UpdateHoverCard(this, /* should_show */ true);
+}
+
+void Tab::OnBlur() {
+ View::OnBlur();
+ controller_->UpdateHoverCard(nullptr, /* should_show */ false);
}
void Tab::OnThemeChanged() {
UpdateForegroundColors();
}
-void Tab::OnViewFocused(views::View* observed_view) {
- controller_->UpdateHoverCard(this, /* should_show */ true);
-}
-
void Tab::SetClosing(bool closing) {
closing_ = closing;
ActiveStateChanged();
diff --git a/chrome/browser/ui/views/tabs/tab.h b/chrome/browser/ui/views/tabs/tab.h
index e26dd4f..6c28cd1a 100644
--- a/chrome/browser/ui/views/tabs/tab.h
+++ b/chrome/browser/ui/views/tabs/tab.h
@@ -99,11 +99,9 @@
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 @@
TabAlertState alert_state);
private:
+ class TabCloseButtonObserver;
friend class AlertIndicatorTest;
friend class TabTest;
friend class TabStripTest;
@@ -195,6 +194,8 @@
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 @@
// 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_;
diff --git a/chrome/browser/ui/views/tabs/tab_controller.h b/chrome/browser/ui/views/tabs/tab_controller.h
index aabed99..900ecfa0 100644
--- a/chrome/browser/ui/views/tabs/tab_controller.h
+++ b/chrome/browser/ui/views/tabs/tab_controller.h
@@ -76,6 +76,9 @@
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,
diff --git a/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.cc b/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.cc
index 8a3dc44..9ea9985 100644
--- a/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.cc
+++ b/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.cc
@@ -377,8 +377,15 @@
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 @@
}
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 {
diff --git a/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc b/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc
index 2c701e91..f0fe341 100644
--- a/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc
+++ b/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 @@
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) {
diff --git a/chrome/browser/ui/views/tabs/tab_strip.cc b/chrome/browser/ui/views/tabs/tab_strip.cc
index e56e5ff1..ed0456e 100644
--- a/chrome/browser/ui/views/tabs/tab_strip.cc
+++ b/chrome/browser/ui/views/tabs/tab_strip.cc
@@ -147,8 +147,8 @@
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 @@
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,
diff --git a/chrome/browser/ui/views/tabs/tab_strip.h b/chrome/browser/ui/views/tabs/tab_strip.h
index 478519e..7c56a2d 100644
--- a/chrome/browser/ui/views/tabs/tab_strip.h
+++ b/chrome/browser/ui/views/tabs/tab_strip.h
@@ -239,6 +239,7 @@
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,
diff --git a/chrome/browser/ui/views/tabs/tab_unittest.cc b/chrome/browser/ui/views/tabs/tab_unittest.cc
index b7059dd6..56fa08da 100644
--- a/chrome/browser/ui/views/tabs/tab_unittest.cc
+++ b/chrome/browser/ui/views/tabs/tab_unittest.cc
@@ -64,6 +64,7 @@
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,
