DialogClientView: Fix regression in Chrome Task Manager focusing.

ui/views/window/dialog_client_view.cc

 // Copyright (c) 2012 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 "ui/views/window/dialog_client_view.h"
 
 #include <algorithm>
 
 #include "build/build_config.h"
 #include "ui/events/keycodes/keyboard_codes.h"
@@ skipping 74 matching lines @@
     GetWidget()->Close();
   }
 }
 
 void DialogClientView::UpdateDialogButtons() {
   const int buttons = GetDialogDelegate()->GetDialogButtons();
 
   if (buttons & ui::DIALOG_BUTTON_OK) {
     if (!ok_button_) {
       ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
-      AddChildView(ok_button_);
+      AddOKButton(ok_button_);
     }
 
     UpdateButton(ok_button_, ui::DIALOG_BUTTON_OK);
   } else if (ok_button_) {
     delete ok_button_;
     ok_button_ = NULL;
   }
 
   if (buttons & ui::DIALOG_BUTTON_CANCEL) {
     if (!cancel_button_) {
       cancel_button_ = CreateDialogButton(ui::DIALOG_BUTTON_CANCEL);
-      AddChildView(cancel_button_);
+      AddCancelButton(cancel_button_);
     }
 
     UpdateButton(cancel_button_, ui::DIALOG_BUTTON_CANCEL);
   } else if (cancel_button_) {
     delete cancel_button_;
     cancel_button_ = NULL;
   }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ skipping 108 matching lines @@
     UpdateDialogButtons();
     CreateExtraView();
   } else if (!details.is_add && details.child != this) {
     if (details.child == ok_button_)
       ok_button_ = nullptr;
     else if (details.child == cancel_button_)
       cancel_button_ = nullptr;
     else if (details.child == extra_view_)
       extra_view_ = nullptr;
   }
-
-  SetupFocusChain();
 }
 
 void DialogClientView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   // The old dialog style needs an explicit background color, while the new
   // dialog style simply inherits the bubble's frame view color.
   const DialogDelegate* dialog = GetDialogDelegate();
 
   if (dialog && !dialog->UseNewStyleForThisDialog()) {
     set_background(views::Background::CreateSolidBackground(GetNativeTheme()->
         GetSystemColor(ui::NativeTheme::kColorId_DialogBackground)));
@@ skipping 30 matching lines @@
   return GetWidget()->widget_delegate()->AsDialogDelegate();
 }
 
 void DialogClientView::CreateExtraView() {
   if (extra_view_)
     return;
 
   extra_view_ = GetDialogDelegate()->CreateExtraView();
   if (extra_view_) {
     extra_view_->SetGroup(kButtonGroup);
-    AddChildView(extra_view_);
+    // Add |extra_view_| after contents_view, if it exists in the view hierarchy
+    // to ensure the correct focus order.
+    int content_view_index = GetIndexOf(contents_view());
+    // The content_view should be the first child, if it exists in the view
+    // hierarchy.
+    DCHECK(content_view_index == -1 || content_view_index == 0);
+    AddChildViewAt(extra_view_, content_view_index + 1);
   }
 }
 
 void DialogClientView::ChildPreferredSizeChanged(View* child) {
   if (child == extra_view_)
     Layout();
 }
 
 void DialogClientView::ChildVisibilityChanged(View* child) {
   ChildPreferredSizeChanged(child);
@@ skipping 14 matching lines @@
     button->SetStyle(Button::STYLE_BUTTON);
   }
   button->SetFocusable(true);
 
   const int kDialogMinButtonWidth = 75;
   button->SetMinSize(gfx::Size(kDialogMinButtonWidth, 0));
   button->SetGroup(kButtonGroup);
   return button;
 }
 
+void DialogClientView::AddOKButton(LabelButton* ok_button) {
+  // Find the view which should precede |ok_button| in focus order.
+  View* previous_view = nullptr;
+  if (!kIsOkButtonOnLeftSide && cancel_button_)
+    previous_view = cancel_button_;
+  else if (extra_view_)
+    previous_view = extra_view_;
+  else
+    previous_view = contents_view();
+  int previous_index = GetIndexOf(previous_view);
+
+  // previous_index may be -1. This may happen when the contents_view() is null
+  // or hasn't been added to the view hierarchy.
+  AddChildViewAt(ok_button, previous_index + 1);
+}
+
+void DialogClientView::AddCancelButton(LabelButton* cancel_button) {
+  // Find the view which should precede |cancel_button| in focus order.
+  View* previous_view = nullptr;
+  if (kIsOkButtonOnLeftSide && ok_button_)
+    previous_view = ok_button_;
+  else if (extra_view_)
+    previous_view = extra_view_;
+  else
+    previous_view = contents_view();
+  int previous_index = GetIndexOf(previous_view);
+
+  // previous_index may be -1. This may happen when the contents_view() is null
+  // or hasn't been added to the view hierarchy.
+  AddChildViewAt(cancel_button, previous_index + 1);
+}
+
 void DialogClientView::UpdateButton(LabelButton* button,
                                     ui::DialogButton type) {
   DialogDelegate* dialog = GetDialogDelegate();
   button->SetText(dialog->GetDialogButtonLabel(type));
   button->SetEnabled(dialog->IsDialogButtonEnabled(type));
   button->SetIsDefault(type == dialog->GetDefaultDialogButton());
 }
 
 int DialogClientView::GetButtonsAndExtraViewRowHeight() const {
   int extra_view_height = ShouldShow(extra_view_) ?
       extra_view_->GetPreferredSize().height() : 0;
   int buttons_height = std::max(
       ok_button_ ? ok_button_->GetPreferredSize().height() : 0,
       cancel_button_ ? cancel_button_->GetPreferredSize().height() : 0);
   return std::max(extra_view_height, buttons_height);
 }
 
 gfx::Insets DialogClientView::GetButtonRowInsets() const {
   return GetButtonsAndExtraViewRowHeight() == 0 ? gfx::Insets()
                                                 : button_row_insets_;
 }
 
-void DialogClientView::SetupFocusChain() {

[sky, 2016/03/22 16:19:23]

This code seems simpler to me. Is there a reason you can't add extra_view_ to
child_views?
An alternative to SetNextFocusableView is to reorder the views using
ReorderChildView.

[karandeepb, 2016/04/06 23:06:53]

extra_view_ is being currently added to child views. The regression occurred
since other clients may add views to the DialogClientView, but SetupFocusChain
did not take that into account. The changes in dialog_client_view_unittest.cc
illustrate this.

ReorderChildView is a good suggestion. However I have to modify the
SetupFocusChain call site since it's not safe to call ReorderChildView while a
view is being removed. For example the code at
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s...
assumes that the call to PropagateRemoveNotifications does not alter the
position of the view being removed.

-  // Create a vector of child views in the order of intended focus.
-  std::vector<View*> child_views;
-  child_views.push_back(contents_view());
-  child_views.push_back(extra_view_);
-  if (kIsOkButtonOnLeftSide) {
-    child_views.push_back(ok_button_);
-    child_views.push_back(cancel_button_);
-  } else {
-    child_views.push_back(cancel_button_);
-    child_views.push_back(ok_button_);
-  }
-
-  // Remove all null views from the vector.
-  child_views.erase(
-      std::remove(child_views.begin(), child_views.end(), nullptr),
-      child_views.end());
-
-  // Setup focus.
-  for (size_t i = 0; i < child_views.size(); i++) {
-    child_views[i]->SetNextFocusableView(
-        i + 1 != child_views.size() ? child_views[i + 1] : nullptr);
-  }
-}
-
 }  // namespace views