Preliminary implementation of Google-style dialogs in ash::internal::DialogFrameView

ash/wm/dialog_frame_view.cc

Index: ash/wm/dialog_frame_view.cc
diff --git a/ash/wm/dialog_frame_view.cc b/ash/wm/dialog_frame_view.cc
index 80bc098d6e461e5dfcb568c883e308fa1d72b06d..dd3d351957ab071d30ff7923953adfccb4787df1 100644
--- a/ash/wm/dialog_frame_view.cc
+++ b/ash/wm/dialog_frame_view.cc
@@ -4,14 +4,47 @@
 
 #include "ash/wm/dialog_frame_view.h"
 
+#include "grit/ui_resources_standard.h"
 #include "ui/base/hit_test.h"
+#include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font.h"
+#include "ui/gfx/insets.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
+#include "ui/views/controls/button/image_button.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 
+namespace {
+
+// TODO(benrg): Make frame shadow and stroke agree with the spec.

[Ben Goodger (Google), 2012/01/13 18:14:50]

Not as a comment on this CL but for your general reference... I think we can
programmatically generate shadows. This is probably what we'll want to do going
forward so we can easily tweak shadow metrics without having to roundtrip with
the designers for new assets.

The way to do this is to use a blur mask filter and a SkDrawLooper... this is
what our Skia implementation of webkit-box-shadow does. You can look at

src/third_party/WebKit/Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp

... as an example of the skia code to write to generate a nice configurable blur
shadow.

+
+const SkColor kDialogBackgroundColor = SK_ColorWHITE;
+const SkColor kDialogTitleColor = SK_ColorBLACK;
+
+// Dialog-size-dependent padding values from the spec, in pixels.
+const int kGoogleSmallDialogVPadding = 16;
+const int kGoogleSmallDialogHPadding = 20;
+const int kGoogleMediumDialogVPadding = 28;
+const int kGoogleMediumDialogHPadding = 32;
+const int kGoogleLargeDialogVPadding = 30;
+const int kGoogleLargeDialogHPadding = 42;
+
+// Dialog layouts themselves contain some padding, which should be ignored in
+// favor of the padding values above. Currently we hack it by nudging the
+// client area outward.
+const int kDialogHPaddingNudge = 13;
+const int kDialogTopPaddingNudge = 5;
+const int kDialogBottomPaddingNudge = 10;
+
+// TODO(benrg): There are no examples in the spec of close boxes on medium- or
+// small-size dialogs, and the close button size is not factored into other
+// sizing decisions. This value could cause problems with smaller dialogs.
+const int kCloseButtonSize = 44;
+
+}  // namespace
+
 namespace ash {
 namespace internal {
 
@@ -21,24 +54,6 @@ const char DialogFrameView::kViewClassName[] = "ash/wm/DialogFrameView";
 // static
 gfx::Font* DialogFrameView::title_font_ = NULL;
 
-// TODO(benrg): tweak these values until they match Google-style.
-// TODO(benrg): this may also mean tweaking the frame shadow opacity.
-const SkColor kDialogBackgroundColor = SK_ColorWHITE;
-// const SkColor kDialogBorderColor = SkColorSetRGB(0xCC, 0xCC, 0xCC);
-const SkColor kDialogTitleColor = SK_ColorBLACK;
-
-// TODO(benrg): Replace with width-based padding heuristic.
-// |kDialogPadding| is the standardized padding amount, the specific per-side
-// padding values are offset from this to achieve a uniform look with our
-// existing code.
-static int kDialogPadding = 30;
-static int kDialogHPadding = kDialogPadding - 13;
-static int kDialogTopPadding = kDialogPadding;
-static int kDialogBottomPadding = kDialogPadding - 10;
-static int kDialogContentVSpacing = 5;
-
-const int kCloseButtonSize = 16;
-
 ////////////////////////////////////////////////////////////////////////////////
 // DialogFrameView, public:
 
@@ -47,6 +62,18 @@ DialogFrameView::DialogFrameView() {
       kDialogBackgroundColor));
   if (!title_font_)
     title_font_ = new gfx::Font(gfx::Font().DeriveFont(4, gfx::Font::NORMAL));
+
+  ResourceBundle& rb = ResourceBundle::GetSharedInstance();
+  close_button_ = new views::ImageButton(this);
+  close_button_->SetImage(views::CustomButton::BS_NORMAL,
+      rb.GetBitmapNamed(IDR_CLOSE_BAR));
+  close_button_->SetImage(views::CustomButton::BS_HOT,
+      rb.GetBitmapNamed(IDR_CLOSE_BAR_H));
+  close_button_->SetImage(views::CustomButton::BS_PUSHED,
+      rb.GetBitmapNamed(IDR_CLOSE_BAR_P));
+  close_button_->SetImageAlignment(views::ImageButton::ALIGN_CENTER,
+                                   views::ImageButton::ALIGN_MIDDLE);
+  AddChildView(close_button_);
 }
 
 DialogFrameView::~DialogFrameView() {
@@ -57,23 +84,22 @@ DialogFrameView::~DialogFrameView() {
 
 gfx::Rect DialogFrameView::GetBoundsForClientView() const {
   gfx::Rect client_bounds = GetLocalBounds();
-  client_bounds.Inset(kDialogHPadding, GetNonClientTopHeight(),
-                      kDialogHPadding, kDialogBottomPadding);
+  client_bounds.Inset(GetClientInsets());
   return client_bounds;
 }
 
 gfx::Rect DialogFrameView::GetWindowBoundsForClientBounds(
     const gfx::Rect& client_bounds) const {
   gfx::Rect window_bounds = client_bounds;
-  window_bounds.Inset(-kDialogHPadding, -GetNonClientTopHeight(),
-                      -kDialogHPadding, -kDialogBottomPadding);
+  window_bounds.Inset(-GetClientInsets());
   return window_bounds;
 }
 
 int DialogFrameView::NonClientHitTest(const gfx::Point& point) {
-  if (close_button_rect_.Contains(point))
+  // TODO(benrg): what exactly should this method do?

[Ben Goodger (Google), 2012/01/13 18:14:50]

I think what you have here is fine as specced. This is the views version of
WM_NCHITTEST.

[benrg, 2012/01/13 19:12:41]

I'm confused because of the other implementations.
chromeos::BubbleFrameView::NonClientHitTest returns HTNOWHERE unconditionally
(not even checking for the close box). OpaqueBrowserFrameView::NonClientHitTest
returns HTNOWHERE when the point is outside the nonclient bounds, does check for
the close box, and also calls frame()->client_view()->NonClientHitTest and
returns that value under some circumstances.

[Ben Goodger (Google), 2012/01/13 20:14:25]

BubbleFrameView is just wrong... OBFV is probably being OCD. I don't think
that's necessary.

+  if (close_button_->GetMirroredBounds().Contains(point))
     return HTCLOSE;
-  return point.y() < GetNonClientTopHeight() ? HTCAPTION : HTCLIENT;
+  return point.y() < GetClientInsets().top() ? HTCAPTION : HTCLIENT;
 }
 
 void DialogFrameView::GetWindowMask(const gfx::Size& size,
@@ -98,19 +124,18 @@ std::string DialogFrameView::GetClassName() const {
 
 void DialogFrameView::Layout() {
   title_display_rect_ = GetLocalBounds();
-  // TODO(benrg): size based on font height rather than bottom padding.
-  close_button_rect_.SetRect(width() - kDialogPadding - kCloseButtonSize,
-                             kDialogTopPadding, kCloseButtonSize,
-                             kCloseButtonSize);
-  title_display_rect_.Inset(kDialogPadding, kDialogTopPadding,
-                            kDialogPadding + kCloseButtonSize,
-                            kDialogBottomPadding);
+  title_display_rect_.Inset(GetPaddingInsets());
   title_display_rect_.set_height(title_font_->GetHeight());
+
+  // The hot rectangle for the close button is flush with the upper right of the
+  // dialog. The close button image is smaller, and is centered in the hot rect.
+  close_button_->SetBounds(
+      width() - kCloseButtonSize,
+      0, kCloseButtonSize, kCloseButtonSize);
 }
 
 void DialogFrameView::OnPaint(gfx::Canvas* canvas) {
   views::View::OnPaint(canvas);
-  canvas->FillRect(SK_ColorRED, close_button_rect_);
   views::WidgetDelegate* delegate = GetWidget()->widget_delegate();
   if (!delegate)
     return;
@@ -118,22 +143,47 @@ void DialogFrameView::OnPaint(gfx::Canvas* canvas) {
                         kDialogTitleColor, title_display_rect_);
 }
 
-// TODO(benrg): You may want to use a views::Button for the close box instead.
-bool DialogFrameView::OnMousePressed(const views::MouseEvent& event) {
-  if (close_button_rect_.Contains(event.location()))
-    return true;
-  return View::OnMousePressed(event);
-}
-void DialogFrameView::OnMouseReleased(const views::MouseEvent& event) {
-  if (close_button_rect_.Contains(event.location()))
+////////////////////////////////////////////////////////////////////////////////
+// DialogFrameView, views::ButtonListener overrides:
+
+void DialogFrameView::ButtonPressed(views::Button* sender,
+                                    const views::Event& event) {
+  if (sender == close_button_)
     GetWidget()->Close();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DialogFrameView, private:
 
-int DialogFrameView::GetNonClientTopHeight() const {
-  return kDialogTopPadding + title_font_->GetHeight() + kDialogContentVSpacing;
+gfx::Insets DialogFrameView::GetPaddingInsets() const {
+  // These three discrete padding sizes come from the spec. If we ever wanted
+  // our Google-style dialogs to be resizable, we would probably need to
+  // smoothly interpolate the padding size instead.
+  int v_padding, h_padding;
+  if (width() <= 280) {
+    v_padding = kGoogleSmallDialogVPadding;
+    h_padding = kGoogleSmallDialogHPadding;
+  } else if (width() <= 480) {
+    v_padding = kGoogleMediumDialogVPadding;
+    h_padding = kGoogleMediumDialogHPadding;
+  } else {
+    v_padding = kGoogleLargeDialogVPadding;
+    h_padding = kGoogleLargeDialogHPadding;
+  }
+  return gfx::Insets(v_padding, h_padding, v_padding, h_padding);
+}
+
+gfx::Insets DialogFrameView::GetClientInsets() const {
+  gfx::Insets insets = GetPaddingInsets();
+  // The title should be separated from the client area by 1 em (dialog spec),
+  // and one em is equal to the font size (CSS spec).
+  insets += gfx::Insets(
+      title_font_->GetHeight() + title_font_->GetFontSize() -
+          kDialogTopPaddingNudge,
+      -kDialogHPaddingNudge,
+      -kDialogBottomPaddingNudge,
+      -kDialogHPaddingNudge);
+  return insets;
 }
 
 }  // namespace internal