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..719308982e468ba5f480fa59eb10bb3f18313e6c 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,15 @@ 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;
+  for (const auto& field : GetFieldDefinitions()) {
+    views::View* focusable_field = CreateInputField(editor_layout.get(), field);
+    if (!potential_first_field_view)

[anthonyvd, 2017/05/18 18:12:25]

I find this really hard to follow because initial_focus_field_view_ and friends
are update in multiple places in the code. Can we instead change
CreateInputField to take a views::View** (the created field) and a bool*
(whether the created field is valid or not) as arguments? This way, we can
remove the focusing logic from CreateInputField entirely and change this loop
here to 

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

if (!initial_focus_field_view_)
  initial_focus_field_view_ = first_field;

Which is *slightly* more lines but makes it clearer what's happening and
centralizes the focus choosing logic here.

WDYT?

[MAD, 2017/05/18 20:31:19]

Yeah, I agree that it wasn't very elegant, I shouldn't have tried to keep the
concept of the CreateInputField updating the data member, and returning
something for the caller.

Thanks for the suggestion.

+      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 +295,8 @@ 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) {
   int column_set =
       field.length_hint == EditorField::LengthHint::HINT_SHORT ? 0 : 1;
 
@@ -301,6 +310,8 @@ void EditorViewController::CreateInputField(views::GridLayout* layout,
   label->SetMultiLine(true);
   layout->AddView(label.release());
 
+  views::View* focusable_field = nullptr;
+
   constexpr int kInputFieldHeight = 28;
   if (field.control_type == EditorField::ControlType::TEXTFIELD) {
     ValidatingTextfield* text_field =
@@ -310,11 +321,9 @@ 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;
+    if (!initial_focus_field_view_ && !text_field->IsValid())
+      initial_focus_field_view_ = text_field;
 
     // |text_field| will now be owned by |row|.
     layout->AddView(text_field, 1, 1, views::GridLayout::FILL,
@@ -329,9 +338,9 @@ 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;
+    if (!initial_focus_field_view_ && !combobox->IsValid())
+      initial_focus_field_view_ = combobox;
 
     // |combobox| will now be owned by |row|.
     layout->AddView(combobox, 1, 1, views::GridLayout::FILL,
@@ -339,7 +348,8 @@ 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);
+    std::unique_ptr<views::View> field_view =
+        CreateCustomFieldView(field.type, &focusable_field);
     DCHECK(field_view);
     layout->AddView(field_view.release());
   }
@@ -360,6 +370,7 @@ void EditorViewController::CreateInputField(views::GridLayout* layout,
 
   // Bottom padding for the row.
   layout->AddPaddingRow(0, kInputRowSpacing);
+  return focusable_field;
 }
 
 int EditorViewController::ComputeWidestExtraViewWidth(