Fix focus rects for checkboxes and radio buttons:

- add concept of default insets to view which get added to any other insets provided by the user. used by label to provide room for a focus border.
- provide the ability for the label to paint its focus border even if it isn't focused. needed because the outer container (the checkbox) gets focus but the inner label does not, however the label knows best the location of its text around which the focus border must be drawn.

please note:
- also make it easier to click checkboxes by not resetting mouse_pressed_handler_ in RootView when a view decides it doesn't want to handle a drag. this is so we can still receive mousereleased notifications when the mouse is released... it's "difficult" to click checkboxes and radio buttons when you accidentally drag a little on their label. (this is the root view change).

- fix slight alignment issue on the general page of options.


Also fix a slight error in my last radio checkbox - clicking on a checked radio button should still focus it.

http://crbug.com/10834

TEST=visit options, Minor Tweaks. click the "always ask before downloading" checkbox and observe that the focus rect tightly surrounds the text label instead of stretching to the right side of the dialog.
Visit options, click an already-checked radio button. observe that it takes focus.
Visit options, click on any checkbox or radio button, drag slightly then release (still within the bounds of the item). note the item is now toggled or selected. click down then drag out and release, note that it is not toggled or selected.

Review URL: http://codereview.chromium.org/92004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@14265 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 80 insertions, 46 deletions

diff --git a/chrome/views/controls/button/checkbox.cc b/chrome/views/controls/button/checkbox.cc
index ae523ed..ae023ce 100644
--- a/chrome/views/controls/button/checkbox.cc
+++ b/chrome/views/controls/button/checkbox.cc
@@ -82,17 +82,8 @@ void Checkbox::Layout() {
   native_wrapper_->GetView()->Layout();
 }
 
-void Checkbox::Paint(ChromeCanvas* canvas) {
-  // Paint the focus border manually since we don't want to send actual focus
-  // in to the inner view.
-  if (HasFocus()) {
-    gfx::Rect label_bounds = label_->bounds();
-    canvas->DrawFocusRect(
-        MirroredLeftPointForRect(label_bounds) - kLabelFocusPaddingHorizontal,
-        0,
-        label_bounds.width() + kLabelFocusPaddingHorizontal * 2,
-        label_bounds.height() - kLabelFocusPaddingVertical * 2);
-  }
+void Checkbox::PaintFocusBorder(ChromeCanvas* canvas) {
+  // Our focus border is rendered by the label, so we don't do anything here.
 }
 
 View* Checkbox::GetViewForPoint(const gfx::Point& point) {
@@ -129,6 +120,18 @@ void Checkbox::OnMouseReleased(const MouseEvent& e, bool canceled) {
   }
 }
 
+bool Checkbox::OnMouseDragged(const MouseEvent& e) {
+  return false;
+}
+
+void Checkbox::WillGainFocus() {
+  label_->set_paint_as_focused(true);
+}
+
+void Checkbox::WillLoseFocus() {
+  label_->set_paint_as_focused(false);
+}
+
 std::string Checkbox::GetClassName() const {
   return kViewClassName;
 }
@@ -147,6 +150,15 @@ void Checkbox::InitBorder() {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
+// Checkbox, protected:
+
+bool Checkbox::HitTestLabel(const MouseEvent& e) {
+  gfx::Point tmp(e.location());
+  ConvertPointToView(this, label_, &tmp);
+  return label_->HitTest(tmp);
+}
+
+////////////////////////////////////////////////////////////////////////////////
 // Checkbox, private:
 
 void Checkbox::Init(const std::wstring& label_text) {
@@ -156,10 +168,4 @@ void Checkbox::Init(const std::wstring& label_text) {
   AddChildView(label_);
 }
 
-bool Checkbox::HitTestLabel(const MouseEvent& e) {
-  gfx::Point tmp(e.location());
-  ConvertPointToView(this, label_, &tmp);
-  return label_->HitTest(tmp);
-}
-
 }  // namespace views
diff --git a/chrome/views/controls/button/checkbox.h b/chrome/views/controls/button/checkbox.h
index b9def2c..54f4e06 100644
--- a/chrome/views/controls/button/checkbox.h
+++ b/chrome/views/controls/button/checkbox.h
@@ -41,7 +41,7 @@ class Checkbox : public NativeButton {
   // Overridden from View:
   virtual gfx::Size GetPreferredSize();
   virtual void Layout();
-  virtual void Paint(ChromeCanvas* canvas);
+  virtual void PaintFocusBorder(ChromeCanvas* canvas);
   virtual View* GetViewForPoint(const gfx::Point& point);
   virtual View* GetViewForPoint(const gfx::Point& point,
                                 bool can_create_floating);
@@ -50,6 +50,9 @@ class Checkbox : public NativeButton {
   virtual void OnMouseExited(const MouseEvent& e);
   virtual bool OnMousePressed(const MouseEvent& e);
   virtual void OnMouseReleased(const MouseEvent& e, bool canceled);
+  virtual bool OnMouseDragged(const MouseEvent& e);
+  virtual void WillGainFocus();
+  virtual void WillLoseFocus();
 
  protected:
   virtual std::string GetClassName() const;
@@ -58,14 +61,14 @@ class Checkbox : public NativeButton {
   virtual void CreateWrapper();
   virtual void InitBorder();
 
- private:
-  // Called from the constructor to create and configure the checkbox label.
-  void Init(const std::wstring& label_text);
-
   // Returns true if the event (in Checkbox coordinates) is within the bounds of
   // the label.
   bool HitTestLabel(const MouseEvent& e);
 
+ private:
+  // Called from the constructor to create and configure the checkbox label.
+  void Init(const std::wstring& label_text);
+
   // The checkbox's label. We don't use the OS version because of transparency
   // and sizing issues.
   Label* label_;
diff --git a/chrome/views/controls/button/radio_button.cc b/chrome/views/controls/button/radio_button.cc
index b8b870cb..fa18a5d 100644
--- a/chrome/views/controls/button/radio_button.cc
+++ b/chrome/views/controls/button/radio_button.cc
@@ -81,13 +81,14 @@ bool RadioButton::IsGroupFocusTraversable() const {
   return false;
 }
 
-void RadioButton::OnMouseReleased(const views::MouseEvent& event,
-                                  bool canceled) {
+void RadioButton::OnMouseReleased(const MouseEvent& event, bool canceled) {
   native_wrapper_->SetPushed(false);
-  // Call through to toggle the button only if we're not already checked, since
-  // radio buttons can't be toggled like checkboxes.
-  if (!checked())
-    Checkbox::OnMouseReleased(event, canceled);
+  // Set the checked state to true only if we are unchecked, since we can't
+  // be toggled on and off like a checkbox.
+  if (!checked() && !canceled && HitTestLabel(event))
+    SetChecked(true);
+
+  ButtonPressed();
 }
 
 std::string RadioButton::GetClassName() const {
diff --git a/chrome/views/controls/button/radio_button.h b/chrome/views/controls/button/radio_button.h
index 1e07bae..0fe6d687 100644
--- a/chrome/views/controls/button/radio_button.h
+++ b/chrome/views/controls/button/radio_button.h
@@ -26,7 +26,7 @@ class RadioButton : public Checkbox {
   // Overridden from View:
   virtual View* GetSelectedViewForGroup(int group_id);
   virtual bool IsGroupFocusTraversable() const;
-  virtual void OnMouseReleased(const views::MouseEvent& event, bool canceled);
+  virtual void OnMouseReleased(const MouseEvent& event, bool canceled);
 
  protected:
   virtual std::string GetClassName() const;
diff --git a/chrome/views/controls/label.cc b/chrome/views/controls/label.cc
index 779cf2b..1f499b2 100644
--- a/chrome/views/controls/label.cc
+++ b/chrome/views/controls/label.cc
@@ -22,6 +22,7 @@ const char Label::kViewClassName[] = "chrome/views/Label";
 
 static const SkColor kEnabledColor = SK_ColorBLACK;
 static const SkColor kDisabledColor = SkColorSetRGB(161, 161, 146);
+static const int kFocusBorderPadding = 1;
 
 Label::Label() {
   Init(L"", GetDefaultFont());
@@ -46,6 +47,7 @@ void Label::Init(const std::wstring& text, const ChromeFont& font) {
   is_multi_line_ = false;
   collapse_when_hidden_ = false;
   rtl_alignment_mode_ = USE_UI_ALIGNMENT;
+  paint_as_focused_ = false;
 }
 
 Label::~Label() {
@@ -143,15 +145,21 @@ void Label::Paint(ChromeCanvas* canvas) {
                         text_bounds.height(),
                         flags);
 
-  if (is_multi_line_) {
-    PaintFocusBorder(canvas);
-  } else {
-    // We'll draw the focus border ourselves, so it is around the text.
-    if (HasFocus())
-      canvas->DrawFocusRect(text_bounds.x(),
-                            text_bounds.y(),
-                            text_bounds.width(),
-                            text_bounds.height());
+  // The focus border always hugs the text, regardless of the label's bounds.
+  if (HasFocus() || paint_as_focused_) {
+    int w = text_bounds.width();
+    int h = 0;
+    // We explicitly OR in MULTI_LINE here since SizeStringInt seems to return
+    // an incorrect height for single line text when the MULTI_LINE flag isn't
+    // specified. o_O...
+    ChromeCanvas::SizeStringInt(paint_text, font_, &w, &h,
+                                flags | ChromeCanvas::MULTI_LINE);
+    gfx::Rect focus_rect = text_bounds;
+    focus_rect.set_width(w);
+    focus_rect.set_height(h);
+    focus_rect.Inset(-kFocusBorderPadding, -kFocusBorderPadding);
+    canvas->DrawFocusRect(focus_rect.x(), focus_rect.y(), focus_rect.width(),
+                          focus_rect.height());
   }
 }
 
@@ -325,6 +333,15 @@ void Label::SetEnabled(bool enabled) {
   SetColor(enabled ? kEnabledColor : kDisabledColor);
 }
 
+gfx::Insets Label::GetInsets() const {
+  gfx::Insets insets = View::GetInsets();
+  if (IsFocusable() || paint_as_focused_)  {
+    insets += gfx::Insets(kFocusBorderPadding, kFocusBorderPadding,
+                          kFocusBorderPadding, kFocusBorderPadding);
+  }
+  return insets;
+}
+
 // static
 ChromeFont Label::GetDefaultFont() {
   return ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont);
diff --git a/chrome/views/controls/label.h b/chrome/views/controls/label.h
index ae8f618..11608fa 100644
--- a/chrome/views/controls/label.h
+++ b/chrome/views/controls/label.h
@@ -153,6 +153,9 @@ class Label : public View {
   // Sets the enabled state. Setting the enabled state resets the color.
   virtual void SetEnabled(bool enabled);
 
+  // Overridden from View:
+  virtual gfx::Insets GetInsets() const;
+
   // Resizes the label so its width is set to the width of the longest line and
   // its height deduced accordingly.
   // This is only intended for multi-line labels and is useful when the label's
@@ -184,6 +187,10 @@ class Label : public View {
   void set_collapse_when_hidden(bool value) { collapse_when_hidden_ = value; }
   bool collapse_when_hidden() const { return collapse_when_hidden_; }
 
+  void set_paint_as_focused(bool paint_as_focused) {
+    paint_as_focused_ = paint_as_focused;
+  }
+
  private:
   // These tests call CalculateDrawStringParams in order to verify the
   // calculations done for drawing text.
@@ -231,6 +238,9 @@ class Label : public View {
   // needs to be flipped around for RTL locales. Please refer to the definition
   // of RTLAlignmentMode for more information.
   RTLAlignmentMode rtl_alignment_mode_;
+  // When embedded in a larger control that is focusable, setting this flag
+  // allows this view to be painted as focused even when it is itself not.
+  bool paint_as_focused_;
 
   DISALLOW_COPY_AND_ASSIGN(Label);
 };
diff --git a/chrome/views/controls/label_unittest.cc b/chrome/views/controls/label_unittest.cc
index ba00f69..58d6655 100644
--- a/chrome/views/controls/label_unittest.cc
+++ b/chrome/views/controls/label_unittest.cc
@@ -193,6 +193,7 @@ TEST(LabelTest, SingleLineSizing) {
 
 TEST(LabelTest, MultiLineSizing) {
   Label label;
+  label.SetFocusable(false);
   std::wstring test_text(L"A random string\nwith multiple lines\nand returns!");
   label.SetText(test_text);
   label.SetMultiLine(true);
@@ -263,6 +264,7 @@ TEST(LabelTest, MultiLineSizing) {
 
 TEST(LabelTest, DrawSingleLineString) {
   Label label;
+  label.SetFocusable(false);
 
   // Turn off mirroring so that we don't need to figure out if
   // align right really means align left.
@@ -377,6 +379,7 @@ TEST(LabelTest, DrawSingleLineString) {
 
 TEST(LabelTest, DrawMultiLineString) {
   Label label;
+  label.SetFocusable(false);
 
   // Turn off mirroring so that we don't need to figure out if
   // align right really means align left.
diff --git a/chrome/views/view.h b/chrome/views/view.h
index 8bedfe5a..f88ad7e 100644
--- a/chrome/views/view.h
+++ b/chrome/views/view.h
@@ -898,7 +898,7 @@ class View : public AcceleratorTarget {
 
   // Returns the insets of the current border. If there is no border an empty
   // insets is returned.
-  gfx::Insets GetInsets() const;
+  virtual gfx::Insets GetInsets() const;
 
 #if defined(OS_WIN)
   // TODO(port): Make GetCursorForPoint portable.
diff --git a/chrome/views/widget/root_view.cc b/chrome/views/widget/root_view.cc
index eed1730..6158a10 100644
--- a/chrome/views/widget/root_view.cc
+++ b/chrome/views/widget/root_view.cc
@@ -402,13 +402,7 @@ bool RootView::OnMouseDragged(const MouseEvent& e) {
     gfx::Point p;
     ConvertPointToMouseHandler(e.location(), &p);
     MouseEvent mouse_event(e.GetType(), p.x(), p.y(), e.GetFlags());
-    if (!mouse_pressed_handler_->ProcessMouseDragged(mouse_event,
-                                                     &drag_info)) {
-      mouse_pressed_handler_ = NULL;
-      return false;
-    } else {
-      return true;
-    }
+    return mouse_pressed_handler_->ProcessMouseDragged(mouse_event, &drag_info);
   }
   return false;
 }