Correct padding and focus ring for frameless constrained window dialog

chrome/browser/ui/constrained_window.h

 // 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.
 
 #ifndef CHROME_BROWSER_UI_CONSTRAINED_WINDOW_H_
 #define CHROME_BROWSER_UI_CONSTRAINED_WINDOW_H_
 
 #include "build/build_config.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/native_widget_types.h"
 
 ///////////////////////////////////////////////////////////////////////////////
 // ConstrainedWindow
 //
 //  This interface represents a window that is constrained to a
 //  WebContentsView's bounds.
 //
 class ConstrainedWindow {
  public:
-  static const int kVerticalPadding = 14; // top/bottom padding.
-  static const int kHorizontalPadding = 17; // left/right padding.
-  static const int kRowPadding = 20;  // Vertical margin between dialog rows.
-  static const int kBorderRadius = 2;  // Border radius for dialog corners.
+  // The kTitleTopPadding is the padding between the top of the window and the

[Ben Goodger (Google), 2012/10/10 16:11:54]

I would tend to put all of this in a file separate from the constrained window
API (constrained_window_constants.h?). it's huge and distracting. Also I think
if you come up with clear variable names the description/diagrams are probably
unnecessary.

[please use gerrit instead, 2012/10/10 19:12:58]

Kenmoore@ has asked to use regular padding without mentioning the x-heights and
baselines of fonts. This means that we do not need to have the diagrams any
more. I am removing them.

+  // title. The goal is to have 20px between the top of the window and the
+  // x-height of the message.
+  //
+  // The kVerticalPadding is the default padding beetween the buttons on the
+  // bottom of the window and the bottom border.
+  //
+  // The kHorizontalPadding is the default padding between the left size of the
+  // window and the contents; and between the right side of the window and the
+  // contents.
+  //
+  // The kRowPadding is the default padding between the window title and the
+  // message below. The goal is to have 20px between the base of the title and
+  // the x-height of the message.
+  //
+  //  ______________________________________________________________________
+  // |                     ^                        ^
+  // |                     |                        |
+  // |                     |--kTitleTopPadding      |
+  // | kHorizontalPadding  |                        |
+  // |     |      _________v_                       |--20px
+  // |<--------->|____   ____|                      |
+  // |                | |   _                       |
+  // |                | |  |_|    _     _           |
+  // |                | |   _   _| |_  | |  ____   _v__  _ _ _ _ x-height
+  // |                | |  | | |_   _| | | |  _ | |    |
+  // |                | |  | |   | |_  | | |  __| | [] |
+  // |                |_|  |_|   |___| |_| |____| |  __| _ _ _ _ baseline
+  // |               _ _ _ _ _ _ _^_ _ _ _ _ _ _  |_|
+  // |               ^            |
+  // |  kRowPadding--|            |
+  // |            _  v            |--20px
+  // |           ||  ||           |
+  // |           ||  || _  _  _  _v _  _  _ _ _ _ x-height
+  // |           | \/ ||_ |_ |_  _|| ||_
+  // |           ||\/|||_  _| _||_||_||_  _ _ _ _ baseline
+  // |                              _|
+  //
+  static const int kTitleTopPadding = 15;
+  static const int kVerticalPadding = 20;
+  static const int kHorizontalPadding = 20;
+  static const int kRowPadding = 15;

[Peter Kasting, 2012/10/09 19:31:10]

How can these values be guaranteed to be correct across platforms and fonts?  It
seems like we'd have to calculate these dynamically to ensure the sizings you
want.

[please use gerrit instead, 2012/10/10 19:12:58]

The author of the mocks (kenmoore@) has asked us to use regular padding as we
have done before and ignore the ascents, descents, x-heights, and baselines. If
fonts change in the future or vary across platforms such that x-height becoms
19px instead of 20px, it is not a disaster.

+
+  // Border radius for dialog corners.
+  static const int kBorderRadius = 2;
+
+  // This is the padding to use on the top and right sides of the "x" close
+  // button in order for the button to appear 10px from the top border and 10px
+  // from the right border. This padding is 4px because the image for the "x"
+  // close button has an invisible 6px border.

[Peter Kasting, 2012/10/09 19:31:10]

Is this border so the button will have a larger hit area?  Because if not it'd
be clearer if we could eliminate the border somehow.

[please use gerrit instead, 2012/10/10 19:12:58]

The border is so the button has a larger hit area. Kenmoore@ says that the
larger button was originally motivated by touch-friendliness. The UX folks have
since changed their minds and have provided us with new (smaller) x button,
which I am checking in as part of this CL now.

+  //
+  // _______________________________________________
+  //                            ^                   |
+  //                           4px                  |
+  //                            v                   |
+  //                   ___________________          |
+  //                  |         ^         |         |
+  //                  |        6px        |         |
+  //                  |         v         |         |
+  //                  |         X < 6px > | < 4px > |
+  //                  |                   |         |
+  //                  |                   |         |
+  //                  |___________________|         |
+  //                                                |
+  //
+  static const int kCloseButtonPadding = 4;
 
   // Font style for dialog text.
   static const ui::ResourceBundle::FontStyle kTextFontStyle =
       ui::ResourceBundle::BaseFont;
   // Font style for bold dialog text.
   static const ui::ResourceBundle::FontStyle kBoldTextFontStyle =
       ui::ResourceBundle::BoldFont;
   // Font style for dialog title.
   static const ui::ResourceBundle::FontStyle kTitleFontStyle =
       ui::ResourceBundle::MediumFont;
@@ skipping 21 matching lines @@
   virtual bool CanShowConstrainedWindow();
 
   // Returns the native window of the constrained window.
   virtual gfx::NativeWindow GetNativeWindow();
 
  protected:
   virtual ~ConstrainedWindow() {}
 };
 
 #endif  // CHROME_BROWSER_UI_CONSTRAINED_WINDOW_H_