fix know issues in RenderText

views/controls/textfield/native_textfield_views_unittest.cc

Index: views/controls/textfield/native_textfield_views_unittest.cc
===================================================================
--- views/controls/textfield/native_textfield_views_unittest.cc	(revision 100008)
+++ views/controls/textfield/native_textfield_views_unittest.cc	(working copy)
@@ -2,6 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+#include <vector>
+
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -15,6 +18,7 @@
 #include "ui/base/clipboard/scoped_clipboard_writer.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
 #include "ui/base/keycodes/keyboard_codes.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/render_text.h"
 #include "views/controls/textfield/native_textfield_views.h"
 #include "views/controls/textfield/textfield.h"
@@ -215,6 +219,19 @@
     SendKeyEvent(key_code, false, false);
   }
 
+  void SendKeyEvent(char16 ch) {
+    if (ch < 0x80) {
+      ui::KeyboardCode code =
+          ch == ' ' ? ui::VKEY_SPACE :
+          static_cast<ui::KeyboardCode>(ui::VKEY_A + ch - 'a');
+      SendKeyEvent(code);
+    } else {
+      KeyEvent event(ui::ET_KEY_PRESSED, ui::VKEY_UNKNOWN, 0);
+      event.set_character(ch);
+      input_method_->DispatchKeyEvent(event);
+    }
+  }
+
   View* GetFocusedView() {
     return widget_->GetFocusManager()->GetFocusedView();
   }
@@ -225,6 +242,44 @@
         gfx::SelectionModel(cursor_pos), false).x();
   }
 
+  // Get the current cursor bounds.
+  gfx::Rect GetCursorBounds() {
+    gfx::RenderText* render_text = textfield_view_->GetRenderText();
+    gfx::Rect bounds = render_text->GetUpdatedCursorBounds();
+    return bounds;
+  }
+
+  // Get the cursor bounds of |sel|.
+  gfx::Rect GetCursorBounds(const gfx::SelectionModel& sel) {
+    gfx::RenderText* render_text = textfield_view_->GetRenderText();
+    gfx::Rect bounds = render_text->GetCursorBounds(sel, true);
+    return bounds;
+  }
+
+  gfx::Rect GetDisplayRect() {
+    return textfield_view_->GetRenderText()->display_rect();
+  }
+
+  // Mouse click on the point whose x-axis is |bound|'s x plus |x_offset| and
+  // y-axis is in the middle of |bound|'s vertical range.
+  void MouseClick(const gfx::Rect bound, int x_offset) {

[msw, 2011/09/09 23:03:24]

Why not use a Point instead of a Rect? Use Rect::origin() as needed.

[xji, 2011/09/12 21:31:48]

I need rect.height() as well.

+    int x = bound.x() +  x_offset;
+    int y = bound.y() + bound.height() / 2;
+    MouseEvent click(ui::ET_MOUSE_PRESSED, x, y, ui::EF_LEFT_BUTTON_DOWN);
+    textfield_view_->OnMousePressed(click);
+    MouseEvent release(ui::ET_MOUSE_RELEASED, 0, 0, ui::EF_LEFT_BUTTON_DOWN);

[msw, 2011/09/09 23:03:24]

The release event should probably be at the same x and y coordinates.

[xji, 2011/09/12 21:31:48]

Done.

+    textfield_view_->OnMouseReleased(release);
+  }
+
+  void NonClientMouseClick() {

[msw, 2011/09/09 23:03:24]

Using this to avoid double/triple click is pretty ingenious. Can you add a
comment to clarify that purpose here?

[xji, 2011/09/12 21:31:48]

Done.

+    MouseEvent click(ui::ET_MOUSE_PRESSED, 0, 0,
+                     ui::EF_LEFT_BUTTON_DOWN | ui::EF_IS_NON_CLIENT);
+    textfield_view_->OnMousePressed(click);
+    MouseEvent release(ui::ET_MOUSE_RELEASED, 0, 0,
+                       ui::EF_LEFT_BUTTON_DOWN | ui::EF_IS_NON_CLIENT);
+    textfield_view_->OnMouseReleased(release);
+  }
+
   // Wrap for visibility in test classes.
   ui::TextInputType GetTextInputType() {
     return textfield_view_->GetTextInputType();
@@ -1141,4 +1196,323 @@
   EXPECT_STR_EQ("ab3", textfield_->text());
 }
 
+TEST_F(NativeTextfieldViewsTest, TextCursorDisplayTest) {

[msw, 2011/09/09 23:03:24]

Some of these tests might be better suited as RenderTextTests.

[xji, 2011/09/12 21:31:48]

Looking at those tests again, I think in NativeTextfieldViewsTest, other tests
have mouse clicking event and could not be moved to RenderTextTest except
TextCursorDisplay(_inRTL)Test. But if running in RenderTextTest, SetText() has
to be used along with SetCursorPosition() in order for the cursor bounds to be
displayed correctly. So, I feel all the tests here probably should stay.

In TextfieldViewsModelTest, those tests that explicitly reset cursor position
after set text, and they tests pure render text operations (such as
MoveCursorLeft/Right when there is no composition text involved) should move to
RenderTextTest. So, I am moving MoveCursorLeft/Right to RenderTextTest. 

Looks like I can also move Selection and SelectWord to RenderTextTest but then I
will need to test the selection range. I prefer to test the string so I leave
selection related tests untouched.

+  InitTextfield(Textfield::STYLE_DEFAULT);
+  // LTR-RTL string in LTR context.
+  SendKeyEvent('a');
+  EXPECT_STR_EQ("a", textfield_->text());
+  int x = GetCursorBounds().x();
+  int prev_x = x;
+
+  SendKeyEvent('b');
+  EXPECT_STR_EQ("ab", textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_LT(prev_x, x);
+  prev_x = x;
+
+  SendKeyEvent(0x05E1);

[msw, 2011/09/09 23:03:24]

Defining constants (if none exist) would be preferable to using un-explained
literals throughout this code.

[xji, 2011/09/12 21:31:48]

Hm... I tried that. But that makes the subsequent EXPECT_EQ not clear enough
unless change the subsequent EXPECT_EQ to use string append first.
So, I reverted it and only use the const when I do not care about the text
itself.

+  EXPECT_EQ(WideToUTF16(L"ab\x05E1"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_EQ(prev_x, x);
+
+  SendKeyEvent(0x05E2);
+  EXPECT_EQ(WideToUTF16(L"ab\x05E1\x5E2"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_EQ(prev_x, x);
+
+  // Clear text.
+  SendKeyEvent(ui::VKEY_A, false, true);

[msw, 2011/09/09 23:03:24]

You can just call SetText here and elsewhere that you clear the textfield.

[xji, 2011/09/12 21:31:48]

I am trying to emulate the real UI operations. Maybe I should leave it as is.

+  SendKeyEvent('\n');
+
+  // RTL-LTR string in LTR context.
+  SendKeyEvent(0x05E1);
+  EXPECT_EQ(WideToUTF16(L"\x05E1"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_EQ(GetDisplayRect().x(), x);
+  prev_x = x;
+
+  SendKeyEvent(0x05E2);
+  EXPECT_EQ(WideToUTF16(L"\x05E1\x05E2"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_EQ(prev_x, x);
+
+  SendKeyEvent('a');
+  EXPECT_EQ(WideToUTF16(L"\x05E1\x5E2"L"a"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_LT(prev_x, x);
+  prev_x = x;
+
+  SendKeyEvent('b');
+  EXPECT_EQ(WideToUTF16(L"\x05E1\x5E2"L"ab"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_LT(prev_x, x);
+}
+
+TEST_F(NativeTextfieldViewsTest, TextCursorDisplayInRTLTest) {
+  std::string locale = l10n_util::GetApplicationLocale("");
+  base::i18n::SetICUDefaultLocale("he");
+
+  InitTextfield(Textfield::STYLE_DEFAULT);
+  // LTR-RTL string in RTL context.
+  SendKeyEvent('a');
+  EXPECT_STR_EQ("a", textfield_->text());
+  int x = GetCursorBounds().x();
+  EXPECT_EQ(GetDisplayRect().right() - 1, x);
+  int prev_x = x;
+
+  SendKeyEvent('b');
+  EXPECT_STR_EQ("ab", textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_EQ(prev_x, x);
+
+  SendKeyEvent(0x05E1);
+  EXPECT_EQ(WideToUTF16(L"ab\x05E1"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_GT(prev_x, x);
+  prev_x = x;
+
+  SendKeyEvent(0x05E2);
+  EXPECT_EQ(WideToUTF16(L"ab\x05E1\x5E2"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_GT(prev_x, x);
+
+  SendKeyEvent(ui::VKEY_A, false, true);

[msw, 2011/09/09 23:03:24]

SetText

+  SendKeyEvent('\n');
+
+  // RTL-LTR string in RTL context.
+  SendKeyEvent(0x05E1);
+  EXPECT_EQ(WideToUTF16(L"\x05E1"), textfield_->text());
+  x = GetCursorBounds().x();
+  prev_x = x;
+
+  SendKeyEvent(0x05E2);
+  EXPECT_EQ(WideToUTF16(L"\x05E1\x05E2"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_GT(prev_x, x);
+  prev_x = x;
+
+  SendKeyEvent('a');
+  EXPECT_EQ(WideToUTF16(L"\x05E1\x5E2"L"a"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_EQ(prev_x, x);
+
+  SendKeyEvent('b');
+  EXPECT_EQ(WideToUTF16(L"\x05E1\x5E2"L"ab"), textfield_->text());
+  x = GetCursorBounds().x();
+  EXPECT_EQ(prev_x, x);
+
+  // Reset locale.
+  base::i18n::SetICUDefaultLocale(locale);
+}
+
+TEST_F(NativeTextfieldViewsTest, HitInsideTextAreaTest) {
+  InitTextfield(Textfield::STYLE_DEFAULT);
+  textfield_->SetText(WideToUTF16(L"ab\x05E1\x5E2"));
+  std::vector<gfx::Rect> cursor_bounds;
+
+  // Save each cursor bound.
+  gfx::SelectionModel sel(0, 0, gfx::SelectionModel::LEADING);
+  cursor_bounds.push_back(GetCursorBounds(sel));
+
+  sel = gfx::SelectionModel(1, 0, gfx::SelectionModel::TRAILING);
+  gfx::Rect bound = GetCursorBounds(sel);
+  sel = gfx::SelectionModel(1, 1, gfx::SelectionModel::LEADING);
+  EXPECT_EQ(bound, GetCursorBounds(sel));
+  cursor_bounds.push_back(bound);
+
+  sel = gfx::SelectionModel(2, 1, gfx::SelectionModel::TRAILING);
+  bound = GetCursorBounds(sel);
+  sel = gfx::SelectionModel(4, 3, gfx::SelectionModel::TRAILING);
+  EXPECT_EQ(bound, GetCursorBounds(sel));
+  cursor_bounds.push_back(bound);
+
+  sel = gfx::SelectionModel(3, 2, gfx::SelectionModel::TRAILING);
+  bound = GetCursorBounds(sel);
+  sel = gfx::SelectionModel(3, 3, gfx::SelectionModel::LEADING);
+  EXPECT_EQ(bound, GetCursorBounds(sel));
+  cursor_bounds.push_back(bound);
+
+  sel = gfx::SelectionModel(2, 2, gfx::SelectionModel::LEADING);
+  bound = GetCursorBounds(sel);
+  sel = gfx::SelectionModel(4, 2, gfx::SelectionModel::LEADING);
+  EXPECT_EQ(bound, GetCursorBounds(sel));
+  cursor_bounds.push_back(bound);
+
+  // Expected cursor position when clicking left and right of each character.
+  size_t cursor_pos_expected[] = {0, 1, 1, 2, 4, 3, 3, 2};
+
+  int index = 0;
+  for (int i = 0; i < static_cast<int>(cursor_bounds.size() - 1); ++i) {
+    int half_width = (cursor_bounds[i + 1].x() - cursor_bounds[i].x()) / 2;
+    MouseClick(cursor_bounds[i], half_width / 2);
+    EXPECT_EQ(cursor_pos_expected[index++], textfield_->GetCursorPosition());
+
+    // To avoid trigger double click. Not using sleep() since it takes longer
+    // for the test to run if using sleep().
+    NonClientMouseClick();
+
+    MouseClick(cursor_bounds[i + 1], -(half_width / 2));

[msw, 2011/09/09 23:03:24]

space between '-' and '('

[xji, 2011/09/12 21:31:48]

Done.

+    EXPECT_EQ(cursor_pos_expected[index++], textfield_->GetCursorPosition());
+
+    NonClientMouseClick();
+  }
+}
+
+TEST_F(NativeTextfieldViewsTest, HitOutsideTextAreaTest) {
+  InitTextfield(Textfield::STYLE_DEFAULT);
+
+  // LTR-RTL string in LTR context.
+  textfield_->SetText(WideToUTF16(L"ab\x05E1\x5E2"));
+
+  SendKeyEvent(ui::VKEY_HOME);
+  gfx::Rect bound = GetCursorBounds();
+  MouseClick(bound, -10);
+  EXPECT_EQ(bound, GetCursorBounds());
+
+  SendKeyEvent(ui::VKEY_END);
+  bound = GetCursorBounds();
+  MouseClick(bound, 10);

[msw, 2011/09/09 23:03:24]

Is 10 pixels really longer than the text width?
Using the text width plus some buffer would be more robust.
Same for other 10 pixel literals below.

[xji, 2011/09/12 21:31:48]

10 pixel is the x_offset from END position's x-axis.

+  EXPECT_EQ(bound, GetCursorBounds());
+
+  NonClientMouseClick();
+
+  // RTL-LTR string in LTR context.
+  textfield_->SetText(WideToUTF16(L"\x05E1\x5E2"L"ab"));
+
+  SendKeyEvent(ui::VKEY_HOME);
+  bound = GetCursorBounds();
+#if defined(OS_WIN)
+  MouseClick(bound, -10);
+#else
+  MouseClick(bound, 10);
+#endif
+  EXPECT_EQ(bound, GetCursorBounds());
+
+  SendKeyEvent(ui::VKEY_END);
+  bound = GetCursorBounds();
+#if defined(OS_WIN)
+  MouseClick(bound, 10);
+#else
+  MouseClick(bound, -10);
+#endif
+  EXPECT_EQ(bound, GetCursorBounds());
+}
+
+TEST_F(NativeTextfieldViewsTest, HitOutsideTextAreaInRTLTest) {
+  std::string locale = l10n_util::GetApplicationLocale("");
+  base::i18n::SetICUDefaultLocale("he");
+
+  InitTextfield(Textfield::STYLE_DEFAULT);
+
+  // RTL-LTR string in RTL context.
+  textfield_->SetText(WideToUTF16(L"\x05E1\x5E2"L"ab"));
+  SendKeyEvent(ui::VKEY_HOME);
+  gfx::Rect bound = GetCursorBounds();
+  MouseClick(bound, 10);
+  EXPECT_EQ(bound, GetCursorBounds());
+
+  SendKeyEvent(ui::VKEY_END);
+  bound = GetCursorBounds();
+  MouseClick(bound, -10);
+  EXPECT_EQ(bound, GetCursorBounds());
+
+  NonClientMouseClick();
+
+  // LTR-RTL string in RTL context.
+  textfield_->SetText(WideToUTF16(L"ab\x05E1\x5E2"));
+  SendKeyEvent(ui::VKEY_HOME);
+  bound = GetCursorBounds();
+#if defined(OS_WIN)
+  MouseClick(bound, 10);
+#else
+  MouseClick(bound, -10);
+#endif
+  EXPECT_EQ(bound, GetCursorBounds());
+
+  SendKeyEvent(ui::VKEY_END);
+  bound = GetCursorBounds();
+#if defined(OS_WIN)
+  MouseClick(bound, -10);
+#else
+  MouseClick(bound, 10);
+#endif
+  EXPECT_EQ(bound, GetCursorBounds());
+
+  // Reset locale.
+  base::i18n::SetICUDefaultLocale(locale);
+}
+
+void OverflowCursorBoundTestVerifier(const gfx::Rect& display,

[msw, 2011/09/09 23:03:24]

A comment would be appreciated:
This verifies that |bound| is contained by |display|.
|bound|'s right edge must be less than |diaplay|'s right edge.

[xji, 2011/09/12 21:31:48]

Done.

+                          const gfx::Rect& bound) {

[msw, 2011/09/09 23:03:24]

Indent this line to match the line above.

[xji, 2011/09/12 21:31:48]

Done.

+  EXPECT_LE(display.x(), bound.x());
+  EXPECT_GT(display.right(), bound.right());
+  EXPECT_LE(display.y(), bound.y());
+  EXPECT_GE(display.bottom(), bound.bottom());
+}
+
+TEST_F(NativeTextfieldViewsTest, OverflowTest) {
+  InitTextfield(Textfield::STYLE_DEFAULT);
+
+  string16 str;
+  for (int i = 0; i < 500; ++i)
+    SendKeyEvent('a');
+  SendKeyEvent(0x05E1);

[msw, 2011/09/09 23:03:24]

Don't we have a bug where on overflow from LTR text, typing an RTL character
doesn't show the full character typed?

What's the intent of this aspect of these tests?

[xji, 2011/09/12 21:31:48]

Yes we do have that bug.
This test does not test whether the character displays inside display_rect. It
only test the cursor bound is inside the range.

+  gfx::Rect bound = GetCursorBounds();
+  gfx::Rect display = GetDisplayRect();
+  OverflowCursorBoundTestVerifier(display, bound);
+
+  // Test mouse pointing.
+  MouseClick(bound, -1);
+  EXPECT_EQ(500U, textfield_->GetCursorPosition());
+
+  // Clear text.
+  SendKeyEvent(ui::VKEY_A, false, true);

[msw, 2011/09/09 23:03:24]

SetText

+  SendKeyEvent('\n');
+
+  for (int i = 0; i < 500; ++i)
+    SendKeyEvent(0x05E1);
+  SendKeyEvent('a');
+  bound = GetCursorBounds();
+  display = GetDisplayRect();
+  OverflowCursorBoundTestVerifier(display, bound);
+
+  MouseClick(bound, -1);
+  EXPECT_EQ(501U, textfield_->GetCursorPosition());
+}
+
+TEST_F(NativeTextfieldViewsTest, OverflowInRTLTest) {
+  std::string locale = l10n_util::GetApplicationLocale("");
+  base::i18n::SetICUDefaultLocale("he");
+
+  InitTextfield(Textfield::STYLE_DEFAULT);
+
+  string16 str;
+  for (int i = 0; i < 500; ++i)
+    SendKeyEvent('a');
+  SendKeyEvent(0x05E1);
+  gfx::Rect bound = GetCursorBounds();
+  gfx::Rect display = GetDisplayRect();
+  OverflowCursorBoundTestVerifier(display, bound);
+
+  MouseClick(bound, 1);
+  EXPECT_EQ(501U, textfield_->GetCursorPosition());
+
+  // Clear text.
+  SendKeyEvent(ui::VKEY_A, false, true);

[msw, 2011/09/09 23:03:24]

SetText

+  SendKeyEvent('\n');
+
+  for (int i = 0; i < 500; ++i)
+    SendKeyEvent(0x05E1);
+  SendKeyEvent('a');
+  bound = GetCursorBounds();
+  display = GetDisplayRect();
+  OverflowCursorBoundTestVerifier(display, bound);
+
+  MouseClick(bound, 1);
+  EXPECT_EQ(500U, textfield_->GetCursorPosition());
+
+  // Reset locale.
+  base::i18n::SetICUDefaultLocale(locale);
+}
+
 }  // namespace views