MacViews: Ensure cancel buttons don't get default button styling.

ui/views/window/dialog_client_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

[tapted, 2017/01/04 23:11:40]

CL description: Harfmony -> Harmony

 // 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/base/material_design/material_design_controller.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/blue_button.h"
 #include "ui/views/controls/button/custom_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/layout/layout_constants.h"
+#include "ui/views/style/platform_style.h"
 #include "ui/views/views_delegate.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 
 namespace views {
 
 namespace {
 
 // The group used by the buttons.  This name is chosen voluntarily big not to
 // conflict with other groups that could be in the dialog content.
@@ skipping 287 matching lines @@
 void DialogClientView::ChildVisibilityChanged(View* child) {
   ChildPreferredSizeChanged(child);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DialogClientView, private:
 
 LabelButton* DialogClientView::CreateDialogButton(ui::DialogButton type) {
   const base::string16 title = GetDialogDelegate()->GetDialogButtonLabel(type);
   LabelButton* button = nullptr;
+
+  const bool is_default =
+      GetDialogDelegate()->GetDefaultDialogButton() == type &&
+      (type != ui::DIALOG_BUTTON_CANCEL ||

[karandeepb, 2017/01/04 07:46:51]

This is similar to what we do in DialogDelegate::UpdateButton. This seems a bit
ugly to me, I think GetDefaultDialogButton should not return CANCEL button on
Mac. However, the problem I think is that GetDefaultDialogButton is virtual. 

One solution may be to have a public non-virtual
GetDefaultDialogButtonPublic(say) on DialogDelegate which delegates to a private
virtual GetDefaultDialogButton. Then GetDefaultDialogButtonPublic can take
kDialogDefaultButtonCanBeCancel into account. Alternatively, the existing
overrides can take kDialogDefaultButtonCanBeCancel into account, but that'll be
error prone.

Also, I haven't modified DialogDelegate::GetInitiallyFocusedView to take
kDialogDefaultButtonCanBeCancel into account, since that would cause no view to
be focused initially for some dialogs.

[tapted, 2017/01/04 23:11:40]

IMO the nicer fix would be to merge the Blue/Regular MD Button codepaths some
more, so that the color can be inferred from SetIsDefault(). It looks like, on
MD, the blue button is actually the same class, just with a call to
MdTextButton::SetProminent(true). It's only on non-MD that it's a different
class. So this may get a cleanup when non-MD buttons get torn out.
I think Patti's change will help move the focus along if someone isn't using
full keyboard access - hopefully we don't have to worry about this.

+       PlatformStyle::kDialogDefaultButtonCanBeCancel);
+
   // The default button is always blue in Harmony.
-  if (GetDialogDelegate()->GetDefaultDialogButton() == type &&
-      (ui::MaterialDesignController::IsSecondaryUiMaterial() ||
-       GetDialogDelegate()->ShouldDefaultButtonBeBlue())) {
+  if (is_default && (ui::MaterialDesignController::IsSecondaryUiMaterial() ||
+                     GetDialogDelegate()->ShouldDefaultButtonBeBlue())) {
     button = MdTextButton::CreateSecondaryUiBlueButton(this, title);
   } else {
     button = MdTextButton::CreateSecondaryUiButton(this, title);
   }
 
   const int kDialogMinButtonWidth = 75;
   button->SetMinSize(gfx::Size(kDialogMinButtonWidth, 0));
   button->SetGroup(kButtonGroup);
   return button;
 }
@@ skipping 47 matching lines @@
       std::remove(child_views.begin(), child_views.end(), nullptr),
       child_views.end());
 
   // Setup focus by reordering views. It is not safe to use SetNextFocusableView
   // since child views may be added externally to this view.
   for (size_t i = 0; i < child_views.size(); i++)
     ReorderChildView(child_views[i], i);
 }
 
 }  // namespace views