This CL fixes issue 41985 - The focus frames are out of range on the Options : RTL.

Previously, Label::CalculateDrawStringParams() computes the text boundary for signle-line and multi-line texts differently, so the boundary of focus rectangle is computed differently too inside Label::Paint().

Now, Label::CalculateDrawStringParams() computes text boundary without differentiate single-line and multi-line text, the boundary itself always takes mirror into consideration. So, the boundary of focus rectangle no longer need mirroring for both cases.


BUG=http://crbug.com/41985
TEST=
1. Launch Chrome with RTL languages UI (ex: chrome.exe --lang=he)
2. click Wrench => Options Under the Hood 
3. click or press tab to focus any string there.
4. the focus should be around the text, not out of the text range.

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

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

3 files changed, 329 insertions, 14 deletions

diff --git a/views/controls/label.cc b/views/controls/label.cc
index 861fc46..a9d74ac 100644
--- a/views/controls/label.cc
+++ b/views/controls/label.cc
@@ -88,14 +88,6 @@ void Label::Paint(gfx::Canvas* canvas) {
 
   if (HasFocus() || paint_as_focused_) {
     text_bounds.Inset(-kFocusBorderPadding, -kFocusBorderPadding);
-    // If the label is a single line of text, then the computed text bound
-    // corresponds directly to the text being drawn and no mirroring is needed
-    // for the RTL case. For multiline text, the text bound is an estimation
-    // and is recomputed in gfx::Canvas::SizeStringInt(). For multiline text
-    // in RTL, we need to take mirroring into account when computing the focus
-    // rectangle.
-    if (flags & gfx::Canvas::MULTI_LINE)
-      text_bounds.set_x(MirroredLeftPointForRect(text_bounds));
     canvas->DrawFocusRect(text_bounds.x(), text_bounds.y(),
                           text_bounds.width(), text_bounds.height());
   }
diff --git a/views/controls/label.h b/views/controls/label.h
index e68593be..60ccb69 100644
--- a/views/controls/label.h
+++ b/views/controls/label.h
@@ -192,6 +192,8 @@ class Label : public View {
   // calculations done for drawing text.
   FRIEND_TEST(LabelTest, DrawSingleLineString);
   FRIEND_TEST(LabelTest, DrawMultiLineString);
+  FRIEND_TEST(LabelTest, DrawSingleLineStringInRTL);
+  FRIEND_TEST(LabelTest, DrawMultiLineStringInRTL);
 
   static gfx::Font GetDefaultFont();
 
diff --git a/views/controls/label_unittest.cc b/views/controls/label_unittest.cc
index 2766643..19751ca 100644
--- a/views/controls/label_unittest.cc
+++ b/views/controls/label_unittest.cc
@@ -411,6 +411,11 @@ TEST(LabelTest, DrawMultiLineString) {
   label.SetText(test_text);
   label.SetMultiLine(true);
   label.SizeToFit(0);
+  gfx::Size extra(50, 10);
+  label.SetBounds(label.x(),
+                  label.y(),
+                  label.width() + extra.width(),
+                  label.height() + extra.height());
 
   // Do some basic verifications for all three alignments.
   std::wstring paint_text;
@@ -418,8 +423,8 @@ TEST(LabelTest, DrawMultiLineString) {
   int flags;
   label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
   EXPECT_EQ(test_text, paint_text);
-  EXPECT_EQ(0, text_bounds.x());
-  EXPECT_EQ(0, text_bounds.y());
+  EXPECT_EQ(extra.width() / 2, text_bounds.x());
+  EXPECT_EQ(extra.height() / 2, text_bounds.y());
   EXPECT_GT(text_bounds.width(), kMinTextDimension);
   EXPECT_GT(text_bounds.height(), kMinTextDimension);
 #if defined(OS_WIN)
@@ -439,7 +444,7 @@ TEST(LabelTest, DrawMultiLineString) {
   label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
   EXPECT_EQ(test_text, paint_text);
   EXPECT_EQ(0, text_bounds.x());
-  EXPECT_EQ(0, text_bounds.y());
+  EXPECT_EQ(extra.height() / 2, text_bounds.y());
   EXPECT_GT(text_bounds.width(), kMinTextDimension);
   EXPECT_GT(text_bounds.height(), kMinTextDimension);
 #if defined(OS_WIN)
@@ -457,8 +462,259 @@ TEST(LabelTest, DrawMultiLineString) {
   text_bounds.SetRect(0, 0, 0, 0);
   label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
   EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(extra.width(), text_bounds.x());
+  EXPECT_EQ(extra.height() / 2, text_bounds.y());
+  EXPECT_GT(text_bounds.width(), kMinTextDimension);
+  EXPECT_GT(text_bounds.height(), kMinTextDimension);
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_RIGHT, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_RIGHT |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+
+  // Test multiline drawing with a border.
+  gfx::Insets border(19, 92, 23, 2);
+  label.set_border(Border::CreateEmptyBorder(border.top(),
+                                             border.left(),
+                                             border.bottom(),
+                                             border.right()));
+  label.SizeToFit(0);
+  label.SetBounds(label.x(),
+                  label.y(),
+                  label.width() + extra.width(),
+                  label.height() + extra.height());
+
+  label.SetHorizontalAlignment(Label::ALIGN_CENTER);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(border.left() + extra.width() / 2, text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2, text_bounds.y());
+  EXPECT_EQ(center_bounds.width(), text_bounds.width());
+  EXPECT_EQ(center_bounds.height(), text_bounds.height());
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_CENTER, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_CENTER |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+
+  label.SetHorizontalAlignment(Label::ALIGN_LEFT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(border.left(), text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2, text_bounds.y());
+  EXPECT_EQ(center_bounds.width(), text_bounds.width());
+  EXPECT_EQ(center_bounds.height(), text_bounds.height());
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_LEFT, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_LEFT |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+
+  label.SetHorizontalAlignment(Label::ALIGN_RIGHT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(extra.width() + border.left(), text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2, text_bounds.y());
+  EXPECT_EQ(center_bounds.width(), text_bounds.width());
+  EXPECT_EQ(center_bounds.height(), text_bounds.height());
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_RIGHT, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_RIGHT |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+}
+
+TEST(LabelTest, DrawSingleLineStringInRTL) {
+  Label label;
+  label.SetFocusable(false);
+
+  label.EnableUIMirroringForRTLLanguages(true);
+  std::string locale = l10n_util::GetApplicationLocale(std::wstring());
+  base::i18n::SetICUDefaultLocale("he");
+
+  std::wstring test_text(L"Here's a string with no returns.");
+  label.SetText(test_text);
+  gfx::Size required_size(label.GetPreferredSize());
+  gfx::Size extra(22, 8);
+  label.SetBounds(0,
+                  0,
+                  required_size.width() + extra.width(),
+                  required_size.height() + extra.height());
+
+  // Do some basic verifications for all three alignments.
+  std::wstring paint_text;
+  gfx::Rect text_bounds;
+  int flags;
+
+  // Centered text.
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  // The text should be centered horizontally and vertically.
+  EXPECT_EQ(extra.width() / 2, text_bounds.x());
+  EXPECT_EQ(extra.height() / 2 , text_bounds.y());
+  EXPECT_EQ(required_size.width(), text_bounds.width());
+  EXPECT_EQ(required_size.height(), text_bounds.height());
+  EXPECT_EQ(0, flags);
+
+  // ALIGN_LEFT label.
+  label.SetHorizontalAlignment(Label::ALIGN_LEFT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  // The text should be right aligned horizontally and centered vertically.
+  EXPECT_EQ(extra.width(), text_bounds.x());
+  EXPECT_EQ(extra.height() / 2 , text_bounds.y());
+  EXPECT_EQ(required_size.width(), text_bounds.width());
+  EXPECT_EQ(required_size.height(), text_bounds.height());
+  EXPECT_EQ(0, flags);
+
+  // ALIGN_RIGHT label.
+  label.SetHorizontalAlignment(Label::ALIGN_RIGHT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  // The text should be left aligned horizontally and centered vertically.
   EXPECT_EQ(0, text_bounds.x());
-  EXPECT_EQ(0, text_bounds.y());
+  EXPECT_EQ(extra.height() / 2 , text_bounds.y());
+  EXPECT_EQ(required_size.width(), text_bounds.width());
+  EXPECT_EQ(required_size.height(), text_bounds.height());
+  EXPECT_EQ(0, flags);
+
+
+  // Test single line drawing with a border.
+  gfx::Insets border(39, 34, 8, 96);
+  label.set_border(Border::CreateEmptyBorder(border.top(),
+                                             border.left(),
+                                             border.bottom(),
+                                             border.right()));
+
+  gfx::Size required_size_with_border(label.GetPreferredSize());
+  EXPECT_EQ(required_size.width() + border.width(),
+            required_size_with_border.width());
+  EXPECT_EQ(required_size.height() + border.height(),
+            required_size_with_border.height());
+  label.SetBounds(0,
+                  0,
+                  required_size_with_border.width() + extra.width(),
+                  required_size_with_border.height() + extra.height());
+
+  // Centered text with border.
+  label.SetHorizontalAlignment(Label::ALIGN_CENTER);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  // The text should be centered horizontally and vertically within the border.
+  EXPECT_EQ(border.left() + extra.width() / 2, text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2 , text_bounds.y());
+  EXPECT_EQ(required_size.width(), text_bounds.width());
+  EXPECT_EQ(required_size.height(), text_bounds.height());
+  EXPECT_EQ(0, flags);
+
+  // ALIGN_LEFT text with border.
+  label.SetHorizontalAlignment(Label::ALIGN_LEFT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  // The text should be right aligned horizontally and centered vertically.
+  EXPECT_EQ(border.left() + extra.width(), text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2 , text_bounds.y());
+  EXPECT_EQ(required_size.width(), text_bounds.width());
+  EXPECT_EQ(required_size.height(), text_bounds.height());
+  EXPECT_EQ(0, flags);
+
+  // ALIGN_RIGHT text.
+  label.SetHorizontalAlignment(Label::ALIGN_RIGHT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  // The text should be left aligned horizontally and centered vertically.
+  EXPECT_EQ(border.left(), text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2 , text_bounds.y());
+  EXPECT_EQ(required_size.width(), text_bounds.width());
+  EXPECT_EQ(required_size.height(), text_bounds.height());
+  EXPECT_EQ(0, flags);
+
+  // Reset locale.
+  base::i18n::SetICUDefaultLocale(locale);
+}
+
+// On Linux the underlying pango routines require a max height in order to
+// ellide multiline text. So until that can be resolved, we set all
+// multiline lables to not ellide in Linux only.
+TEST(LabelTest, DrawMultiLineStringInRTL) {
+  Label label;
+  label.SetFocusable(false);
+
+  // Test for RTL.
+  label.EnableUIMirroringForRTLLanguages(true);
+  std::string locale = l10n_util::GetApplicationLocale(std::wstring());
+  base::i18n::SetICUDefaultLocale("he");
+
+  std::wstring test_text(L"Another string\nwith returns\n\n!");
+  label.SetText(test_text);
+  label.SetMultiLine(true);
+  label.SizeToFit(0);
+  gfx::Size extra(50, 10);
+  label.SetBounds(label.x(),
+                  label.y(),
+                  label.width() + extra.width(),
+                  label.height() + extra.height());
+
+  // Do some basic verifications for all three alignments.
+  std::wstring paint_text;
+  gfx::Rect text_bounds;
+  int flags;
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(extra.width() / 2, text_bounds.x());
+  EXPECT_EQ(extra.height() / 2, text_bounds.y());
+  EXPECT_GT(text_bounds.width(), kMinTextDimension);
+  EXPECT_GT(text_bounds.height(), kMinTextDimension);
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_CENTER, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_CENTER |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+  gfx::Rect center_bounds(text_bounds);
+
+  label.SetHorizontalAlignment(Label::ALIGN_LEFT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(extra.width(), text_bounds.x());
+  EXPECT_EQ(extra.height() / 2, text_bounds.y());
   EXPECT_GT(text_bounds.width(), kMinTextDimension);
   EXPECT_GT(text_bounds.height(), kMinTextDimension);
 #if defined(OS_WIN)
@@ -471,6 +727,25 @@ TEST(LabelTest, DrawMultiLineString) {
       flags);
 #endif
 
+  label.SetHorizontalAlignment(Label::ALIGN_RIGHT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(0, text_bounds.x());
+  EXPECT_EQ(extra.height() / 2, text_bounds.y());
+  EXPECT_GT(text_bounds.width(), kMinTextDimension);
+  EXPECT_GT(text_bounds.height(), kMinTextDimension);
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_LEFT, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_LEFT |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+
   // Test multiline drawing with a border.
   gfx::Insets border(19, 92, 23, 2);
   label.set_border(Border::CreateEmptyBorder(border.top(),
@@ -478,13 +753,18 @@ TEST(LabelTest, DrawMultiLineString) {
                                              border.bottom(),
                                              border.right()));
   label.SizeToFit(0);
+  label.SetBounds(label.x(),
+                  label.y(),
+                  label.width() + extra.width(),
+                  label.height() + extra.height());
+
   label.SetHorizontalAlignment(Label::ALIGN_CENTER);
   paint_text.clear();
   text_bounds.SetRect(0, 0, 0, 0);
   label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
   EXPECT_EQ(test_text, paint_text);
-  EXPECT_EQ(center_bounds.x() + border.left(), text_bounds.x());
-  EXPECT_EQ(center_bounds.y() + border.top(), text_bounds.y());
+  EXPECT_EQ(border.left() + extra.width() / 2, text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2, text_bounds.y());
   EXPECT_EQ(center_bounds.width(), text_bounds.width());
   EXPECT_EQ(center_bounds.height(), text_bounds.height());
 #if defined(OS_WIN)
@@ -496,6 +776,47 @@ TEST(LabelTest, DrawMultiLineString) {
       gfx::Canvas::NO_ELLIPSIS,
       flags);
 #endif
+
+  label.SetHorizontalAlignment(Label::ALIGN_LEFT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(border.left() + extra.width(), text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2, text_bounds.y());
+  EXPECT_EQ(center_bounds.width(), text_bounds.width());
+  EXPECT_EQ(center_bounds.height(), text_bounds.height());
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_RIGHT, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_RIGHT |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+
+  label.SetHorizontalAlignment(Label::ALIGN_RIGHT);
+  paint_text.clear();
+  text_bounds.SetRect(0, 0, 0, 0);
+  label.CalculateDrawStringParams(&paint_text, &text_bounds, &flags);
+  EXPECT_EQ(test_text, paint_text);
+  EXPECT_EQ(border.left(), text_bounds.x());
+  EXPECT_EQ(border.top() + extra.height() / 2, text_bounds.y());
+  EXPECT_EQ(center_bounds.width(), text_bounds.width());
+  EXPECT_EQ(center_bounds.height(), text_bounds.height());
+#if defined(OS_WIN)
+  EXPECT_EQ(gfx::Canvas::MULTI_LINE | gfx::Canvas::TEXT_ALIGN_LEFT, flags);
+#else
+  EXPECT_EQ(
+      gfx::Canvas::MULTI_LINE |
+      gfx::Canvas::TEXT_ALIGN_LEFT |
+      gfx::Canvas::NO_ELLIPSIS,
+      flags);
+#endif
+
+  // Reset Locale
+  base::i18n::SetICUDefaultLocale(locale);
 }
 
 }  // namespace views