From f3997a2dcaa3bb2cdc153e6cbd073016040c4381 Mon Sep 17 00:00:00 2001 From: mukai Date: Fri, 20 Mar 2015 15:19:53 -0700 Subject: [PATCH] Change Label::GetTextSize() behavior back to the original. Previously Label::GetTextSize() was affected by the current layout. This seemed wrong, therefore my patch r320138 changed it to return the ideal text size. However this makes several regressions when Label is combined with BoxLayout or GridLayout, because they assumes the old behaviors. Ideally it's better to fix their layout logic, but before diving into the layout logic, I think I'll revert the behavior back to the original, because there can be some other affected views. Also BoxLayout's class comment says it wants to layout views based on their preferred size, it may be hard to update the behavior. BUG=467526, 468494 R=sky@chromium.org TEST=manually confirmed that reported views work well. the new test case verifies the old behavior. Review URL: https://codereview.chromium.org/1024753005 Cr-Commit-Position: refs/heads/master@{#321652} --- ui/views/controls/label.cc | 8 +++++--- ui/views/controls/label.h | 2 +- ui/views/controls/label_unittest.cc | 24 ++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/ui/views/controls/label.cc b/ui/views/controls/label.cc index fb182a58384684..3d0cf30192cb2c 100644 --- a/ui/views/controls/label.cc +++ b/ui/views/controls/label.cc @@ -515,12 +515,14 @@ gfx::Size Label::GetTextSize() const { // Cancel the display rect of |render_text_|. The display rect may be // specified in GetHeightForWidth(), and specifying empty Rect cancels // its effect. See also the comment in GetHeightForWidth(). - render_text_->SetDisplayRect(gfx::Rect()); + // TODO(mukai): use gfx::Rect() to compute the ideal size rather than + // the current width(). See crbug.com/468494, crbug.com/467526, and + // the comment for MultilinePreferredSizeTest in label_unittest.cc. + render_text_->SetDisplayRect(gfx::Rect(0, 0, width(), 0)); size = render_text_->GetStringSize(); } else { // Get the natural text size, unelided and only wrapped on newlines. - std::vector lines; - base::SplitString(render_text_->GetDisplayText(), '\n', &lines); + std::vector lines = GetLinesForWidth(width()); scoped_ptr render_text(gfx::RenderText::CreateInstance()); render_text->SetFontList(font_list()); for (size_t i = 0; i < lines.size(); ++i) { diff --git a/ui/views/controls/label.h b/ui/views/controls/label.h index 300b17dcf5f0d3..03eade981af0e8 100644 --- a/ui/views/controls/label.h +++ b/ui/views/controls/label.h @@ -172,7 +172,7 @@ class VIEWS_EXPORT Label : public View { // Get the text broken into lines as needed to fit the given |width|. std::vector GetLinesForWidth(int width) const; - // Get the natural text size, unelided and only wrapped on newlines. + // Get the text size for the current layout. gfx::Size GetTextSize() const; void RecalculateColors(); diff --git a/ui/views/controls/label_unittest.cc b/ui/views/controls/label_unittest.cc index af372bfafcb7e2..9dd2733413b3fd 100644 --- a/ui/views/controls/label_unittest.cc +++ b/ui/views/controls/label_unittest.cc @@ -183,6 +183,30 @@ TEST_F(LabelTest, ObscuredSurrogatePair) { EXPECT_EQ(test_text, label.text()); } +// This test case verifies the label preferred size will change based on the +// current layout, which may seem wrong. However many of our code base assumes +// this behavior, therefore this behavior will have to be kept until the code +// with this assumption is fixed. See http://crbug.com/468494 and +// http://crbug.com/467526. +// TODO(mukai): fix the code assuming this behavior and then fix Label +// implementation, and remove this test case. +TEST_F(LabelTest, MultilinePreferredSizeTest) { + Label label; + label.SetText(ASCIIToUTF16("This is an example.")); + + gfx::Size single_line_size = label.GetPreferredSize(); + + label.SetMultiLine(true); + gfx::Size multi_line_size = label.GetPreferredSize(); + EXPECT_EQ(single_line_size, multi_line_size); + + int new_width = multi_line_size.width() / 2; + label.SetBounds(0, 0, new_width, label.GetHeightForWidth(new_width)); + gfx::Size new_size = label.GetPreferredSize(); + EXPECT_GT(multi_line_size.width(), new_size.width()); + EXPECT_LT(multi_line_size.height(), new_size.height()); +} + TEST_F(LabelTest, TooltipProperty) { Label label; label.SetText(ASCIIToUTF16("My cool string."));