Uses child views in Autofill Popup so we can trigger

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..003986b83fcc48b71614947776c313489a84894a 100644
--- a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
+++ b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
@@ -4,10 +4,14 @@
 
 #include "chrome/browser/ui/views/autofill/autofill_popup_view_views.h"
 
+#include "base/optional.h"
 #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"
@@ -16,30 +20,78 @@
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/views/border.h"
+#include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
 namespace autofill {
 
+namespace {
+
+// Child view only for triggering accessibility events. Rendering is handled
+// by |AutofillPopupViewViews|.
+class AutofillPopupChildView : public views::View {
+ public:
+  // Internal class name.
+  static const char kViewClassName[];
+
+  // |controller| should not be NULL.

[Evan Stade, 2017/03/16 17:39:00]

in this case you can probably make controller_ (and the ctor param) a const ref.

On the other hand, for now (and probably in the future) you can get by with just
passing a reference to an autofill::Suggestion.

[csashi, 2017/03/16 22:42:06]

Done.

+  AutofillPopupChildView(AutofillPopupController* controller, size_t index)
+      : controller_(controller), index_(index) {
+    SetFocusBehavior(FocusBehavior::ALWAYS);
+  }
+
+ private:
+  ~AutofillPopupChildView() override {}
+
+  // views::Views implementation
+  const char* GetClassName() const override { return kViewClassName; }
+
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
+    node_data->role = ui::AX_ROLE_MENU_ITEM;
+    node_data->SetName(controller_->GetSuggestionAt(index_).value);
+  }
+
+  AutofillPopupController* controller_;  // Weak reference.
+  const size_t index_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutofillPopupChildView);
+};
+
+// static
+const char AutofillPopupChildView::kViewClassName[] = "AutofillPopupChildView";
+
+}  // namespace
+
 AutofillPopupViewViews::AutofillPopupViewViews(
     AutofillPopupController* controller,
     views::Widget* parent_widget)
     : AutofillPopupBaseView(controller, parent_widget),
-      controller_(controller) {}
+      controller_(controller) {
+  CreateChildViews();
+  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() {
+void AutofillPopupViewViews::OnSuggestionsChanged() {
+  // We recreate the child views so we can be sure the |controller_|'s
+  // |GetLineCount()| will match the number of child views. Otherwise,
+  // the number of suggestions i.e. |GetLineCount()| may not match 1x1 with the
+  // child views. See crbug.com/697466.
+  RemoveAllChildViews(true /* delete_children */);

[Evan Stade, 2017/03/16 17:39:00]

nit: seems like this should be inside CreateChildViews

[csashi, 2017/03/16 22:42:06]

Done.

+  CreateChildViews();
   DoUpdateBoundsAndRedrawPopup();
 }
 
@@ -51,6 +103,7 @@ void AutofillPopupViewViews::OnPaint(gfx::Canvas* canvas) {
       ui::NativeTheme::kColorId_ResultsTableNormalBackground));
   OnPaintBorder(canvas);
 
+  DCHECK_EQ(controller_->GetLineCount(), static_cast<size_t>(child_count()));
   for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
     gfx::Rect line_rect = controller_->layout_model().GetRowBounds(i);
 
@@ -66,8 +119,26 @@ void AutofillPopupViewViews::OnPaint(gfx::Canvas* canvas) {
   }
 }
 
-void AutofillPopupViewViews::InvalidateRow(size_t row) {
-  SchedulePaintInRect(controller_->layout_model().GetRowBounds(row));
+void AutofillPopupViewViews::OnSelectedRowChanged(
+    base::Optional<size_t> previous_row_selection,
+    base::Optional<size_t> current_row_selection) {
+  if (previous_row_selection &&
+      previous_row_selection < controller_->GetLineCount()) {

[Evan Stade, 2017/03/16 17:39:00]

why is this second check required?

[csashi, 2017/03/16 22:42:06]

Because the previous_row_selection could be for a row that has been deleted? I
ported this check from the check that was present in the controller
(https://codesearch.chromium.org/chromium/src/chrome/browser/ui/autofill/autof...)

[Evan Stade, 2017/03/17 14:11:55]

but that check looks like it turned into a dcheck in the controller

[csashi, 2017/03/17 16:30:35]

The dcheck in the controller is for the newly selected line (which needs to be <
|GetLineCount()|. IIUC, the previous selection can be >= |GetLineCount()| and we
don't have a dcheck for that.

[Evan Stade, 2017/03/17 19:08:54]

fair enough, but I think somewhere in Show() you should be resetting the line
selection to none as necessary, and it should be the controller's job to make
sure previous_row_selection is a valid row.

[csashi, 2017/03/17 21:55:21]

Done.

+    SchedulePaintInRect(controller_->layout_model().GetRowBounds(
+        previous_row_selection.value()));
+  }
+
+  if (current_row_selection) {
+    SchedulePaintInRect(controller_->layout_model().GetRowBounds(
+        current_row_selection.value()));

[Evan Stade, 2017/03/16 17:39:00]

s/current_row_selection.value()/*current_row_selection/

(here and elsewhere)

[csashi, 2017/03/16 22:42:06]

Done.

+    DCHECK_LT(current_row_selection.value(),
+              static_cast<size_t>(child_count()));
+    views::View* child = child_at(current_row_selection.value());
+    DCHECK_EQ(child->GetClassName(), AutofillPopupChildView::kViewClassName);

[Evan Stade, 2017/03/16 17:39:00]

not sure this is necessary

[csashi, 2017/03/16 22:42:06]

Acknowledged. A previous reviewer asked that I add it in case other types of
child views were added. Seemed reasonable to check this.

[Evan Stade, 2017/03/17 14:11:55]

I mean, lots of things would break then right, like the size/child count checks.

[csashi, 2017/03/17 16:30:35]

Done.

+    AutofillPopupChildView* child_view =
+        static_cast<AutofillPopupChildView*>(child);

[Evan Stade, 2017/03/17 14:11:55]

nit: do you need this cast?

[csashi, 2017/03/17 16:30:35]

Done.

+    child_view->NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
+  }
 }
 
 /**
@@ -189,6 +260,19 @@ 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));
+}
+
+void AutofillPopupViewViews::CreateChildViews() {
+  for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
+    AutofillPopupChildView* child = new AutofillPopupChildView(controller_, i);
+    AddChildView(child);

[Evan Stade, 2017/03/16 17:39:00]

nit: combine these two lines.

[csashi, 2017/03/16 22:42:06]

Done.

+  }
+}
+
 AutofillPopupView* AutofillPopupView::Create(
     AutofillPopupController* controller) {
   views::Widget* observing_widget =