Introduce a11y selection interface to allow testing of PopupRowView.

This CL adds an interface for a11y selection announcements, which is implemented by PopupBaseView. The introduction of this interface allows to test PopupRowView independently of its container, PopupViewViews. The CL also adds initial unit tests for PopupRowView. More will follow once control surfaces are supported. Bug: 1411172 Change-Id: Ifa01bb096751d011fafaeb27b2dca7e5c7da3c86 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4254826 Commit-Queue: Jan Keitel <jkeitel@google.com> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Cr-Commit-Position: refs/heads/main@{#1107395}
chromium · Feb 20, 2023 · da38733 · da38733
1 parent 04c1946
commit da38733
Show file tree

Hide file tree

Showing 9 changed files with 304 additions and 74 deletions.
diff --git a/chrome/browser/ui/views/autofill/popup/popup_base_view.cc b/chrome/browser/ui/views/autofill/popup/popup_base_view.cc
@@ -46,6 +46,35 @@
 
 namespace autofill {
 
+namespace {
+
+// The maximum number of pixels the suggestions dialog is shifted towards the
+// center the focused field.
+constexpr int kMaximumPixelsToMoveSuggestionToCenter = 120;
+
+// The maximum width percentage the suggestion dialog is shifted towards the
+// center of the focused field.
+constexpr int kMaximumWidthPercentageToMoveTheSuggestionToCenter = 50;
+
+}  // namespace
+
+// static
+int PopupBaseView::GetCornerRadius() {
+  return ChromeLayoutProvider::Get()->GetCornerRadiusMetric(
+      views::Emphasis::kMedium);
+}
+
+// static
+int PopupBaseView::GetHorizontalMargin() {
+  return views::MenuConfig::instance().item_horizontal_padding +
+         GetCornerRadius();
+}
+
+// static
+int PopupBaseView::GetHorizontalPadding() {
+  return GetHorizontalMargin();
+}
+
 // The widget that the PopupBaseView will be attached to.
 class PopupBaseView::Widget : public views::Widget {
  public:
@@ -68,21 +97,23 @@ class PopupBaseView::Widget : public views::Widget {
 
   // views::Widget:
   const ui::ThemeProvider* GetThemeProvider() const override {
-    if (!autofill_popup_base_view_ || !autofill_popup_base_view_->browser()) {
+    if (!autofill_popup_base_view_ ||
+        !autofill_popup_base_view_->GetBrowser()) {
       return nullptr;
     }
 
     return &ThemeService::GetThemeProviderForProfile(
-        autofill_popup_base_view_->browser()->profile());
+        autofill_popup_base_view_->GetBrowser()->profile());
   }
 
   views::Widget* GetPrimaryWindowWidget() override {
-    if (!autofill_popup_base_view_ || !autofill_popup_base_view_->browser()) {
+    if (!autofill_popup_base_view_ ||
+        !autofill_popup_base_view_->GetBrowser()) {
       return nullptr;
     }
 
     BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(
-        autofill_popup_base_view_->browser());
+        autofill_popup_base_view_->GetBrowser());
     if (!browser_view) {
       return nullptr;
     }
@@ -94,31 +125,9 @@ class PopupBaseView::Widget : public views::Widget {
   const raw_ptr<PopupBaseView, DanglingUntriaged> autofill_popup_base_view_;
 };
 
-// static
-int PopupBaseView::GetCornerRadius() {
-  return ChromeLayoutProvider::Get()->GetCornerRadiusMetric(
-      views::Emphasis::kMedium);
-}
-
-// static
-int PopupBaseView::GetHorizontalMargin() {
-  return views::MenuConfig::instance().item_horizontal_padding +
-         GetCornerRadius();
-}
-
-// static
-int PopupBaseView::GetHorizontalPadding() {
-  return GetHorizontalMargin();
-}
-
 PopupBaseView::PopupBaseView(base::WeakPtr<AutofillPopupViewDelegate> delegate,
                              views::Widget* parent_widget)
-    : delegate_(delegate), parent_widget_(parent_widget) {
-  // GetWebContents() may return nullptr in some tests.
-  if (GetWebContents()) {
-    browser_ = chrome::FindBrowserWithWebContents(GetWebContents());
-  }
-}
+    : delegate_(delegate), parent_widget_(parent_widget) {}
 
 PopupBaseView::~PopupBaseView() {
   if (delegate_) {
@@ -130,6 +139,13 @@ PopupBaseView::~PopupBaseView() {
   CHECK(!IsInObserverList());
 }
 
+Browser* PopupBaseView::GetBrowser() {
+  if (content::WebContents* web_contents = GetWebContents()) {
+    return chrome::FindBrowserWithWebContents(web_contents);
+  }
+  return nullptr;
+}
+
 bool PopupBaseView::DoShow() {
   const bool initialize_widget = !GetWidget();
   if (initialize_widget) {
@@ -202,7 +218,7 @@ void PopupBaseView::DoHide() {
   }
 }
 
-void PopupBaseView::NotifyAXSelection(View& selected_view) {
+void PopupBaseView::NotifyAXSelection(views::View& selected_view) {
   if (!is_ax_menu_start_event_fired_) {
     // Fire the menu start event once, right before the first item is selected.
     // By firing these and the matching kMenuEnd events, we are telling screen
@@ -321,7 +337,7 @@ gfx::Rect PopupBaseView::GetOptionalPositionAndPlaceArrowOnPopup(
   int maximum_pixel_offset_to_center =
       base::FeatureList::IsEnabled(features::kAutofillMoreProminentPopup)
           ? features::kAutofillMoreProminentPopupMaxOffsetToCenterParam.Get()
-          : kMaximumPixelsToMoveSuggstionToCenter;
+          : kMaximumPixelsToMoveSuggestionToCenter;
 
   // Deduce the arrow and the position.
   views::BubbleBorder::Arrow arrow = GetOptimalPopupPlacement(

diff --git a/chrome/browser/ui/views/autofill/popup/popup_base_view.h b/chrome/browser/ui/views/autofill/popup/popup_base_view.h
@@ -12,6 +12,8 @@
 #include "build/build_config.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view_delegate.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/views/autofill/popup/popup_base_view.h"
+#include "chrome/browser/ui/views/autofill/popup/popup_row_view.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/metadata/metadata_header_macros.h"
 #include "ui/views/focus/widget_focus_manager.h"
@@ -23,24 +25,17 @@ namespace autofill {
 
 // Class that deals with the event handling for Autofill-style popups. This
 // class should only be instantiated by sub-classes.
-class PopupBaseView : public views::WidgetDelegateView,
+class PopupBaseView : public PopupRowView::AccessibilitySelectionDelegate,
+                      public views::WidgetDelegateView,
                       public views::WidgetFocusChangeListener,
                       public views::WidgetObserver {
  public:
-  METADATA_HEADER(PopupBaseView);
-
-  // Consider the input element is |kElementBorderPadding| pixels larger at the
+  // Consider the input element is `kElementBorderPadding` pixels larger at the
   // top and at the bottom in order to reposition the dropdown, so that it
   // doesn't look too close to the element.
   static constexpr int kElementBorderPadding = 1;
 
-  // The maximum number of pixels the suggestions dialog is shifted towards the
-  // center the focused field..
-  static constexpr int kMaximumPixelsToMoveSuggstionToCenter = 120;
-
-  // The maximum width percentage the suggestion dialog is shifted towards the
-  // center of the focused field.
-  static constexpr int kMaximumWidthPercentageToMoveTheSuggestionToCenter = 50;
+  METADATA_HEADER(PopupBaseView);
 
   PopupBaseView(const PopupBaseView&) = delete;
   PopupBaseView& operator=(const PopupBaseView&) = delete;
@@ -53,9 +48,10 @@ class PopupBaseView : public views::WidgetDelegateView,
   static int GetHorizontalPadding();
 
   // Notify accessibility that an item has been selected.
-  void NotifyAXSelection(View& view);
+  void NotifyAXSelection(views::View& view) override;
 
-  Browser* browser() { return browser_; }
+  // Returns the browser in which this popup is shown.
+  Browser* GetBrowser();
 
  protected:
   PopupBaseView(base::WeakPtr<AutofillPopupViewDelegate> delegate,
@@ -131,10 +127,7 @@ class PopupBaseView : public views::WidgetDelegateView,
   base::WeakPtr<AutofillPopupViewDelegate> delegate_;
 
   // The widget of the window that triggered this popup. Weak reference.
-  raw_ptr<views::Widget> parent_widget_;
-
-  // The browser this popup is shown in.
-  raw_ptr<Browser> browser_;
+  raw_ptr<views::Widget> parent_widget_ = nullptr;
 
   // Ensures that the menu start event is not fired redundantly.
   bool is_ax_menu_start_event_fired_ = false;

diff --git a/chrome/browser/ui/views/autofill/popup/popup_cell_view_unittest.cc b/chrome/browser/ui/views/autofill/popup/popup_cell_view_unittest.cc
@@ -51,6 +51,7 @@ class PopupCellViewTest : public ChromeViewsTestBase {
 
   void TearDown() override {
     view_ = nullptr;
+    generator_.reset();
     widget_.reset();
     ChromeViewsTestBase::TearDown();
   }

diff --git a/chrome/browser/ui/views/autofill/popup/popup_row_view.cc b/chrome/browser/ui/views/autofill/popup/popup_row_view.cc
@@ -12,9 +12,9 @@
 #include "base/memory/raw_ptr.h"
 #include "chrome/browser/ui/autofill/autofill_popup_controller.h"
 #include "chrome/browser/ui/browser_element_identifiers.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/passwords/ui_utils.h"
-#include "chrome/browser/ui/views/autofill/popup/popup_base_view.h"
 #include "chrome/browser/ui/views/autofill/popup/popup_cell_view.h"
 #include "chrome/browser/ui/views/autofill/popup/popup_row_strategy.h"
 #include "chrome/browser/ui/views/autofill/popup/popup_view_utils.h"
@@ -63,21 +63,29 @@ std::unique_ptr<PopupRowView> PopupRowView::Create(PopupViewViews& popup_view,
       break;
   }
 
-  return std::make_unique<PopupRowView>(popup_view, std::move(strategy));
+  return std::make_unique<PopupRowView>(
+      /*a11y_selection_delegate=*/popup_view, /*selection_delegate=*/popup_view,
+      controller, std::move(strategy));
 }
 
-PopupRowView::PopupRowView(PopupViewViews& popup_view,
-                           std::unique_ptr<PopupRowStrategy> strategy)
-    : popup_view_(popup_view), strategy_(std::move(strategy)) {
+PopupRowView::PopupRowView(
+    AccessibilitySelectionDelegate& a11y_selection_delegate,
+    SelectionDelegate& selection_delegate,
+    base::WeakPtr<AutofillPopupController> controller,
+    std::unique_ptr<PopupRowStrategy> strategy)
+    : a11y_selection_delegate_(a11y_selection_delegate),
+      controller_(controller),
+      strategy_(std::move(strategy)) {
   DCHECK(strategy_);
   // TODO(crbug.com/1411172): Use a BoxLayout once controls are supported.
   SetUseDefaultFillLayout(true);
   content_view_ = AddChildView(strategy_->CreateContent());
-  content_view_->SetOnExitedCallback(
-      base::BindRepeating(&PopupViewViews::SetSelectedCell,
-                          base::Unretained(&popup_view), absl::nullopt));
+  content_view_->SetOnExitedCallback(base::BindRepeating(
+      &SelectionDelegate::SetSelectedCell,
+      base::Unretained(&selection_delegate), absl::nullopt));
   content_view_->SetOnEnteredCallback(base::BindRepeating(
-      &PopupViewViews::SetSelectedCell, base::Unretained(&popup_view),
+      &SelectionDelegate::SetSelectedCell,
+      base::Unretained(&selection_delegate),
       PopupViewViews::CellIndex{strategy_->GetLineNumber(),
                                 PopupRowView::CellType::kContent}));
 }
@@ -109,28 +117,32 @@ void PopupRowView::SetSelectedCell(absl::optional<CellType> cell) {
 
   if (PopupCellView* new_view = view_from_type(selected_cell_)) {
     new_view->SetSelected(true);
-    GetPopupView().NotifyAXSelection(*new_view);
+    GetA11ySelectionDelegate().NotifyAXSelection(*new_view);
   } else {
     // Set the selected cell to none in case an invalid choice was made (e.g.
     // selecting a control cell when none exists).
     selected_cell_ = absl::nullopt;
   }
 }
 
+// TODO(crbug.com/1411172): Move to `PopupViewViews` class.
 void PopupRowView::MaybeShowIphPromo() {
-  std::string feature_name = GetPopupView()
-                                 .controller()
-                                 ->GetSuggestionAt(strategy_->GetLineNumber())
-                                 .feature_for_iph;
+  if (!controller_) {
+    return;
+  }
+  std::string feature_name =
+      controller_->GetSuggestionAt(strategy_->GetLineNumber()).feature_for_iph;
   if (feature_name.empty()) {
     return;
   }
 
   if (feature_name == "IPH_AutofillVirtualCardSuggestion") {
     SetProperty(views::kElementIdentifierKey,
                 kAutofillCreditCardSuggestionEntryElementId);
-    Browser* browser = GetPopupView().browser();
-    DCHECK(browser);
+    Browser* browser = chrome::FindLastActive();
+    if (!browser) {
+      return;
+    }
     browser->window()->MaybeShowFeaturePromo(
         feature_engagement::kIPHAutofillVirtualCardSuggestionFeature);
   }

diff --git a/chrome/browser/ui/views/autofill/popup/popup_row_view.h b/chrome/browser/ui/views/autofill/popup/popup_row_view.h
@@ -9,13 +9,16 @@
 
 #include "base/memory/raw_ptr.h"
 #include "base/memory/raw_ref.h"
-#include "chrome/browser/ui/views/autofill/popup/popup_cell_view.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/ui/autofill/autofill_popup_controller.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "ui/base/metadata/metadata_header_macros.h"
 #include "ui/views/view.h"
 
 namespace autofill {
 
+class AutofillPopupController;
+class PopupCellView;
 class PopupRowStrategy;
 class PopupViewViews;
 
@@ -33,10 +36,34 @@ class PopupRowView : public views::View {
     kControl = 1
   };
 
+  // Interface used to announce changes in selected cells to accessibility
+  // frameworks.
+  class AccessibilitySelectionDelegate {
+   public:
+    virtual ~AccessibilitySelectionDelegate() = default;
+
+    // Notify accessibility that an item represented by `view` has been
+    // selected.
+    virtual void NotifyAXSelection(views::View& view) = 0;
+  };
+
+  // Interface used to keep track of cell selection. This may be needed if the
+  // parent needs to keep state of which row is currently in order to process
+  // keyboard events.
+  class SelectionDelegate {
+   public:
+    using CellIndex = std::pair<size_t, PopupRowView::CellType>;
+
+    virtual ~SelectionDelegate() = default;
+
+    virtual absl::optional<CellIndex> GetSelectedCell() const = 0;
+    virtual void SetSelectedCell(absl::optional<CellIndex> cell_index) = 0;
+  };
+
   METADATA_HEADER(PopupRowView);
-  // TODO(crbug.com/1411172): Consider passing an interface instead of an
-  // implementation to allow for separate testing of this class.
-  PopupRowView(PopupViewViews& popup_view,
+  PopupRowView(AccessibilitySelectionDelegate& a11y_selection_delegate,
+               SelectionDelegate& selection_delegate,
+               base::WeakPtr<AutofillPopupController> controller,
                std::unique_ptr<PopupRowStrategy> strategy);
   PopupRowView(const PopupRowView&) = delete;
   PopupRowView& operator=(const PopupRowView&) = delete;
@@ -60,10 +87,16 @@ class PopupRowView : public views::View {
   PopupCellView& GetContentView() { return *content_view_; }
 
  private:
-  PopupViewViews& GetPopupView() { return popup_view_.get(); }
-
-  // The parent view containing this row.
-  const raw_ref<PopupViewViews> popup_view_;
+  AccessibilitySelectionDelegate& GetA11ySelectionDelegate() {
+    return a11y_selection_delegate_.get();
+  }
+
+  // The delegate used for accessibility announcements (implemented by the
+  // parent).
+  const raw_ref<AccessibilitySelectionDelegate> a11y_selection_delegate_;
+  // The controller for the parent view.
+  const base::WeakPtr<AutofillPopupController> controller_;
+  // The strategy from which the actual view content of this row is created.
   const std::unique_ptr<PopupRowStrategy> strategy_;
 
   // Which (if any) cell of this row is currently selected.