Fix focus rectangle for label.

The current focus rectangle logic is wrong -- it's always drawn
outside of local bounds, which is not drawn actually. The expected
behavior is to draw the focus rectangle around the actual text.

BUG=467510
R=sky@chromium.org
TEST=the new test case covers

Review URL: https://codereview.chromium.org/1018463004

Cr-Commit-Position: refs/heads/master@{#320943}

ui/gfx/render_text.cc

@@ -939,6 +939,17 @@ void RenderText::SetDisplayOffset(int horizontal_offset) {
   cursor_bounds_ = GetCursorBounds(selection_model_, insert_mode_);
 }
 
+Vector2d RenderText::GetLineOffset(size_t line_number) {
+  Vector2d offset = display_rect().OffsetFromOrigin();
+  // TODO(ckocagil): Apply the display offset for multiline scrolling.
+  if (!multiline())
+    offset.Add(GetUpdatedDisplayOffset());
+  else
+    offset.Add(Vector2d(0, lines_[line_number].preceding_heights));
+  offset.Add(GetAlignmentOffset(line_number));
+  return offset;
+}
+
 RenderText::RenderText()
     : horizontal_alignment_(base::i18n::IsRTL() ? ALIGN_RIGHT : ALIGN_LEFT),
       directionality_mode_(DIRECTIONALITY_FROM_TEXT),
@@ -1067,17 +1078,6 @@ void RenderText::UndoCompositionAndSelectionStyles() {
   composition_and_selection_styles_applied_ = false;
 }
 
-Vector2d RenderText::GetLineOffset(size_t line_number) {
-  Vector2d offset = display_rect().OffsetFromOrigin();
-  // TODO(ckocagil): Apply the display offset for multiline scrolling.
-  if (!multiline())
-    offset.Add(GetUpdatedDisplayOffset());
-  else
-    offset.Add(Vector2d(0, lines_[line_number].preceding_heights));
-  offset.Add(GetAlignmentOffset(line_number));
-  return offset;
-}
-
 Point RenderText::ToTextPoint(const Point& point) {
   return point - GetLineOffset(0);
   // TODO(ckocagil): Convert multiline view space points to text space.

ui/gfx/render_text.h

@@ -461,6 +461,10 @@ class GFX_EXPORT RenderText {
   const Vector2d& GetUpdatedDisplayOffset();
   void SetDisplayOffset(int horizontal_offset);
 
+  // Returns the line offset from the origin after applying the text alignment
+  // and the display offset.
+  Vector2d GetLineOffset(size_t line_number);
+
  protected:
   RenderText();
 
@@ -569,10 +573,6 @@ class GFX_EXPORT RenderText {
   void ApplyCompositionAndSelectionStyles();
   void UndoCompositionAndSelectionStyles();
 
-  // Returns the line offset from the origin after applying the text alignment
-  // and the display offset.
-  Vector2d GetLineOffset(size_t line_number);
-
   // Convert points from the text space to the view space and back. Handles the
   // display area, display offset, application LTR/RTL mode and multiline.
   Point ToTextPoint(const Point& point);

ui/views/controls/label.cc

@@ -354,11 +354,8 @@ void Label::OnPaint(gfx::Canvas* canvas) {
 
     PaintText(canvas);
   }
-  if (HasFocus()) {
-    gfx::Rect focus_bounds = GetLocalBounds();
-    focus_bounds.Inset(-kFocusBorderPadding, -kFocusBorderPadding);
-    canvas->DrawFocusRect(focus_bounds);
-  }
+  if (HasFocus())
+    canvas->DrawFocusRect(GetFocusBounds());
 }
 
 void Label::OnNativeThemeChanged(const ui::NativeTheme* theme) {
@@ -433,6 +430,8 @@ void Label::MaybeBuildRenderTextLines() {
     return;
 
   gfx::Rect rect = GetContentsBounds();
+  if (focusable())
+    rect.Inset(kFocusBorderPadding, kFocusBorderPadding);
   if (rect.IsEmpty())
     return;
 
@@ -479,6 +478,25 @@ void Label::MaybeBuildRenderTextLines() {
   RecalculateColors();
 }
 
+gfx::Rect Label::GetFocusBounds() {
+  MaybeBuildRenderTextLines();
+
+  gfx::Rect focus_bounds;
+  if (lines_.empty()) {
+    focus_bounds = gfx::Rect(GetTextSize());
+  } else {
+    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()));
+    }
+  }
+
+  focus_bounds.Inset(-kFocusBorderPadding, -kFocusBorderPadding);
+  focus_bounds.Intersect(GetLocalBounds());
+  return focus_bounds;
+}
+
 std::vector<base::string16> Label::GetLinesForWidth(int width) const {
   std::vector<base::string16> lines;
   const gfx::WordWrapBehavior wrap =

ui/views/controls/label.h

@@ -150,6 +150,8 @@ class VIEWS_EXPORT Label : public View {
   FRIEND_TEST_ALL_PREFIXES(LabelTest, ResetRenderTextData);
   FRIEND_TEST_ALL_PREFIXES(LabelTest, MultilineSupportedRenderText);
   FRIEND_TEST_ALL_PREFIXES(LabelTest, TextChangeWithoutLayout);
+  FRIEND_TEST_ALL_PREFIXES(LabelFocusTest, FocusBounds);
+  FRIEND_TEST_ALL_PREFIXES(LabelFocusTest, EmptyLabel);
 
   void Init(const base::string16& text, const gfx::FontList& font_list);
 
@@ -165,6 +167,8 @@ class VIEWS_EXPORT Label : public View {
   // Set up |lines_| to actually be painted.
   void MaybeBuildRenderTextLines();
 
+  gfx::Rect GetFocusBounds();
+
   // Get the text broken into lines as needed to fit the given |width|.
   std::vector<base::string16> GetLinesForWidth(int width) const;
 

ui/views/controls/label_unittest.cc

@@ -11,6 +11,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/canvas.h"
 #include "ui/views/border.h"
+#include "ui/views/test/focus_manager_test.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/widget/widget.h"
 
@@ -20,6 +21,24 @@ namespace views {
 
 typedef ViewsTestBase LabelTest;
 
+class LabelFocusTest : public FocusManagerTest {
+ public:
+  LabelFocusTest() {}
+  ~LabelFocusTest() override {}
+
+ protected:
+  views::Label* label() { return label_; }
+
+ private:
+  // FocusManagerTest:
+  void InitContentView() override {
+    label_ = new views::Label();
+    GetContentsView()->AddChildView(label_);
+  }
+
+  views::Label* label_;
+};
+
 // All text sizing measurements (width and height) should be greater than this.
 const int kMinTextDimension = 4;
 
@@ -548,4 +567,54 @@ TEST_F(LabelTest, MultilineSupportedRenderText) {
 }
 #endif
 
+TEST_F(LabelFocusTest, FocusBounds) {
+  label()->SetText(ASCIIToUTF16("Example"));
+  gfx::Size normal_size = label()->GetPreferredSize();
+
+  label()->SetFocusable(true);
+  label()->RequestFocus();
+  gfx::Size focusable_size = label()->GetPreferredSize();
+  // Focusable label requires larger size to paint the focus rectangle.
+  EXPECT_GT(focusable_size.width(), normal_size.width());
+  EXPECT_GT(focusable_size.height(), normal_size.height());
+
+  label()->SizeToPreferredSize();
+  gfx::Rect focus_bounds = label()->GetFocusBounds();
+  EXPECT_EQ(label()->GetLocalBounds().ToString(), focus_bounds.ToString());
+
+  label()->SetBounds(
+      0, 0, focusable_size.width() * 2, focusable_size.height() * 2);
+  label()->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  focus_bounds = label()->GetFocusBounds();
+  EXPECT_EQ(0, focus_bounds.x());
+  EXPECT_LT(0, focus_bounds.y());
+  EXPECT_GT(label()->bounds().bottom(), focus_bounds.bottom());
+  EXPECT_EQ(focusable_size.ToString(), focus_bounds.size().ToString());
+
+  label()->SetHorizontalAlignment(gfx::ALIGN_RIGHT);
+  focus_bounds = label()->GetFocusBounds();
+  EXPECT_LT(0, focus_bounds.x());
+  EXPECT_EQ(label()->bounds().right(), focus_bounds.right());
+  EXPECT_LT(0, focus_bounds.y());
+  EXPECT_GT(label()->bounds().bottom(), focus_bounds.bottom());
+  EXPECT_EQ(focusable_size.ToString(), focus_bounds.size().ToString());
+
+  label()->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  label()->SetElideBehavior(gfx::FADE_TAIL);
+  label()->SetBounds(0, 0, focusable_size.width() / 2, focusable_size.height());
+  focus_bounds = label()->GetFocusBounds();
+  EXPECT_EQ(0, focus_bounds.x());
+  EXPECT_EQ(focusable_size.width() / 2, focus_bounds.width());
+}
+
+TEST_F(LabelFocusTest, EmptyLabel) {
+  label()->SetFocusable(true);
+  label()->RequestFocus();
+  label()->SizeToPreferredSize();
+
+  gfx::Rect focus_bounds = label()->GetFocusBounds();
+  EXPECT_FALSE(focus_bounds.IsEmpty());
+  EXPECT_LT(label()->font_list().GetHeight(), focus_bounds.height());
+}
+
 }  // namespace views