Stablizing Mac autofill accessibility when popup show/hides

This is a follow up change of CL:1667787, which introduced
AutofillPopupControllerImpl::FireControlsChangedEvent for autofill
popup accessibility.

On Mac and potentially other platforms, when accessibility is
enabled, upon invoking autofill popup and FireControlsChangedEvent
we end up dereferencing a nullptr of AxPlatformNode due to
Mac does not have a complete implementation of AxPlatformNode yet.

This CL fixes the above by adding a check for AxPlatformNode in
FireControlsChangedEvent.

Changes:
 1. Introduced a check for invalid AxPlatformNode in
    AutofillPopupControllerImpl::FireControlsChangedEvent to return
    early from firing event.
 2. Added associated unit tests for FireControlsChangedEvent.
 3. Exposed GetWebContentsPopupControllerAxId virtual to
    AutofillPopupDelegate..

Bug: 986587
Change-Id: I9a6ed49330a9ea9d7d4e5483c3ee06d5675919ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1714297
Commit-Queue: Victor Fei <vicfei@microsoft.com>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Ian Prest <iapres@microsoft.com>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686461}

chrome/browser/ui/autofill/autofill_popup_controller_impl.cc

@@ -597,24 +597,31 @@ void AutofillPopupControllerImpl::FireControlsChangedEvent(bool is_show) {
 
   // Retrieve the ax node id associated with the current web contents' element
   // that has a controller relation to the current autofill popup.
-  int32_t node_id = static_cast<AutofillExternalDelegate*>(delegate_.get())
-                        ->GetWebContentsPopupControllerAxId();
+  int32_t node_id = delegate_->GetWebContentsPopupControllerAxId();
 
   // We can only raise controls changed accessibility event when we have a valid
   // ax tree and an ax node associated with the ax tree for the popup
   // controller, and a valid ax unique id for the popup controllee.
-  if (!ax_tree_manager || !ax_tree_manager->GetDelegate(tree_id, node_id) ||
-      !view_->GetAxUniqueId())
+  if (!ax_tree_manager)
+    return;
+  ui::AXPlatformNodeDelegate* ax_platform_node_delegate =
+      ax_tree_manager->GetDelegate(tree_id, node_id);
+  if (!ax_platform_node_delegate)
+    return;
+  ui::AXPlatformNode* target_node =
+      ax_platform_node_delegate->GetFromNodeID(node_id);
+  base::Optional<int32_t> popup_ax_id = view_->GetAxUniqueId();
+  if (!target_node || !popup_ax_id)
     return;
 
+  // All the conditions are valid, raise the accessibility event and set global
+  // popup ax unique id.
   if (is_show)
-    ui::SetActivePopupAxUniqueId(view_->GetAxUniqueId());
+    ui::SetActivePopupAxUniqueId(popup_ax_id);
   else
     ui::ClearActivePopupAxUniqueId();
 
-  ax_tree_manager->GetDelegate(tree_id, node_id)
-      ->GetFromNodeID(node_id)
-      ->NotifyAccessibilityEvent(ax::mojom::Event::kControlsChanged);
+  target_node->NotifyAccessibilityEvent(ax::mojom::Event::kControlsChanged);
 }
 
 }  // namespace autofill

chrome/browser/ui/autofill/autofill_popup_controller_impl.h

@@ -139,6 +139,11 @@ class AutofillPopupControllerImpl : public AutofillPopupController {
 
   AutofillPopupLayoutModel& LayoutModelForTesting() { return layout_model_; }
 
+  // Raise an accessibility event to indicate the controls relation of the
+  // form control of the popup and popup itself has changed based on the popup's
+  // show or hide action.
+  void FireControlsChangedEvent(bool is_show);
+
  private:
 #if !defined(OS_ANDROID)
   FRIEND_TEST_ALL_PREFIXES(AutofillPopupControllerUnitTest, ElideText);
@@ -155,12 +160,8 @@ class AutofillPopupControllerImpl : public AutofillPopupController {
   // Hides |view_| unless it is null and then deletes |this|.
   void HideViewAndDie();
 
-  // Raise an accessibility event to indicate the controls relation of the
-  // form control of the popup and popup itself has changed based on the popup's
-  // show or hide action.
-  void FireControlsChangedEvent(bool is_show);
-
   friend class AutofillPopupControllerUnitTest;
+  friend class AutofillPopupControllerAccessibilityUnitTest;
   void SetViewForTesting(AutofillPopupView* view) { view_ = view; }
 
   PopupControllerCommon controller_common_;

components/autofill/core/browser/autofill_external_delegate.cc

@@ -286,6 +286,10 @@ AutofillDriver* AutofillExternalDelegate::GetAutofillDriver() {
   return driver_;
 }
 
+int32_t AutofillExternalDelegate::GetWebContentsPopupControllerAxId() const {
+  return query_field_.form_control_ax_id;
+}
+
 void AutofillExternalDelegate::RegisterDeletionCallback(
     base::OnceClosure deletion_callback) {
   deletion_callback_ = std::move(deletion_callback);
@@ -295,10 +299,6 @@ void AutofillExternalDelegate::Reset() {
   manager_->client()->HideAutofillPopup();
 }
 
-int32_t AutofillExternalDelegate::GetWebContentsPopupControllerAxId() const {
-  return query_field_.form_control_ax_id;
-}
-
 base::WeakPtr<AutofillExternalDelegate> AutofillExternalDelegate::GetWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }

components/autofill/core/browser/autofill_external_delegate.h

@@ -57,6 +57,10 @@ class AutofillExternalDelegate : public AutofillPopupDelegate {
   // popup type after call to |onQuery|.
   PopupType GetPopupType() const override;
   AutofillDriver* GetAutofillDriver() override;
+
+  // Returns the ax node id associated with the current web contents' element
+  // who has a controller relation to the current autofill popup.
+  int32_t GetWebContentsPopupControllerAxId() const override;
   void RegisterDeletionCallback(base::OnceClosure deletion_callback) override;
 
   // Records and associates a query_id with web form data.  Called
@@ -96,10 +100,6 @@ class AutofillExternalDelegate : public AutofillPopupDelegate {
   // values or settings.
   void Reset();
 
-  // Returns the ax node id associated with the current web contents' element
-  // who has a controller relation to the current autofill popup.
-  int32_t GetWebContentsPopupControllerAxId() const;
-
   const FormData& query_form() const { return query_form_; }
 
  protected:

components/autofill/core/browser/ui/autofill_popup_delegate.h

@@ -56,6 +56,10 @@ class AutofillPopupDelegate {
   // Returns the associated AutofillDriver.
   virtual AutofillDriver* GetAutofillDriver() = 0;
 
+  // Returns the ax node id associated with the current web contents' element
+  // who has a controller relation to the current autofill popup.
+  virtual int32_t GetWebContentsPopupControllerAxId() const = 0;
+
   // Sets |deletion_callback| to be called from the delegate's destructor.
   // Useful for deleting objects which cannot be owned by the delegate but
   // should not outlive it.

components/password_manager/core/browser/password_autofill_manager.cc

@@ -260,6 +260,12 @@ autofill::AutofillDriver* PasswordAutofillManager::GetAutofillDriver() {
   return password_manager_driver_->GetAutofillDriver();
 }
 
+int32_t PasswordAutofillManager::GetWebContentsPopupControllerAxId() const {
+  // TODO: Needs to be implemented once we step up accessibility features later.
+  NOTIMPLEMENTED_LOG_ONCE() << "See http://crbug.com/991253";
+  return 0;
+}
+
 void PasswordAutofillManager::RegisterDeletionCallback(
     base::OnceClosure deletion_callback) {
   deletion_callback_ = std::move(deletion_callback);

components/password_manager/core/browser/password_autofill_manager.h

@@ -55,6 +55,7 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate {
   void ClearPreviewedForm() override;
   autofill::PopupType GetPopupType() const override;
   autofill::AutofillDriver* GetAutofillDriver() override;
+  int32_t GetWebContentsPopupControllerAxId() const override;
   void RegisterDeletionCallback(base::OnceClosure deletion_callback) override;
 
   // Invoked when a password mapping is added.