Focus first invalid field of payment request editor

chrome/browser/ui/views/payments/editor_view_controller.cc

Index: chrome/browser/ui/views/payments/editor_view_controller.cc
diff --git a/chrome/browser/ui/views/payments/editor_view_controller.cc b/chrome/browser/ui/views/payments/editor_view_controller.cc
index de66b31d1a1a9009503ef960bf73eff6f059b5df..ba1c853444371631de935567a66d2ee921354072 100644
--- a/chrome/browser/ui/views/payments/editor_view_controller.cc
+++ b/chrome/browser/ui/views/payments/editor_view_controller.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/payments/editor_view_controller.h"
 
+#include <algorithm>
 #include <map>
 #include <memory>
 #include <utility>
@@ -78,7 +79,7 @@ EditorViewController::EditorViewController(
     PaymentRequestDialogView* dialog,
     BackNavigationType back_navigation_type)
     : PaymentRequestSheetController(spec, state, dialog),
-      first_field_view_(nullptr),
+      initial_focus_field_view_(nullptr),
       back_navigation_type_(back_navigation_type) {}
 
 EditorViewController::~EditorViewController() {}
@@ -102,7 +103,8 @@ std::unique_ptr<views::View> EditorViewController::CreateHeaderView() {
 }
 
 std::unique_ptr<views::View> EditorViewController::CreateCustomFieldView(
-    autofill::ServerFieldType type) {
+    autofill::ServerFieldType type,
+    views::View** focusable_field) {
   return nullptr;
 }
 
@@ -140,8 +142,7 @@ void EditorViewController::FillContentView(views::View* content_view) {
 
 void EditorViewController::UpdateEditorView() {
   UpdateContentView();
-  // TODO(crbug.com/704254): Find how to update the parent view bounds so that
-  // the vertical scrollbar size gets updated.
+  UpdateFocus(GetFirstFocusedView());
   dialog()->EditorViewUpdated();
 }
 
@@ -167,8 +168,8 @@ void EditorViewController::ButtonPressed(views::Button* sender,
 }
 
 views::View* EditorViewController::GetFirstFocusedView() {
-  if (first_field_view_)
-    return first_field_view_;
+  if (initial_focus_field_view_)
+    return initial_focus_field_view_;
   return PaymentRequestSheetController::GetFirstFocusedView();
 }
 
@@ -185,6 +186,7 @@ std::unique_ptr<views::View> EditorViewController::CreateEditorView() {
   std::unique_ptr<views::View> editor_view = base::MakeUnique<views::View>();
   text_fields_.clear();
   comboboxes_.clear();
+  initial_focus_field_view_ = nullptr;
 
   // The editor view is padded horizontally.
   editor_view->SetBorder(views::CreateEmptyBorder(
@@ -257,8 +259,21 @@ std::unique_ptr<views::View> EditorViewController::CreateEditorView() {
                      kFieldExtraViewHorizontalPadding - long_extra_view_width;
   columns_long->AddPaddingColumn(0, long_padding);
 
-  for (const auto& field : GetFieldDefinitions())
-    CreateInputField(editor_layout.get(), field);
+  views::View* potential_first_field_view = nullptr;

[anthonyvd, 2017/05/18 21:15:37]

nit: first_field?

[MAD, 2017/05/19 13:43:51]

Done.

+  for (const auto& field : GetFieldDefinitions()) {
+    bool valid = false;
+    views::View* focusable_field =
+        CreateInputField(editor_layout.get(), field, &valid);
+    if (!initial_focus_field_view_) {
+      if (!valid)
+        initial_focus_field_view_ = focusable_field;
+      else if (!potential_first_field_view)

[anthonyvd, 2017/05/18 21:15:37]

If you pull this else if statement out of the parent if (the first field is
always going into this when it matters anyways), you'll be able to collapse both
conditions above into if (!initial_focus_field_view_ && !valid).

Basically either we need to remember the very first field, or it doesn't matter
because one of them is invalid.

[MAD, 2017/05/19 13:43:51]

I'm not sure I understand. The idea here is that, if initial_focus_field_view_
is set, there's nothing else to be done, no need to check for a potential first
field, since we already found it.

[anthonyvd, 2017/05/19 15:11:42]

Sorry I'm being unclear. The algorithm is "If at least one field is invalid,
focus the first invalid field. Otherwise, focus the first field in the editor"
so this code can become

views::View* first_field = nullptr;
for (const auto& field : GetFieldDefinitions()) {
    bool valid = false;
    views::View* focusable_field =
        CreateInputField(editor_layout.get(), field, &valid);
    if (!first_field)
        first_field = focusable_field;
    if (!initial_focus_field_view_ && !valid)
        initial_focus_field_view_ = focusable_field;
}

When this loop exists, |first_field| is always set to the first field in the
editor and |initial_focus_field_| is set to the first invalid one if there's
one. The if at line 275 takes care of the rest.

[MAD, 2017/05/19 15:35:35]

Done.

+        potential_first_field_view = focusable_field;
+    }
+  }
+
+  if (!initial_focus_field_view_)
+    initial_focus_field_view_ = potential_first_field_view;
 
   // Adds the "* indicates a required field" label in "disabled" grey text.
   std::unique_ptr<views::Label> required_field = base::MakeUnique<views::Label>(
@@ -286,8 +301,9 @@ std::unique_ptr<views::View> EditorViewController::CreateEditorView() {
 // |_______________________|__________________________________|
 // |   (empty)             | Error label                      |
 // +----------------------------------------------------------+
-void EditorViewController::CreateInputField(views::GridLayout* layout,
-                                            const EditorField& field) {
+views::View* EditorViewController::CreateInputField(views::GridLayout* layout,
+                                                    const EditorField& field,
+                                                    bool* valid) {
   int column_set =
       field.length_hint == EditorField::LengthHint::HINT_SHORT ? 0 : 1;
 
@@ -301,6 +317,8 @@ void EditorViewController::CreateInputField(views::GridLayout* layout,
   label->SetMultiLine(true);
   layout->AddView(label.release());
 
+  views::View* focusable_field = nullptr;

[anthonyvd, 2017/05/18 21:15:37]

Since this isn't necessarily related to focus anymore, maybe it can be renamed
to just field?

[MAD, 2017/05/19 13:43:51]

Well, it still kind of is, especially in the custom field case, where there
might be more than one field, but we want to return the one that should receive
focus. I could add documentation here if you want, but I thought it was clear
enough in the header file documentation of this method.

[anthonyvd, 2017/05/19 15:11:42]

Ah I didn't think about composite fields. Yeah that makes sense!

+
   constexpr int kInputFieldHeight = 28;
   if (field.control_type == EditorField::ControlType::TEXTFIELD) {
     ValidatingTextfield* text_field =
@@ -310,11 +328,8 @@ void EditorViewController::CreateInputField(views::GridLayout* layout,
     // Using autofill field type as a view ID (for testing).
     text_field->set_id(static_cast<int>(field.type));
     text_fields_.insert(std::make_pair(text_field, field));
-
-    // TODO(crbug.com/718582): Make the initial focus the first incomplete/empty
-    // field.
-    if (!first_field_view_)
-      first_field_view_ = text_field;
+    focusable_field = text_field;
+    *valid = text_field->IsValid();
 
     // |text_field| will now be owned by |row|.
     layout->AddView(text_field, 1, 1, views::GridLayout::FILL,
@@ -329,9 +344,8 @@ void EditorViewController::CreateInputField(views::GridLayout* layout,
     combobox->set_id(static_cast<int>(field.type));
     combobox->set_listener(this);
     comboboxes_.insert(std::make_pair(combobox, field));
-
-    if (!first_field_view_)
-      first_field_view_ = combobox;
+    focusable_field = combobox;
+    *valid = combobox->IsValid();
 
     // |combobox| will now be owned by |row|.
     layout->AddView(combobox, 1, 1, views::GridLayout::FILL,
@@ -339,7 +353,9 @@ void EditorViewController::CreateInputField(views::GridLayout* layout,
   } else {
     // Custom field view will now be owned by |row|. And it must be valid since
     // the derived class specified a custom view for this field.
-    std::unique_ptr<views::View> field_view = CreateCustomFieldView(field.type);
+    DCHECK(!focusable_field);
+    std::unique_ptr<views::View> field_view =
+        CreateCustomFieldView(field.type, &focusable_field);
     DCHECK(field_view);
     layout->AddView(field_view.release());
   }
@@ -360,6 +376,7 @@ void EditorViewController::CreateInputField(views::GridLayout* layout,
 
   // Bottom padding for the row.
   layout->AddPaddingRow(0, kInputRowSpacing);
+  return focusable_field;
 }
 
 int EditorViewController::ComputeWidestExtraViewWidth(