Fix focus rectangle for label.

ui/views/controls/label.cc

 // 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.
 
 #include "ui/views/controls/label.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <vector>
@@ skipping 309 matching lines @@
     return true;
   }
 
   return false;
 }
 
 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) {
   if (previous_bounds.size() != size())
     InvalidateLayout();
 }
 
 void Label::OnPaint(gfx::Canvas* canvas) {
   View::OnPaint(canvas);
   if (is_first_paint_text_) {
     // TODO(vadimt): Remove ScopedTracker below once crbug.com/431326 is fixed.
     tracked_objects::ScopedTracker tracking_profile(
         FROM_HERE_WITH_EXPLICIT_FUNCTION("431326 Label::PaintText first"));
 
     is_first_paint_text_ = false;
     PaintText(canvas);
   } else {
     // TODO(vadimt): Remove ScopedTracker below once crbug.com/431326 is fixed.
     tracked_objects::ScopedTracker tracking_profile(
         FROM_HERE_WITH_EXPLICIT_FUNCTION("431326 Label::PaintText not first"));
 
     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) {
   UpdateColorsFromTheme(theme);
 }
 
 void Label::OnDeviceScaleFactorChanged(float device_scale_factor) {
   View::OnDeviceScaleFactorChanged(device_scale_factor);
   // When the device scale factor is changed, some font rendering parameters is
   // changed (especially, hinting). The bounding box of the text has to be
@@ skipping 54 matching lines @@
   render_text->SetCursorEnabled(false);
   render_text->SetText(text);
   return render_text.Pass();
 }
 
 void Label::MaybeBuildRenderTextLines() {
   if (!lines_.empty())
     return;
 
   gfx::Rect rect = GetContentsBounds();
+  if (focusable())
+    rect.Inset(kFocusBorderPadding, kFocusBorderPadding);
   if (rect.IsEmpty())
     return;
 
   gfx::HorizontalAlignment alignment = horizontal_alignment();
   gfx::DirectionalityMode directionality = render_text_->directionality_mode();
   if (multi_line()) {
     // Force the directionality and alignment of the first line on other lines.
     bool rtl =
         render_text_->GetDisplayTextDirection() == base::i18n::RIGHT_TO_LEFT;
     if (alignment == gfx::ALIGN_TO_HEAD)
@@ skipping 108 matching lines @@
 }
 
 bool Label::ShouldShowDefaultTooltip() const {
   const gfx::Size text_size = GetTextSize();
   const gfx::Size size = GetContentsBounds().size();
   return !obscured() && (text_size.width() > size.width() ||
                          (multi_line() && text_size.height() > size.height()));
 }
 
 }  // namespace views