RenderText: handle center-aligned text in UpdateCachedBoundsAndOffset()

ui/gfx/render_text_unittest.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/gfx/render_text.h"
 
 #include <algorithm>
 
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 1544 matching lines @@
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(large_content_cases); i++) {
     render_text->SetHorizontalAlignment(large_content_cases[i].alignment);
     render_text->SetDisplayOffset(large_content_cases[i].offset);
     EXPECT_EQ(large_content_cases[i].expected_offset,
               render_text->GetUpdatedDisplayOffset().x());
   }
 }
 
+TEST_F(RenderTextTest, DisplayOffsetForCenteredTextChangingAtEnds) {
+  scoped_ptr<RenderText> render_text(RenderText::CreateInstance());
+  render_text->SetText(ASCIIToUTF16("abcdefghij"));
+  render_text->SetFontList(FontList("Arial, 13px"));

[msw, 2014/07/16 21:55:54]

nit: can you avoid setting the font?

[mohsen, 2014/07/16 23:36:21]

Sure, Done. That was what I saw other tests do.

+  render_text->SetHorizontalAlignment(ALIGN_CENTER);
+
+  const Size display_size(render_text->GetContentWidth() / 2,
+                          render_text->font_list().GetHeight());
+  render_text->SetDisplayRect(Rect(display_size));
+
+  // Move cursor to the beginning of text and check the offset.

[msw, 2014/07/16 21:55:54]

nit: "Move the cursor" here and at line 1588.

[mohsen, 2014/07/16 23:36:21]

Done.

+  render_text->SetCursorPosition(0);
+  int expected_offset =

[msw, 2014/07/16 21:55:54]

nit: inline the |expected_offset| calculations into the EXPECT_EQ macros.

[mohsen, 2014/07/16 23:36:21]

Done.

+      (render_text->GetContentWidth() - display_size.width()) / 2;
+  EXPECT_EQ(expected_offset, render_text->GetUpdatedDisplayOffset().x());

[msw, 2014/07/16 21:55:54]

Hmm, is there a better way of verifying the intended outcomes? For example,
could we instead check that the cursor bounds are within the display rect?

[mohsen, 2014/07/16 23:36:21]

The intended outcome is that no extra space should appear before or after text
when it does not fit the display rect. The best (only?) way I can think of to
verify this is to check the offset against min/max allowed offsets.

[msw, 2014/07/16 23:51:59]

I think glyph bounds would be good indicator and easy to test; ie. check:
EXPECT_EQ(display_rect.x(), render_text->GetGlyphBounds(0).x()) and
EXPECT_EQ(display_rect.right(),
render_text->GetGlyphBounds(render_text->text().length() - 1).right()).
Otherwise, maybe use GetUpdatedCursorBounds or GetSubstringBounds.

+
+  // Make text shorter (by removing a character), put the cursor at the

[msw, 2014/07/16 21:55:54]

nit: "Make the text" here and at line 1594.

[mohsen, 2014/07/16 23:36:21]

Done.

+  // beginning, and check the offset.
+  render_text->SetText(render_text->text().substr(1));
+  render_text->SetCursorPosition(0);
+  expected_offset = (render_text->GetContentWidth() - display_size.width()) / 2;
+  EXPECT_EQ(expected_offset, render_text->GetUpdatedDisplayOffset().x());
+
+  // Move cursor to the end of text and check the offset.
+  render_text->SetCursorPosition(render_text->text().length());
+  expected_offset =
+      (display_size.width() - render_text->GetContentWidth() - 1) / 2;
+  EXPECT_EQ(expected_offset, render_text->GetUpdatedDisplayOffset().x());
+
+  // Make text shorter (by removing a character), put cursor at the end, and

[msw, 2014/07/16 21:55:54]

Instead of setting the text and cursor (which completely changes the object
state), it'd probably be better to just set the display rect and check
GetUpdatedDisplayOffset/GetLineOffset/GetCursorBounds. That would correspond
better with the use case of shrinking or expanding a textfield's width and
expecting the cursor to remain visible, and for there to be no unused space with
text overflow. That said, I'm not sure we have such tests for the more common
left/right alignments...

[mohsen, 2014/07/16 23:36:21]

Done, it's cleaner and more to the point of the test.

+  // check the offset.
+  render_text->SetText(render_text->text().substr(1));
+  render_text->SetCursorPosition(render_text->text().length());
+  expected_offset =
+      (display_size.width() - render_text->GetContentWidth() - 1) / 2;
+  EXPECT_EQ(expected_offset, render_text->GetUpdatedDisplayOffset().x());
+}
+
 TEST_F(RenderTextTest, SameFontForParentheses) {
   struct {
     const base::char16 left_char;
     const base::char16 right_char;
   } punctuation_pairs[] = {
     { '(', ')' },
     { '{', '}' },
     { '<', '>' },
   };
   struct {
@@ skipping 533 matching lines @@
   for (size_t i = 0; i < arraysize(kTestStrings); ++i) {
     render_text->SetText(WideToUTF16(kTestStrings[i]));
     render_text->EnsureLayout();
 
     for (size_t j = 0; j < render_text->text().length(); ++j)
       EXPECT_FALSE(render_text->GetGlyphBounds(j).is_empty());
   }
 }
 
 }  // namespace gfx