Fix pane focus accelerator handling for Help Bubbles.
Previously, HelpBubbleViews::ToggleFocusForAccessibility() was
attempting to determine where focus was and then appropriately focusing
the bubble or the element it was anchored to.
However, this was triggered on F6 (CMD+OPT+DOWN on Mac), which is a
browser-specific accelerator. The logic chain becomes:
- User presses accelerator
- If focus is in browser, ToggleFocusForAccessibility() gets called as
expected, which detects focus is not in bubble, so bubble is focused
- If focus in in bubble:
- Accelerator processing determines that bubble does not handle the
accelerator
- Accelerator processing hands the accelerator off to the anchor
view, which implicitly focuses the anchor view
- ToggleFocusForAccessibility() gets called, but focus is no longer
in the help bubble
- The help bubble is re-focused (incorrect behavior)
We previously had a complicated workaround that did not work in all
cases (specifically when the anchor view was not in the main browser
view, but in another bubble).
This CL replaces this with actually handling the accelerator for rotate
focus in the bubble itself. Since HelpBubbleViews is not specific to
browser (it will be componentized later), we create a new delegate class
that is capable of reporting the accelerators used for focus toggle, and
explicitly registering for those with the correct handler (which calls
ToggleFocusForAccessibility() directly).
Some small changes needed to be added to
constructor argument forwarding, but those should have been there all
along.
FrameworkSpecificImplementation: :MaybeRegister() to allow for
Change-Id: I6bd0458d75ca661d5bec421ba3ab311978eb6590
Bug: 1310126, 1310124
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3550606
Reviewed-by: David Pennington <dpenning@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/main@{#986305}
diff --git a/chrome/browser/ui/user_education/help_bubble_factory_registry.h b/chrome/browser/ui/user_education/help_bubble_factory_registry.h
index c50df19..d493b6e 100644
--- a/chrome/browser/ui/user_education/help_bubble_factory_registry.h
+++ b/chrome/browser/ui/user_education/help_bubble_factory_registry.h
@@ -6,6 +6,7 @@
#define CHROME_BROWSER_UI_USER_EDUCATION_HELP_BUBBLE_FACTORY_REGISTRY_H_
#include <map>
+#include <utility>
#include <vector>
#include "base/callback_list.h"
@@ -47,9 +48,9 @@
// Adds a bubble factory of type `T` to the list of bubble factories, if it
// is not already present.
- template <class T>
- void MaybeRegister() {
- factories_.MaybeRegister<T>();
+ template <class T, typename... Args>
+ void MaybeRegister(Args&&... args) {
+ factories_.MaybeRegister<T>(std::forward<Args>(args)...);
}
private:
diff --git a/chrome/browser/ui/views/user_education/browser_user_education_service.cc b/chrome/browser/ui/views/user_education/browser_user_education_service.cc
index 0da7a85..2b69bf7 100644
--- a/chrome/browser/ui/views/user_education/browser_user_education_service.cc
+++ b/chrome/browser/ui/views/user_education/browser_user_education_service.cc
@@ -4,6 +4,8 @@
#include "chrome/browser/ui/views/user_education/browser_user_education_service.h"
+#include <vector>
+
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
#include "chrome/app/chrome_command_ids.h"
@@ -15,7 +17,7 @@
#include "chrome/browser/ui/user_education/help_bubble_params.h"
#include "chrome/browser/ui/user_education/tutorial/tutorial_description.h"
#include "chrome/browser/ui/user_education/tutorial/tutorial_registry.h"
-#include "chrome/browser/ui/views/tabs/tab_strip.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/user_education/help_bubble_factory_views.h"
#include "chrome/grit/chromium_strings.h"
#include "chrome/grit/generated_resources.h"
@@ -26,6 +28,9 @@
#include "ui/base/interaction/element_tracker.h"
#include "ui/base/interaction/interaction_sequence.h"
#include "ui/views/interaction/element_tracker_views.h"
+#include "ui/views/view.h"
+#include "ui/views/view_utils.h"
+#include "ui/views/widget/widget.h"
#if BUILDFLAG(IS_MAC)
#include "chrome/browser/ui/views/user_education/help_bubble_factory_mac.h"
@@ -36,12 +41,43 @@
const char kTabGroupTutorialMetricPrefix[] = "TabGroup";
constexpr char kTabGroupHeaderElementName[] = "TabGroupHeader";
+class BrowserHelpBubbleAcceleratorDelegate
+ : public HelpBubbleAcceleratorDelegate {
+ public:
+ BrowserHelpBubbleAcceleratorDelegate() = default;
+ ~BrowserHelpBubbleAcceleratorDelegate() override = default;
+
+ std::vector<ui::Accelerator> GetPaneNavigationAccelerators(
+ ui::TrackedElement* anchor_element) const override {
+ std::vector<ui::Accelerator> result;
+ if (anchor_element->IsA<views::TrackedElementViews>()) {
+ auto* widget = anchor_element->AsA<views::TrackedElementViews>()
+ ->view()
+ ->GetWidget();
+ if (widget) {
+ auto* const client_view =
+ widget->GetPrimaryWindowWidget()->client_view();
+ if (client_view && views::IsViewClass<BrowserView>(client_view)) {
+ auto* const browser_view = static_cast<BrowserView*>(client_view);
+ ui::Accelerator accel;
+ if (browser_view->GetAccelerator(IDC_FOCUS_NEXT_PANE, &accel))
+ result.push_back(accel);
+ if (browser_view->GetAccelerator(IDC_FOCUS_PREVIOUS_PANE, &accel))
+ result.push_back(accel);
+ }
+ }
+ }
+ return result;
+ }
+};
+
} // namespace
const char kTabGroupTutorialId[] = "Tab Group Tutorial";
void RegisterChromeHelpBubbleFactories(HelpBubbleFactoryRegistry& registry) {
- registry.MaybeRegister<HelpBubbleFactoryViews>();
+ static base::NoDestructor<BrowserHelpBubbleAcceleratorDelegate> delegate;
+ registry.MaybeRegister<HelpBubbleFactoryViews>(delegate.get());
#if BUILDFLAG(IS_MAC)
registry.MaybeRegister<HelpBubbleFactoryMac>();
#endif
diff --git a/chrome/browser/ui/views/user_education/help_bubble_factory_views.cc b/chrome/browser/ui/views/user_education/help_bubble_factory_views.cc
index 063e4ad..d4679912 100644
--- a/chrome/browser/ui/views/user_education/help_bubble_factory_views.cc
+++ b/chrome/browser/ui/views/user_education/help_bubble_factory_views.cc
@@ -43,50 +43,51 @@
if (!help_bubble_view_)
return false;
- // Focus can't be determined just by widget activity; we must check to see if
- // there's a focused view in the anchor widget or the top-level browser
- // widget (if different).
- auto* const anchor = help_bubble_view_->GetAnchorView();
- const bool is_focus_in_ancestor_widget =
- (anchor && anchor->GetFocusManager()->GetFocusedView()) ||
- help_bubble_view_->GetWidget()
- ->GetPrimaryWindowWidget()
- ->GetFocusManager()
- ->GetFocusedView();
-
// If the focus isn't in the help bubble, focus the help bubble.
// Note that if is_focus_in_ancestor_widget is true, then anchor both exists
// and has a widget, so anchor->GetWidget() will always be valid.
- if (is_focus_in_ancestor_widget) {
+ if (!help_bubble_view_->IsFocusInHelpBubble()) {
help_bubble_view_->GetWidget()->Activate();
help_bubble_view_->RequestFocus();
return true;
}
+ auto* const anchor = help_bubble_view_->GetAnchorView();
if (!anchor)
return false;
- // An AccessiblePaneView can receive focus, but is not necessarily itself
- // accessibility focusable. Use the built-in functionality for focusing
- // elements of AccessiblePaneView instead.
- if (!anchor->IsAccessibilityFocusable()) {
- if (views::IsViewClass<views::AccessiblePaneView>(anchor)) {
- // You can't focus an accessible pane if it's already in accessibility
- // mode, so avoid doing that; the SetPaneFocus() call will go back into
- // accessibility navigation mode.
- anchor->GetFocusManager()->SetKeyboardAccessible(false);
- return static_cast<views::AccessiblePaneView*>(anchor)->SetPaneFocus(
- nullptr);
- } else {
- return false;
- }
+ bool set_focus = false;
+ if (anchor->IsAccessibilityFocusable()) {
+#if BUILDFLAG(IS_MAC)
+ // Mac does not automatically pass activation on focus, so we have to do it
+ // manually.
+ anchor->GetWidget()->Activate();
+#else
+ // Focus the anchor. We can't request focus for an accessibility-only view
+ // until we turn on keyboard accessibility for its focus manager.
+ anchor->GetFocusManager()->SetKeyboardAccessible(true);
+#endif
+ anchor->RequestFocus();
+ set_focus = true;
+ } else if (views::IsViewClass<views::AccessiblePaneView>(anchor)) {
+ // An AccessiblePaneView can receive focus, but is not necessarily itself
+ // accessibility focusable. Use the built-in functionality for focusing
+ // elements of AccessiblePaneView instead.
+#if BUILDFLAG(IS_MAC)
+ // Mac does not automatically pass activation on focus, so we have to do it
+ // manually.
+ anchor->GetWidget()->Activate();
+#else
+ // You can't focus an accessible pane if it's already in accessibility
+ // mode, so avoid doing that; the SetPaneFocus() call will go back into
+ // accessibility navigation mode.
+ anchor->GetFocusManager()->SetKeyboardAccessible(false);
+#endif
+ set_focus =
+ static_cast<views::AccessiblePaneView*>(anchor)->SetPaneFocus(nullptr);
}
- // Focus the anchor. We can't request focus for an accessibility-only view
- // until we turn on keyboard accessibility for its focus manager.
- anchor->GetFocusManager()->SetKeyboardAccessible(true);
- anchor->RequestFocus();
- return true;
+ return set_focus;
}
void HelpBubbleViews::OnAnchorBoundsChanged() {
@@ -106,6 +107,20 @@
: ui::ElementContext();
}
+bool HelpBubbleViews::AcceleratorPressed(const ui::Accelerator& accelerator) {
+ if (CanHandleAccelerators()) {
+ ToggleFocusForAccessibility();
+ return true;
+ }
+
+ return false;
+}
+
+bool HelpBubbleViews::CanHandleAccelerators() const {
+ return help_bubble_view_ && help_bubble_view_->GetWidget() &&
+ help_bubble_view_->GetWidget()->IsActive();
+}
+
void HelpBubbleViews::MaybeResetAnchorView() {
if (!help_bubble_view_)
return;
@@ -136,7 +151,12 @@
OnWidgetClosing(widget);
}
-HelpBubbleFactoryViews::HelpBubbleFactoryViews() = default;
+HelpBubbleFactoryViews::HelpBubbleFactoryViews(
+ HelpBubbleAcceleratorDelegate* accelerator_delegate)
+ : accelerator_delegate_(accelerator_delegate) {
+ DCHECK(accelerator_delegate_);
+}
+
HelpBubbleFactoryViews::~HelpBubbleFactoryViews() = default;
std::unique_ptr<HelpBubble> HelpBubbleFactoryViews::CreateBubble(
@@ -145,8 +165,15 @@
views::View* const anchor_view =
element->AsA<views::TrackedElementViews>()->view();
anchor_view->SetProperty(kHasInProductHelpPromoKey, true);
- return base::WrapUnique(
+ auto result = base::WrapUnique(
new HelpBubbleViews(new HelpBubbleView(anchor_view, std::move(params))));
+ for (const auto& accelerator :
+ accelerator_delegate_->GetPaneNavigationAccelerators(element)) {
+ result->bubble_view()->GetFocusManager()->RegisterAccelerator(
+ accelerator, ui::AcceleratorManager::HandlerPriority::kNormalPriority,
+ result.get());
+ }
+ return result;
}
bool HelpBubbleFactoryViews::CanBuildBubbleForTrackedElement(
diff --git a/chrome/browser/ui/views/user_education/help_bubble_factory_views.h b/chrome/browser/ui/views/user_education/help_bubble_factory_views.h
index 909c585..06039c8 100644
--- a/chrome/browser/ui/views/user_education/help_bubble_factory_views.h
+++ b/chrome/browser/ui/views/user_education/help_bubble_factory_views.h
@@ -11,6 +11,7 @@
#include "chrome/browser/ui/user_education/help_bubble_factory.h"
#include "chrome/browser/ui/user_education/help_bubble_params.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
+#include "ui/base/accelerators/accelerator.h"
#include "ui/base/interaction/element_identifier.h"
#include "ui/base/interaction/element_tracker.h"
#include "ui/base/interaction/framework_specific_implementation.h"
@@ -20,12 +21,31 @@
class HelpBubbleView;
+// Provides access to pane navigation accelerators so we can properly handle
+// them.
+class HelpBubbleAcceleratorDelegate {
+ public:
+ HelpBubbleAcceleratorDelegate() = default;
+ HelpBubbleAcceleratorDelegate(const HelpBubbleAcceleratorDelegate&) = delete;
+ void operator=(const HelpBubbleAcceleratorDelegate&) = delete;
+ virtual ~HelpBubbleAcceleratorDelegate() = default;
+
+ // Gets a list of accelerators that can be used to navigate panes, which
+ // should trigger HelpBubble::ToggleFocusForAccessibility(). We need this
+ // because we do not by default have access to the current app's
+ // accelerator provider nor to the specific command IDs.
+ virtual std::vector<ui::Accelerator> GetPaneNavigationAccelerators(
+ ui::TrackedElement* anchor_element) const = 0;
+};
+
// Views-specific implementation of the help bubble.
//
// Because this is a FrameworkSpecificImplementation, you can use:
// help_bubble->AsA<HelpBubbleViews>()->bubble_view()
// to retrieve the underlying bubble view.
-class HelpBubbleViews : public HelpBubble, public views::WidgetObserver {
+class HelpBubbleViews : public HelpBubble,
+ public views::WidgetObserver,
+ public ui::AcceleratorTarget {
public:
~HelpBubbleViews() override;
@@ -42,6 +62,10 @@
gfx::Rect GetBoundsInScreen() const override;
ui::ElementContext GetContext() const override;
+ // ui::AcceleratorTarget
+ bool AcceleratorPressed(const ui::Accelerator& accelerator) override;
+ bool CanHandleAccelerators() const override;
+
private:
friend class HelpBubbleFactoryViews;
friend class HelpBubbleFactoryMac;
@@ -66,7 +90,8 @@
// Factory implementation for HelpBubbleViews.
class HelpBubbleFactoryViews : public HelpBubbleFactory {
public:
- HelpBubbleFactoryViews();
+ explicit HelpBubbleFactoryViews(
+ HelpBubbleAcceleratorDelegate* accelerator_delegate);
~HelpBubbleFactoryViews() override;
DECLARE_FRAMEWORK_SPECIFIC_METADATA()
@@ -76,6 +101,9 @@
HelpBubbleParams params) override;
bool CanBuildBubbleForTrackedElement(
const ui::TrackedElement* element) const override;
+
+ private:
+ base::raw_ptr<HelpBubbleAcceleratorDelegate> accelerator_delegate_;
};
#endif // CHROME_BROWSER_UI_VIEWS_USER_EDUCATION_HELP_BUBBLE_FACTORY_VIEWS_H_
diff --git a/chrome/browser/ui/views/user_education/help_bubble_factory_views_interactive_uitest.cc b/chrome/browser/ui/views/user_education/help_bubble_factory_views_interactive_uitest.cc
index af3021af..e9d9623 100644
--- a/chrome/browser/ui/views/user_education/help_bubble_factory_views_interactive_uitest.cc
+++ b/chrome/browser/ui/views/user_education/help_bubble_factory_views_interactive_uitest.cc
@@ -4,7 +4,9 @@
#include "base/callback_forward.h"
#include "build/build_config.h"
+#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
#include "chrome/browser/ui/browser_element_identifiers.h"
#include "chrome/browser/ui/user_education/help_bubble_factory_registry.h"
#include "chrome/browser/ui/user_education/help_bubble_params.h"
@@ -82,3 +84,58 @@
#endif
EXPECT_TRUE(GetAnchorElement()->view()->HasFocus());
}
+
+// Sending accelerators to the main window is flaky at best and doesn't work at
+// all at worst on Mac, so we can't test it directly here.
+// See http://crbug.com/824551 and ConstrainedWindowViewTest.FocusTest for
+// examples of the tests simply not working on Mac.
+// #if BUILDFLAG(IS_MAC)
+// #define MAYBE_ToggleFocusViaAccelerator DISABLED_ToggleFocusViaAccelerator
+// #else
+// #define MAYBE_ToggleFocusViaAccelerator ToggleFocusViaAccelerator
+// #endif
+
+IN_PROC_BROWSER_TEST_F(HelpBubbleFactoryViewsUiTest,
+ ToggleFocusViaAccelerator) {
+ HelpBubbleParams params;
+ params.body_text = u"Hello world!";
+ auto help_bubble_ptr =
+ registry()->CreateHelpBubble(GetAnchorElement(), std::move(params));
+ auto* const browser_view = BrowserView::GetBrowserViewForBrowser(browser());
+ auto* const bubble_view =
+ help_bubble_ptr->AsA<HelpBubbleViews>()->bubble_view();
+
+#if BUILDFLAG(IS_MAC)
+
+ // Focus the help bubble.
+ views::test::WidgetActivationWaiter bubble_waiter(bubble_view->GetWidget(),
+ true);
+ ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FOCUS_NEXT_PANE));
+ bubble_waiter.Wait();
+
+ // Focus the browser.
+ views::test::WidgetActivationWaiter browser_waiter(browser_view->GetWidget(),
+ true);
+ ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FOCUS_NEXT_PANE));
+ browser_waiter.Wait();
+
+#else // !BUILDFLAG(IS_MAC)
+
+ // Get the appropriate accelerator.
+ ui::Accelerator accel;
+ ASSERT_TRUE(browser_view->GetAccelerator(IDC_FOCUS_NEXT_PANE, &accel));
+
+ // Focus the help bubble.
+ views::test::WidgetActivationWaiter bubble_waiter(bubble_view->GetWidget(),
+ true);
+ ASSERT_TRUE(browser_view->GetFocusManager()->ProcessAccelerator(accel));
+ bubble_waiter.Wait();
+
+ // Focus the browser.
+ views::test::WidgetActivationWaiter browser_waiter(browser_view->GetWidget(),
+ true);
+ ASSERT_TRUE(bubble_view->GetFocusManager()->ProcessAccelerator(accel));
+ browser_waiter.Wait();
+
+#endif // BUILDFLAG(IS_MAC)
+}
diff --git a/chrome/browser/ui/views/user_education/help_bubble_view.cc b/chrome/browser/ui/views/user_education/help_bubble_view.cc
index e5cdc4ff..7d4690c3 100644
--- a/chrome/browser/ui/views/user_education/help_bubble_view.cc
+++ b/chrome/browser/ui/views/user_education/help_bubble_view.cc
@@ -388,12 +388,11 @@
}
// Add close button (optional).
- ClosePromoButton* close_button = nullptr;
if (params.buttons.empty() || params.force_close_button) {
int close_string_id =
params.tutorial_progress ? IDS_CLOSE_TUTORIAL : IDS_CLOSE_PROMO;
// Since we set the cancel callback, we will use CancelDialog() to dismiss.
- close_button =
+ close_button_ =
(params.tutorial_progress ? progress_container : top_text_container)
->AddChildView(std::make_unique<ClosePromoButton>(
close_string_id,
@@ -483,15 +482,15 @@
views::FlexSpecification(progress_layout.GetDefaultFlexRule()));
// Close button should float right in whatever container it's in.
- if (close_button) {
- close_button->SetProperty(
+ if (close_button_) {
+ close_button_->SetProperty(
views::kFlexBehaviorKey,
views::FlexSpecification(views::LayoutOrientation::kHorizontal,
views::MinimumFlexSizeRule::kPreferred,
views::MaximumFlexSizeRule::kUnbounded)
.WithAlignment(views::LayoutAlignment::kEnd));
- close_button->SetProperty(views::kMarginsKey,
- gfx::Insets::TLBR(0, default_spacing, 0, 0));
+ close_button_->SetProperty(views::kMarginsKey,
+ gfx::Insets::TLBR(0, default_spacing, 0, 0));
}
// Icon view should have padding between it and the title or body label.
@@ -563,8 +562,8 @@
// Want a consistent initial focused view if one is available.
if (!button_container->children().empty()) {
SetInitiallyFocusedView(button_container->children()[0]);
- } else if (close_button) {
- SetInitiallyFocusedView(close_button);
+ } else if (close_button_) {
+ SetInitiallyFocusedView(close_button_);
}
SetProperty(views::kElementIdentifierKey, kHelpBubbleElementIdForTesting);
@@ -681,6 +680,22 @@
return contents && views::IsViewClass<HelpBubbleView>(contents);
}
+bool HelpBubbleView::IsFocusInHelpBubble() const {
+#if BUILDFLAG(IS_MAC)
+ if (close_button_ && close_button_->HasFocus())
+ return true;
+ if (default_button_ && default_button_->HasFocus())
+ return true;
+ for (auto* button : non_default_buttons_) {
+ if (button->HasFocus())
+ return true;
+ }
+ return false;
+#else
+ return GetWidget()->IsActive();
+#endif
+}
+
views::LabelButton* HelpBubbleView::GetDefaultButtonForTesting() const {
return default_button_;
}
diff --git a/chrome/browser/ui/views/user_education/help_bubble_view.h b/chrome/browser/ui/views/user_education/help_bubble_view.h
index 0d355c0c..8ba6a95 100644
--- a/chrome/browser/ui/views/user_education/help_bubble_view.h
+++ b/chrome/browser/ui/views/user_education/help_bubble_view.h
@@ -48,6 +48,8 @@
// Returns whether the given dialog is a help bubble.
static bool IsHelpBubble(views::DialogDelegate* dialog);
+ bool IsFocusInHelpBubble() const;
+
views::LabelButton* GetDefaultButtonForTesting() const;
views::LabelButton* GetNonDefaultButtonForTesting(int index) const;
@@ -80,7 +82,8 @@
// If the bubble has buttons, it must be focusable.
std::vector<views::MdTextButton*> non_default_buttons_;
- base::raw_ptr<views::MdTextButton> default_button_;
+ base::raw_ptr<views::MdTextButton> default_button_ = nullptr;
+ base::raw_ptr<views::Button> close_button_ = nullptr;
// This is the base accessible name of the window.
std::u16string accessible_name_;
diff --git a/ui/base/interaction/framework_specific_implementation.h b/ui/base/interaction/framework_specific_implementation.h
index 5113ffa..ee3bb210 100644
--- a/ui/base/interaction/framework_specific_implementation.h
+++ b/ui/base/interaction/framework_specific_implementation.h
@@ -157,13 +157,18 @@
}
// Adds an instance of `DerivedClass` if it is not already present.
- template <class DerivedClass>
- void MaybeRegister() {
+ // Additional arguments in `params` will only be consumed if the class needs
+ // to be added, so do not allocate resources that are not scoped and movable
+ // (i.e. pass a std::unique_ptr rather than "new X", so the object will be
+ // properly cleaned up if it is not used).
+ template <class DerivedClass, typename... Args>
+ void MaybeRegister(Args&&... args) {
for (const auto& instance : instances_) {
if (instance->template IsA<DerivedClass>())
return;
}
- instances_.push_back(std::make_unique<DerivedClass>());
+ instances_.push_back(
+ std::make_unique<DerivedClass>(std::forward<Args>(args)...));
}
private:
