Fix Password Manager popup a11y.

Password generation popup a11y behaviour was made consistent
with autofill popup: the field tells AT that it has autofill
option (SetSuggestionAvailability) and the popup options can
be focused and read as regular atutofill suggestions.

What changed in images:
Now the user is notified that there are autofill options by focusing a password field (notice "...with autofill menu")
https://screenshot.googleplex.com/8LEuBmmLwJeJEwG

The behaviour (focusing and utterances) was made consistent with the autofill popup:
https://screenshot.googleplex.com/xHxSFqwnhngtonb
This is just more close to the interface: the user is notified about new menu, other than new window popped up on the screen.
+ the a11y message was fixed


Bug: 1313129
Change-Id: I8007c6dfc4d93da446a5fa87e3b64cbe5623c357
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4091625
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Commit-Queue: Dmitry Vykochko <vykochko@google.com>
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Joanmarie Diggs <jdiggs@igalia.com>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1097469}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -55,6 +55,7 @@
 #include "components/autofill/core/browser/logging/log_manager.h"
 #include "components/autofill/core/browser/logging/log_receiver.h"
 #include "components/autofill/core/browser/ui/suggestion.h"
+#include "components/autofill/core/common/mojom/autofill_types.mojom-shared.h"
 #include "components/autofill/core/common/password_generation_util.h"
 #include "components/back_forward_cache/back_forward_cache_disable.h"
 #include "components/browsing_data/content/browsing_data_helper.h"
@@ -1173,6 +1174,9 @@ void ChromePasswordManagerClient::AutomaticGenerationAvailable(
       popup_controller_->GeneratedPasswordRejected();
     }
 
+    driver->SetSuggestionAvailability(
+        ui_data.generation_element_id,
+        autofill::mojom::AutofillState::kAutofillAvailable);
     return;
   }
 
@@ -1682,6 +1686,12 @@ void ChromePasswordManagerClient::ShowPasswordGenerationPopup(
     popup_controller_->Show(
         PasswordGenerationPopupController::kOfferGeneration);
   }
+
+  driver->SetSuggestionAvailability(
+      ui_data.generation_element_id,
+      popup_controller_->IsVisible()
+          ? autofill::mojom::AutofillState::kAutofillAvailable
+          : autofill::mojom::AutofillState::kNoSuggestions);
 }
 
 gfx::RectF ChromePasswordManagerClient::TransformToRootCoordinates(

chrome/browser/resources/chromeos/accessibility/chromevox/background/desktop_automation_handler.js

@@ -737,7 +737,9 @@ export class DesktopAutomationHandler extends DesktopAutomationInterface {
           target.className === 'PasswordPopupSuggestionView' ||
           target.className === 'AutofillPopupFooterView' ||
           target.className === 'AutofillPopupWarningView' ||
-          target.className === 'AutofillPopupBaseView') {
+          target.className === 'AutofillPopupBaseView' ||
+          target.className ===
+              'PasswordGenerationPopupViewViews::GeneratedPasswordBox') {
         override = true;
       }
 

chrome/browser/ui/passwords/password_generation_popup_view_browsertest.cc

@@ -10,18 +10,52 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/password_manager/password_manager_uitest_util.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/passwords/password_generation_popup_controller_impl.h"
 #include "chrome/browser/ui/passwords/password_generation_popup_view_tester.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/password_manager/content/browser/content_password_manager_driver.h"
 #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
 #include "components/password_manager/core/common/password_manager_features.h"
+#include "content/public/browser/browser_accessibility_state.h"
 #include "content/public/test/browser_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/accessibility/ax_enums.mojom-shared.h"
+#include "ui/accessibility/platform/ax_platform_node.h"
+#include "ui/accessibility/platform/ax_platform_node_delegate.h"
+#include "ui/accessibility/platform/child_iterator_base.h"
+#include "ui/gfx/native_widget_types.h"
+#include "ui/views/widget/any_widget_observer.h"
+#include "ui/views/widget/widget.h"
 
 namespace autofill {
 
+namespace {
+
+ui::AXPlatformNodeDelegate* FindNode(ui::AXPlatformNodeDelegate* root,
+                                     const std::string& class_name) {
+  if (!root) {
+    return nullptr;
+  }
+
+  if (root->GetStringAttribute(ax::mojom::StringAttribute::kClassName) ==
+      class_name) {
+    return root;
+  }
+
+  for (auto it = root->ChildrenBegin(); *it != *root->ChildrenEnd(); ++(*it)) {
+    ui::AXPlatformNodeDelegate* child_found = FindNode(it->get(), class_name);
+    if (child_found) {
+      return child_found;
+    }
+  }
+
+  return nullptr;
+}
+
+}  // namespace
+
 class PasswordGenerationPopupViewTest : public InProcessBrowserTest {};
 
 class PasswordGenerationPopupViewWithStrengthIndicatorTest
@@ -352,4 +386,75 @@ IN_PROC_BROWSER_TEST_F(
   web_contents->Close();
 }
 
+using PasswordGenerationPopupViewAxTest = InProcessBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(PasswordGenerationPopupViewAxTest, PopupInAxTree) {
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_LINUX)
+  content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  auto container_bounds = web_contents->GetContainerBounds();
+  password_generation::PasswordGenerationUIData ui_data(
+      gfx::RectF(container_bounds.x(), container_bounds.y(), 10, 10),
+      /*max_length=*/10,
+      /*generation_element=*/std::u16string(),
+      /*user_typed_password=*/std::u16string(), FieldRendererId(100),
+      /*is_generation_element_password_type=*/true, base::i18n::TextDirection(),
+      FormData());
+
+  TestGenerationPopupObserver observer;
+  base::WeakPtr<PasswordGenerationPopupControllerImpl> controller =
+      PasswordGenerationPopupControllerImpl::GetOrCreate(
+          /*previous=*/nullptr, ui_data.bounds, ui_data,
+          password_manager::ContentPasswordManagerDriverFactory::
+              FromWebContents(web_contents)
+                  ->GetDriverForFrame(web_contents->GetPrimaryMainFrame())
+                  ->AsWeakPtr(),
+          &observer, web_contents, web_contents->GetPrimaryMainFrame());
+
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey{},
+                                       "PasswordGenerationPopupViewViews");
+  controller->Show(GenerationUIState::kOfferGeneration);
+
+  gfx::NativeWindow window = gfx::kNullNativeWindow;
+#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX)
+  // On Mac and Linux the whole ax tree grows from the main root windows
+  // and the popup node can be found there. This gives more confidence
+  // that it is in the right place than on Windows (see below) where
+  // the popup subtree lives separately.
+  waiter.WaitIfNeededAndGet();
+  window = chrome::FindLastActive()->window()->GetNativeWindow();
+#elif BUILDFLAG(IS_WIN)
+  views::Widget* dialog_widget = waiter.WaitIfNeededAndGet();
+  window = dialog_widget->GetNativeWindow();
+#endif
+
+  ui::AXPlatformNode* root_node = ui::AXPlatformNode::FromNativeWindow(window);
+  ui::AXPlatformNodeDelegate* root_node_delegate = root_node->GetDelegate();
+  ui::AXPlatformNodeDelegate* node_delegate =
+      FindNode(root_node_delegate,
+               "PasswordGenerationPopupViewViews::GeneratedPasswordBox");
+
+  ASSERT_THAT(node_delegate, ::testing::NotNull());
+  EXPECT_FALSE(
+      node_delegate->GetBoolAttribute(ax::mojom::BoolAttribute::kSelected));
+
+  // Set the screen reader focus by calling a method on the controller directly,
+  // it normally is triggered by UI events when the screen reader is on,
+  // screen reader presence is hard/expensive to emulate.
+  static_cast<PasswordGenerationPopupController*>(controller.get())
+      ->SetSelected();
+
+  EXPECT_TRUE(
+      node_delegate->GetBoolAttribute(ax::mojom::BoolAttribute::kSelected));
+
+  web_contents->Close();
+  content::BrowserAccessibilityState::GetInstance()->EnableAccessibility();
+#else
+  GTEST_SKIP() << "Accessibility reflection is not supported on this platform.";
+#endif
+}
+
 }  // namespace autofill

chrome/browser/ui/views/autofill/autofill_popup_base_view.cc

@@ -244,7 +244,8 @@ void AutofillPopupBaseView::NotifyAXSelection(View* selected_view) {
   constexpr auto kDerivedClasses = base::MakeFixedFlatSet<base::StringPiece>(
       {"AutofillPopupSuggestionView", "PasswordPopupSuggestionView",
        "AutofillPopupFooterView", "AutofillPopupSeparatorView",
-       "AutofillPopupWarningView", "AutofillPopupBaseView"});
+       "AutofillPopupWarningView", "AutofillPopupBaseView",
+       "PasswordGenerationPopupViewViews::GeneratedPasswordBox"});
   DCHECK(kDerivedClasses.contains(selected_view->GetClassName()))
       << "If you add a new derived class from AutofillPopupRowView, add it "
          "here and to onSelection(evt) in "

chrome/browser/ui/views/passwords/password_generation_popup_view_views.cc

@@ -29,6 +29,7 @@
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/native_theme/native_theme.h"
+#include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/image_view.h"
@@ -108,6 +109,25 @@ class PasswordGenerationPopupViewViews::GeneratedPasswordBox
   GeneratedPasswordBox() = default;
   ~GeneratedPasswordBox() override = default;
 
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    if (!controller_) {
+      return;
+    }
+
+    node_data->role = ax::mojom::Role::kListBoxOption;
+    node_data->AddBoolAttribute(ax::mojom::BoolAttribute::kSelected,
+                                controller_->password_selected());
+    node_data->SetNameChecked(base::JoinString(
+        {controller_->SuggestedText(), controller_->password()}, u" "));
+    const std::u16string help_text = l10n_util::GetStringFUTF16(
+        IDS_PASSWORD_GENERATION_PROMPT_GOOGLE_PASSWORD_MANAGER,
+        l10n_util::GetStringUTF16(
+            IDS_PASSWORD_BUBBLES_PASSWORD_MANAGER_LINK_TEXT_SYNCED_TO_ACCOUNT),
+        controller_->GetPrimaryAccountEmail());
+
+    node_data->SetDescription(help_text);
+  }
+
   // Fills the view with strings provided by |controller|.
   void Init(base::WeakPtr<PasswordGenerationPopupController> controller);
 
@@ -270,7 +290,7 @@ void PasswordGenerationPopupViewViews::PasswordSelectionUpdated() {
   DCHECK(FullPopupVisible());
 
   if (controller_->password_selected())
-    NotifyAXSelection(this);
+    NotifyAXSelection(this->password_view_);
 
   if (!GetWidget())
     return;
@@ -378,13 +398,18 @@ void PasswordGenerationPopupViewViews::OnPaint(gfx::Canvas* canvas) {
 
 void PasswordGenerationPopupViewViews::GetAccessibleNodeData(
     ui::AXNodeData* node_data) {
+  // TODO(crbug.com/1404297): kListBox is used for the same reason as in
+  // AutofillPopupViewNativeViews. See crrev.com/c/2545285 for details.
+  // Consider using a more appropriate role (e.g. kMenuListPopup or similar).
+  node_data->role = ax::mojom::Role::kListBox;
+
   if (!controller_) {
+    node_data->AddState(ax::mojom::State::kCollapsed);
+    node_data->AddState(ax::mojom::State::kInvisible);
     return;
   }
-  node_data->role = ax::mojom::Role::kMenuItem;
-  node_data->SetNameChecked(base::JoinString(
-      {controller_->SuggestedText(), controller_->password()}, u" "));
-  node_data->SetDescription(controller_->HelpText());
+
+  node_data->AddState(ax::mojom::State::kExpanded);
 }
 
 gfx::Size PasswordGenerationPopupViewViews::CalculatePreferredSize() const {
@@ -410,3 +435,7 @@ PasswordGenerationPopupView* PasswordGenerationPopupView::Create(
 
   return new PasswordGenerationPopupViewViews(controller, observing_widget);
 }
+
+BEGIN_METADATA(PasswordGenerationPopupViewViews,
+               autofill::AutofillPopupBaseView)
+END_METADATA

chrome/browser/ui/views/passwords/password_generation_popup_view_views.h

@@ -9,7 +9,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/ui/passwords/password_generation_popup_view.h"
 #include "chrome/browser/ui/views/autofill/autofill_popup_base_view.h"
-
+#include "ui/base/metadata/metadata_header_macros.h"
 class PasswordGenerationPopupController;
 
 namespace views {
@@ -19,6 +19,8 @@ class StyledLabel;
 class PasswordGenerationPopupViewViews : public autofill::AutofillPopupBaseView,
                                          public PasswordGenerationPopupView {
  public:
+  METADATA_HEADER(PasswordGenerationPopupViewViews);
+
   PasswordGenerationPopupViewViews(
       base::WeakPtr<PasswordGenerationPopupController> controller,
       views::Widget* parent_widget);

components/password_manager/content/browser/content_password_manager_driver.cc

@@ -11,6 +11,7 @@
 #include "components/autofill/content/browser/content_autofill_driver.h"
 #include "components/autofill/core/browser/logging/log_manager.h"
 #include "components/autofill/core/common/form_data.h"
+#include "components/autofill/core/common/unique_ids.h"
 #include "components/password_manager/content/browser/bad_message.h"
 #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
 #include "components/password_manager/content/browser/form_meta_data.h"
@@ -209,6 +210,12 @@ void ContentPasswordManagerDriver::ClearPreviewedForm() {
   GetAutofillAgent()->ClearPreviewedForm();
 }
 
+void ContentPasswordManagerDriver::SetSuggestionAvailability(
+    autofill::FieldRendererId generation_element_id,
+    const autofill::mojom::AutofillState state) {
+  GetAutofillAgent()->SetSuggestionAvailability(generation_element_id, state);
+}
+
 PasswordGenerationFrameHelper*
 ContentPasswordManagerDriver::GetPasswordGenerationHelper() {
   return &password_generation_helper_;

components/password_manager/content/browser/content_password_manager_driver.h

@@ -81,6 +81,9 @@ class ContentPasswordManagerDriver
                          const std::u16string& password) override;
   void PreviewGenerationSuggestion(const std::u16string& password) override;
   void ClearPreviewedForm() override;
+  void SetSuggestionAvailability(
+      autofill::FieldRendererId generation_element_id,
+      const autofill::mojom::AutofillState state) override;
   PasswordGenerationFrameHelper* GetPasswordGenerationHelper() override;
   PasswordManager* GetPasswordManager() override;
   PasswordAutofillManager* GetPasswordAutofillManager() override;

components/password_manager/core/browser/password_manager_driver.h

@@ -11,6 +11,7 @@
 #include "base/functional/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "base/types/strong_alias.h"
+#include "components/autofill/core/common/mojom/autofill_types.mojom-shared.h"
 #include "components/autofill/core/common/unique_ids.h"
 #include "ui/accessibility/ax_tree_id.h"
 
@@ -110,6 +111,13 @@ class PasswordManagerDriver
   // Tells the driver to clear previewed password and username fields.
   virtual void ClearPreviewedForm() = 0;
 
+  // Updates the autofill availability state of the DOM node with
+  // |generation_element_id|. It is critical for a11y to keep it updated
+  // to make proper announcements.
+  virtual void SetSuggestionAvailability(
+      autofill::FieldRendererId generation_element_id,
+      const autofill::mojom::AutofillState state) = 0;
+
   // Returns the PasswordGenerationFrameHelper associated with this instance.
   virtual PasswordGenerationFrameHelper* GetPasswordGenerationHelper() = 0;
 

components/password_manager/core/browser/stub_password_manager_driver.cc

@@ -39,6 +39,10 @@ void StubPasswordManagerDriver::PreviewGenerationSuggestion(
 void StubPasswordManagerDriver::ClearPreviewedForm() {
 }
 
+void StubPasswordManagerDriver::SetSuggestionAvailability(
+    autofill::FieldRendererId generation_element_id,
+    const autofill::mojom::AutofillState state) {}
+
 PasswordGenerationFrameHelper*
 StubPasswordManagerDriver::GetPasswordGenerationHelper() {
   return nullptr;

components/password_manager/core/browser/stub_password_manager_driver.h

@@ -36,6 +36,9 @@ class StubPasswordManagerDriver : public PasswordManagerDriver {
                          const std::u16string& password) override;
   void PreviewGenerationSuggestion(const std::u16string& password) override;
   void ClearPreviewedForm() override;
+  void SetSuggestionAvailability(
+      autofill::FieldRendererId generation_element_id,
+      const autofill::mojom::AutofillState state) override;
   PasswordGenerationFrameHelper* GetPasswordGenerationHelper() override;
   PasswordManagerInterface* GetPasswordManager() override;
   PasswordAutofillManager* GetPasswordAutofillManager() override;

components/password_manager/ios/ios_password_manager_driver.h

@@ -47,6 +47,9 @@ class IOSPasswordManagerDriver
                          const std::u16string& password) override;
   void PreviewGenerationSuggestion(const std::u16string& password) override;
   void ClearPreviewedForm() override;
+  void SetSuggestionAvailability(
+      autofill::FieldRendererId generation_element_id,
+      const autofill::mojom::AutofillState state) override;
   password_manager::PasswordGenerationFrameHelper* GetPasswordGenerationHelper()
       override;
   password_manager::PasswordManagerInterface* GetPasswordManager() override;

components/password_manager/ios/ios_password_manager_driver.mm

@@ -94,6 +94,12 @@
   NOTIMPLEMENTED();
 }
 
+void IOSPasswordManagerDriver::SetSuggestionAvailability(
+    autofill::FieldRendererId generation_element_id,
+    const autofill::mojom::AutofillState state) {
+  NOTIMPLEMENTED();
+}
+
 password_manager::PasswordGenerationFrameHelper*
 IOSPasswordManagerDriver::GetPasswordGenerationHelper() {
   return password_generation_helper_.get();

content/browser/accessibility/browser_accessibility.h

@@ -129,7 +129,7 @@ class CONTENT_EXPORT BrowserAccessibility : public ui::AXPlatformNodeDelegate {
     PlatformChildIterator& operator--() override;
     PlatformChildIterator operator--(int);
     gfx::NativeViewAccessible GetNativeViewAccessible() const override;
-    BrowserAccessibility* get() const;
+    BrowserAccessibility* get() const override;
     absl::optional<size_t> GetIndexInParent() const override;
     BrowserAccessibility& operator*() const override;
     BrowserAccessibility* operator->() const override;

ui/accessibility/platform/ax_platform_node_delegate.h

@@ -77,7 +77,7 @@ class COMPONENT_EXPORT(AX_PLATFORM) AXPlatformNodeDelegate {
   using AXRange = ui::AXRange<AXPosition::element_type>;
 
   AXPlatformNodeDelegate();
-  
+
   AXPlatformNodeDelegate(const AXPlatformNodeDelegate&) = delete;
   AXPlatformNodeDelegate& operator=(const AXPlatformNodeDelegate&) = delete;