DialogClientView: Correct the order in which subviews are focused.

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 62 matching lines @@
 }
 
 void DialogClientView::CancelWindow() {
   // Only notify the delegate once. See |notified_delegate_|'s comment.
   if (!notified_delegate_ && GetDialogDelegate()->Cancel()) {
     notified_delegate_ = true;
     Close();
   }
 }
 
 void DialogClientView::UpdateDialogButtons() {

[sky, 2016/02/16 16:47:28]

Because UpdateDialogButtons can be called at any time, and the delegate could
return different values from GetDialogButtons, won't your change fail in certain
cases? For example, if we want ok->cancel ordering but cancel is enabled first,
then ok, won't the focus order be cancel->ok? Seems like the best solution is to
explicitly set next focusable view?

[karandeepb, 2016/02/17 09:16:14]

Yeah. Thanks for the catch. I am now explicitly setting the next focusable view.

-  const int buttons = GetDialogDelegate()->GetDialogButtons();
-  ui::Accelerator escape(ui::VKEY_ESCAPE, ui::EF_NONE);
-
-  if (buttons & ui::DIALOG_BUTTON_OK) {
-    if (!ok_button_) {
-      ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
-      if (!(buttons & ui::DIALOG_BUTTON_CANCEL))
-        ok_button_->AddAccelerator(escape);
-      AddChildView(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);
-      cancel_button_->AddAccelerator(escape);
-      AddChildView(cancel_button_);
-    }
-
-    UpdateButton(cancel_button_, ui::DIALOG_BUTTON_CANCEL);
-  } else if (cancel_button_) {
-    delete cancel_button_;
-    cancel_button_ = NULL;
+  // Add buttons in left to right order, so that the focus order matches.
+  if (kIsOkButtonOnLeftSide) {
+    UpdateOKButton();
+    UpdateCancelButton();
+  } else {
+    UpdateCancelButton();
+    UpdateOKButton();
   }
 
   // Use the escape key to close the window if there are no dialog buttons.
   if (!has_dialog_buttons())
-    AddAccelerator(escape);
+    AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
   else
     ResetAccelerators();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // DialogClientView, ClientView overrides:
 
 bool DialogClientView::CanClose() {
   if (notified_delegate_)
     return true;
@@ skipping 113 matching lines @@
 bool DialogClientView::AcceleratorPressed(const ui::Accelerator& accelerator) {
   DCHECK_EQ(accelerator.key_code(), ui::VKEY_ESCAPE);
   Close();
   return true;
 }
 
 void DialogClientView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   ClientView::ViewHierarchyChanged(details);
   if (details.is_add && details.child == this) {
+    CreateExtraView();
     UpdateDialogButtons();
-    CreateExtraView();
     CreateFootnoteView();
   } else if (!details.is_add && details.child != this) {
     if (details.child == ok_button_)
       ok_button_ = NULL;
     if (details.child == cancel_button_)
       cancel_button_ = NULL;
   }
 }
 
 void DialogClientView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
@@ skipping 90 matching lines @@
 }
 
 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());
 }
 
+void DialogClientView::UpdateOKButton() {
+  const int buttons = GetDialogDelegate()->GetDialogButtons();
+  if (buttons & ui::DIALOG_BUTTON_OK) {
+    if (!ok_button_) {
+      ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
+      if (!(buttons & ui::DIALOG_BUTTON_CANCEL))
+        ok_button_->AddAccelerator(
+            ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
+      AddChildView(ok_button_);
+    }
+
+    UpdateButton(ok_button_, ui::DIALOG_BUTTON_OK);
+  } else if (ok_button_) {
+    delete ok_button_;
+    ok_button_ = nullptr;
+  }
+}
+
+void DialogClientView::UpdateCancelButton() {
+  if (GetDialogDelegate()->GetDialogButtons() & ui::DIALOG_BUTTON_CANCEL) {
+    if (!cancel_button_) {
+      cancel_button_ = CreateDialogButton(ui::DIALOG_BUTTON_CANCEL);
+      cancel_button_->AddAccelerator(
+          ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
+      AddChildView(cancel_button_);
+    }
+
+    UpdateButton(cancel_button_, ui::DIALOG_BUTTON_CANCEL);
+  } else if (cancel_button_) {
+    delete cancel_button_;
+    cancel_button_ = nullptr;
+  }
+}
+
 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 {
   // NOTE: The insets only apply to the buttons, extra view, and footnote view.
   return GetButtonsAndExtraViewRowHeight() == 0 ? gfx::Insets() :
       gfx::Insets(0, kButtonHEdgeMarginNew,
                   kButtonVEdgeMarginNew, kButtonHEdgeMarginNew);
 }
 
 void DialogClientView::Close() {
   GetWidget()->Close();
   GetDialogDelegate()->OnClosed();
 }
 
 }  // namespace views