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..5172da61b57f1279ec763917d4871519870a87b3 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"
@@ -16,30 +19,87 @@
 #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.
+  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
+
+// static
+constexpr int AutofillPopupView::kNoSelection;

[Evan Stade, 2017/03/15 15:18:58]

I'm confused why you need this line or what it's doing

[csashi, 2017/03/15 21:31:09]

It is a constant to indicate no selection. I tried removing it and converting
everything to Optional, but because we don't want to pass Optional as argument,
I still need a special value to indicate there was no selection. If I remove
this, I would need to convert the parameters to size_t * and pass nullptr to
mean no selection, but that seemed less desirable than having a special value
for no selection.

[Evan Stade, 2017/03/16 01:23:44]

I understand the purpose of kNoSelection, but it's declared and defined in a
header and then re-declared here with no definition. This line serves what
purpose?
why not?

[csashi, 2017/03/16 01:50:55]

Don't we need an initializer for static variable? 
I did not use optional based on following documentation:
https://codesearch.chromium.org/chromium/src/docs/optional.md?q=base::Optiona...

[csashi, 2017/03/16 01:53:41]

Removed. I must have misunderstood.

[csashi, 2017/03/16 02:18:23]

Sorry for the flip-flop. I put it back based on the advice @
https://engdoc.corp.google.com/eng/doc/devguide/cpp/primer.shtml?cl=head#stat....

I noticed that there are some cases in Chrome where we have skipped the
definition in the .cc (e.g.  kMaxPolymorphicMapCount) and some cases where we
have both the declaration and definition (e.g. 
RenderText::kDragToEndIfOutsideVerticalBounds).

+
 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.
+  // Currently, child views get the data from 'controller_'. If the child views
+  // save additional state corresponding to the suggestion, we must recreate the
+  // child views even when |GetLineCount()| == |child_count()| because the
+  // suggestion content may change.

[Evan Stade, 2017/03/15 15:18:58]

I think we should remove this conditional and the comment about it. I don't
think the optimization is buying us much and may not be valid in the near
future.

[csashi, 2017/03/15 21:31:08]

Done.

+  if (controller_->GetLineCount() != static_cast<size_t>(child_count())) {
+    RemoveAllChildViews(true /* delete_children */);
+    CreateChildViews();
+  }
   DoUpdateBoundsAndRedrawPopup();
 }
 
@@ -51,6 +111,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 +127,26 @@ void AutofillPopupViewViews::OnPaint(gfx::Canvas* canvas) {
   }
 }
 
-void AutofillPopupViewViews::InvalidateRow(size_t row) {
-  SchedulePaintInRect(controller_->layout_model().GetRowBounds(row));
+void AutofillPopupViewViews::OnSelectedRowChanged(
+    size_t previous_row_selection,
+    size_t current_row_selection) {
+  if (previous_row_selection != static_cast<size_t>(kNoSelection) &&

[Evan Stade, 2017/03/15 15:18:58]

it seems off that you're casting -1 to a size_t. Either use int for the row
indexing or use base::Optional<size_t> (I prefer the latter)

[csashi, 2017/03/15 21:31:08]

Done. I did not use optional because of following documentation:
https://codesearch.chromium.org/chromium/src/docs/optional.md?q=base::Optiona...

+      previous_row_selection < controller_->GetLineCount())
+    SchedulePaintInRect(
+        controller_->layout_model().GetRowBounds(previous_row_selection));

[Evan Stade, 2017/03/15 15:18:58]

nit: please use curlies and add a newline after

[csashi, 2017/03/15 21:31:08]

Done.

+  if (current_row_selection != static_cast<size_t>(kNoSelection)) {
+    DCHECK_GE(current_row_selection, static_cast<size_t>(0));

[Evan Stade, 2017/03/15 15:18:58]

is it even possible for this check to fail? size_t is always >= 0

[csashi, 2017/03/15 21:31:08]

Done.

+    DCHECK_LT(current_row_selection, controller_->GetLineCount());

[Evan Stade, 2017/03/15 15:18:58]

I think these DCHECKs are a little over the top and are making this function
hard to read. It's not the view's job to verify the controller is acting
sensibly.

[csashi, 2017/03/15 21:31:08]

Done. Agreed.  I did not know suggestions could changes without recreating this
class and could not understand the root cause of the bug and went overboard.

+    SchedulePaintInRect(
+        controller_->layout_model().GetRowBounds(current_row_selection));
+    DCHECK_LT(current_row_selection, static_cast<size_t>(child_count()));
+    views::View* child = child_at(current_row_selection);
+    DCHECK(child);

[Evan Stade, 2017/03/15 15:18:58]

nit: this is redundant with the check on L142

I think all DCHECKs in this function can boil down to making sure child_count()
== GetLineCount(). But it should be enough that you added that in OnPaint.

[csashi, 2017/03/15 21:31:08]

Done.

+    DCHECK_EQ(child->GetClassName(), AutofillPopupChildView::kViewClassName);
+    AutofillPopupChildView* child_view =
+        static_cast<AutofillPopupChildView*>(child);
+    child_view->NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
+  }
 }
 
 /**
@@ -189,6 +268,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);
+  }
+}
+
 AutofillPopupView* AutofillPopupView::Create(
     AutofillPopupController* controller) {
   views::Widget* observing_widget =