Focus first invalid field of payment request editor

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/payments/editor_view_controller.h"
 
+#include <algorithm>
 #include <map>
 #include <memory>
 #include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/views/payments/payment_request_dialog_view.h"
 #include "chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h"
 #include "chrome/browser/ui/views/payments/payment_request_views_util.h"
 #include "chrome/browser/ui/views/payments/validating_combobox.h"
@@ skipping 54 matching lines @@
 }
 
 }  // namespace
 
 EditorViewController::EditorViewController(
     PaymentRequestSpec* spec,
     PaymentRequestState* state,
     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() {}
 
 void EditorViewController::DisplayErrorMessageForField(
     const EditorField& field,
     const base::string16& error_message) {
   const auto& label_view_it = error_labels_.find(field);
   DCHECK(label_view_it != error_labels_.end());
 
   label_view_it->second->RemoveAllChildViews(/*delete_children=*/true);
   if (!error_message.empty()) {
     label_view_it->second->AddChildView(
         CreateErrorLabelView(error_message, field).release());
   }
   RelayoutPane();
 }
 
 std::unique_ptr<views::View> EditorViewController::CreateHeaderView() {
   return nullptr;
 }
 
 std::unique_ptr<views::View> EditorViewController::CreateCustomFieldView(
-    autofill::ServerFieldType type) {
+    autofill::ServerFieldType type,
+    views::View** focusable_field) {
   return nullptr;
 }
 
 std::unique_ptr<views::View> EditorViewController::CreateExtraViewForField(
     autofill::ServerFieldType type) {
   return nullptr;
 }
 
 std::unique_ptr<views::Button> EditorViewController::CreatePrimaryButton() {
   std::unique_ptr<views::Button> button(
@@ skipping 17 matching lines @@
   std::unique_ptr<views::View> header_view = CreateHeaderView();
   if (header_view.get())
     content_view->AddChildView(header_view.release());
 
   // The heart of the editor dialog: all the input fields with their labels.
   content_view->AddChildView(CreateEditorView().release());
 }
 
 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();
 }
 
 void EditorViewController::ButtonPressed(views::Button* sender,
                                          const ui::Event& event) {
   switch (sender->tag()) {
     case static_cast<int>(EditorViewControllerTags::SAVE_BUTTON):
       if (ValidateModelAndSave()) {
         switch (back_navigation_type_) {
           case BackNavigationType::kOneStep:
             dialog()->GoBack();
             break;
           case BackNavigationType::kPaymentSheet:
             dialog()->GoBackToPaymentSheet();
             break;
         }
       }
       break;
     default:
       PaymentRequestSheetController::ButtonPressed(sender, event);
       break;
   }
 }
 
 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();
 }
 
 void EditorViewController::ContentsChanged(views::Textfield* sender,
                                            const base::string16& new_contents) {
   static_cast<ValidatingTextfield*>(sender)->OnContentsChanged();
 }
 
 void EditorViewController::OnPerformAction(views::Combobox* sender) {
   static_cast<ValidatingCombobox*>(sender)->OnContentsChanged();
 }
 
 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(
       0, payments::kPaymentRequestRowHorizontalInsets, 0,
       payments::kPaymentRequestRowHorizontalInsets));
 
   // All views have fixed size except the Field which stretches. The fixed
   // padding at the end is computed so that Field views have a minimum of
   // 176/272dp (short/long fields) as per spec.
   // ___________________________________________________________________________
@@ skipping 52 matching lines @@
                           0, views::GridLayout::FIXED, long_extra_view_width,
                           0);
   // The padding at the end is fixed, computed to make sure the long field
   // maintains its minimum width.
   int long_padding = kDialogMinWidth - kLongFieldMinimumWidth - kLabelWidth -
                      (2 * kPaymentRequestRowHorizontalInsets) -
                      kLabelInputFieldHorizontalPadding -
                      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>(
       l10n_util::GetStringUTF16(IDS_PAYMENTS_REQUIRED_FIELD_MESSAGE));
   required_field->SetDisabledColor(
       required_field->GetNativeTheme()->GetSystemColor(
           ui::NativeTheme::kColorId_LabelDisabledColor));
   required_field->SetEnabled(false);
 
   views::ColumnSet* required_field_columns = editor_layout->AddColumnSet(2);
   required_field_columns->AddColumn(views::GridLayout::LEADING,
                                     views::GridLayout::CENTER, 1,
                                     views::GridLayout::USE_PREF, 0, 0);
   editor_layout->StartRow(0, 2);
   editor_layout->AddView(required_field.release());
 
   editor_view->SetLayoutManager(editor_layout.release());
 
   return editor_view;
 }
 
 // Each input field is a 4-quadrant grid.
 // +----------------------------------------------------------+
 // | Field Label           | Input field (textfield/combobox) |
 // |_______________________|__________________________________|
 // |   (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;
 
   // This is the top padding for every row.
   constexpr int kInputRowSpacing = 6;
   layout->StartRowWithPadding(0, column_set, 0, kInputRowSpacing);
 
   std::unique_ptr<views::Label> label = base::MakeUnique<views::Label>(
       field.required ? field.label + base::ASCIIToUTF16("*") : field.label);
 
   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 =
         new ValidatingTextfield(CreateValidationDelegate(field));
     text_field->SetText(GetInitialValueForType(field.type));
     text_field->set_controller(this);
     // 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,
                     views::GridLayout::FILL, 0, kInputFieldHeight);
   } else if (field.control_type == EditorField::ControlType::COMBOBOX) {
     ValidatingCombobox* combobox = new ValidatingCombobox(
         GetComboboxModelForType(field.type), CreateValidationDelegate(field));
     base::string16 initial_value = GetInitialValueForType(field.type);
     if (!initial_value.empty())
       combobox->SelectValue(initial_value);
     // Using autofill field type as a view ID.
     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,
                     views::GridLayout::FILL, 0, kInputFieldHeight);
   } 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());
   }
 
   // If an extra view needs to go alongside the input field view, add it to the
   // last column.
   std::unique_ptr<views::View> extra_view = CreateExtraViewForField(field.type);
   if (extra_view)
     layout->AddView(extra_view.release());
 
   layout->StartRow(0, column_set);
   layout->SkipColumns(1);
   std::unique_ptr<views::View> error_label_view =
       base::MakeUnique<views::View>();
   error_label_view->SetLayoutManager(new views::FillLayout);
   error_labels_[field] = error_label_view.get();
   layout->AddView(error_label_view.release());
 
   // Bottom padding for the row.
   layout->AddPaddingRow(0, kInputRowSpacing);
+  return focusable_field;
 }
 
 int EditorViewController::ComputeWidestExtraViewWidth(
     EditorField::LengthHint size) {
   int widest_column_width = 0;
 
   for (const auto& field : GetFieldDefinitions()) {
     if (field.length_hint != size)
       continue;
 
     std::unique_ptr<views::View> extra_view =
         CreateExtraViewForField(field.type);
     if (!extra_view)
       continue;
     widest_column_width =
         std::max(extra_view->GetPreferredSize().width(), widest_column_width);
   }
   return widest_column_width;
 }
 
 }  // namespace payments