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

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

Now, Label::CalculateDrawStringParams() computes text boundary without differentiate singleline and multiline 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

TBR=xji@chromium.org
Review URL: http://codereview.chromium.org/1988001

git-svn-id: svn://svn.chromium.org/chrome/branches/375/src@46495 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 89e9a572..1baba68 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