Fix focus rectangle for label.

ui/views/controls/label.cc

Index: ui/views/controls/label.cc
diff --git a/ui/views/controls/label.cc b/ui/views/controls/label.cc
index 06f5bac122010995e116357b9a3e639cad66e830..6ad5c70a6e7cfa710f907976e2b9241f378fc586 100644
--- a/ui/views/controls/label.cc
+++ b/ui/views/controls/label.cc
@@ -327,10 +327,22 @@ void Label::OnEnabledChanged() {
   RecalculateColors();
 }
 
-void Label::PaintText(gfx::Canvas* canvas) {
+gfx::Rect Label::PaintText(gfx::Canvas* canvas) {
   MaybeBuildRenderTextLines();
-  for (size_t i = 0; i < lines_.size(); ++i)
+  gfx::Rect focus_bounds;
+  for (size_t i = 0; i < lines_.size(); ++i) {
+    gfx::Point origin;
+    origin += lines_[i]->GetLineOffset(0);
+    focus_bounds.Union(gfx::Rect(origin, lines_[i]->GetStringSize()));
     lines_[i]->Draw(canvas);
+  }
+
+  if (HasFocus()) {

[sky, 2015/03/16 21:29:57]

What if the label has focus, but is empty? What will focus_bounds be?

[Jun Mukai, 2015/03/16 22:05:38]

Good point, I believe a thin rectangle (line-height with no width or 1px width)
should be drawn. Added the handler code and a test case.

+    focus_bounds.Inset(-kFocusBorderPadding, -kFocusBorderPadding);

[sky, 2015/03/16 21:29:57]

What if kFocusBorderPadding is > current size?

[Jun Mukai, 2015/03/16 22:05:37]

Fixed to be capped inside of the size.

+    canvas->DrawFocusRect(focus_bounds);
+    return focus_bounds;
+  }
+  return gfx::Rect();

[sky, 2015/03/16 21:29:57]

Is there a reason not to always return focus_rect? And why is this returning the
focus rect in the first place? If you say so you can get the focus rect can we
move that into a separate function?

[Jun Mukai, 2015/03/16 22:05:37]

Changed to keep the focus bounds as a field |focus_bounds_| rather than
returning here.
The motivation was not to keep an additional field just for tests though.

[sky, 2015/03/16 22:56:09]

Why use a field at all for this? The focus rect is only needed if the label has
focus. I prefer a standalone function called from PaintText that calculates it.
Your test can call the same function.

[Jun Mukai, 2015/03/17 00:03:47]

I see. separated as GetFocusBounds().

 }
 
 void Label::OnBoundsChanged(const gfx::Rect& previous_bounds) {
@@ -354,11 +366,6 @@ void Label::OnPaint(gfx::Canvas* canvas) {
 
     PaintText(canvas);
   }
-  if (HasFocus()) {
-    gfx::Rect focus_bounds = GetLocalBounds();
-    focus_bounds.Inset(-kFocusBorderPadding, -kFocusBorderPadding);
-    canvas->DrawFocusRect(focus_bounds);
-  }
 }
 
 void Label::OnNativeThemeChanged(const ui::NativeTheme* theme) {
@@ -433,6 +440,8 @@ void Label::MaybeBuildRenderTextLines() {
     return;
 
   gfx::Rect rect = GetContentsBounds();
+  if (focusable())
+    rect.Inset(kFocusBorderPadding, kFocusBorderPadding);
   if (rect.IsEmpty())
     return;