Accessibility for Autofill Popup View in native code

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

Index: chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
index 89925cee23efa839e82e4105a6ff2f6b6bc68806..d5eae2471a0c783bddda2e80d6ef5afe2e04d760 100644
--- a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
+++ b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
@@ -6,8 +6,11 @@
 
 #include "chrome/browser/ui/autofill/autofill_popup_controller.h"
 #include "chrome/browser/ui/autofill/autofill_popup_layout_model.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/browser/suggestion.h"
+#include "ui/accessibility/ax_node_data.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/point.h"
@@ -21,22 +24,28 @@
 namespace autofill {
 
 AutofillPopupViewViews::AutofillPopupViewViews(
-    AutofillPopupController* controller,
-    views::Widget* parent_widget)
+    AutofillPopupController* controller, views::Widget* parent_widget)
     : AutofillPopupBaseView(controller, parent_widget),
-      controller_(controller) {}
+      controller_(controller) {
+  for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
+    AutofillPopupChildView *child = new AutofillPopupChildView(controller, i);

[Mathieu, 2017/02/27 18:41:55]

AutofillPopupChildView* child

[csashi, 2017/02/27 18:49:44]

Is this an unfinished comment?

[Mathieu, 2017/02/27 19:01:05]

No the * is at the wrong location, sorry that wasn't clear

[csashi, 2017/02/27 19:09:38]

Done.

+    AddChildViewAt(child, static_cast<int>(i));
+  }
+  SetFocusBehavior(FocusBehavior::ALWAYS);
+}
 
 AutofillPopupViewViews::~AutofillPopupViewViews() {}
 
 void AutofillPopupViewViews::Show() {
   DoShow();
+  NotifyAccessibilityEvent(ui::AX_EVENT_MENU_START, true);
 }
 
 void AutofillPopupViewViews::Hide() {
   // The controller is no longer valid after it hides us.
   controller_ = NULL;
-
   DoHide();
+  NotifyAccessibilityEvent(ui::AX_EVENT_MENU_END, true);
 }
 
 void AutofillPopupViewViews::UpdateBoundsAndRedrawPopup() {
@@ -70,6 +79,13 @@ void AutofillPopupViewViews::InvalidateRow(size_t row) {
   SchedulePaintInRect(controller_->layout_model().GetRowBounds(row));
 }
 
+void AutofillPopupViewViews::NotifyAccessibilityEventForRow(
+    ui::AXEvent event_type, size_t row) {
+  AutofillPopupChildView *child_view =
+      static_cast<AutofillPopupChildView*>(child_at(row));
+  child_view->NotifyAccessibilityEvent(event_type, true);
+}
+
 /**
 * Autofill entries in ltr.
 *
@@ -189,6 +205,12 @@ void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas,
   }
 }
 
+void AutofillPopupViewViews::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  node_data->role = ui::AX_ROLE_MENU;
+  node_data->SetName(l10n_util::GetStringUTF16(
+      IDS_AUTOFILL_POPUP_ACCESSIBLE_NODE_DATA));
+}
+
 AutofillPopupView* AutofillPopupView::Create(
     AutofillPopupController* controller) {
   views::Widget* observing_widget =
@@ -203,4 +225,21 @@ AutofillPopupView* AutofillPopupView::Create(
   return new AutofillPopupViewViews(controller, observing_widget);
 }
 
+AutofillPopupChildView::AutofillPopupChildView(
+    AutofillPopupController* controller, size_t index)
+    : controller_(controller), index_(index) {
+  SetFocusBehavior(FocusBehavior::ALWAYS);
+}
+
+AutofillPopupChildView::~AutofillPopupChildView() {}
+
+void AutofillPopupChildView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  if (!controller_)

[Mathieu, 2017/02/27 18:41:55]

since AutofillPopupViewViews doesn't seem to guard against a null controller,
let's remove this check and add a comment to AutofillPopupViewViews's
construction declaration to say that |controller| should never be null.

[csashi, 2017/02/27 18:49:44]

The controller_ is set to NULL on Hide. Are we sure GetAccessibleNodeData will
not be called after Hide? If yes, I can remove the check.

[Mathieu, 2017/02/27 19:01:05]

Hmm I don't see where the controller becomes null, can you be more specific? And
if it is so, in that case your child view will keep the potentially invalid
pointer even though its content is potentially destroyed elsewhere, right?

[csashi, 2017/02/27 19:09:38]

Done. My mistake, I was referring to AutofillPopupViewViews.

+    return;
+  // We do not special case |POPUP_ITEM_ID_SEPARATOR| suggestion because we
+  // skip that suggestion when the user navigates through the popup suggestions.
+  node_data->role = ui::AX_ROLE_MENU_ITEM;
+  node_data->SetName(controller_->GetElidedValueAt(index_));
+}
+
 }  // namespace autofill