Skip to content

Commit

Permalink
Change Label::GetTextSize() behavior back to the original.
Browse files Browse the repository at this point in the history
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}
  • Loading branch information
jmuk authored and Commit bot committed Mar 20, 2015
1 parent e2021e6 commit f3997a2
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 4 deletions.
8 changes: 5 additions & 3 deletions ui/views/controls/label.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<base::string16> lines;
base::SplitString(render_text_->GetDisplayText(), '\n', &lines);
std::vector<base::string16> lines = GetLinesForWidth(width());
scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance());
render_text->SetFontList(font_list());
for (size_t i = 0; i < lines.size(); ++i) {
Expand Down
2 changes: 1 addition & 1 deletion ui/views/controls/label.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<base::string16> 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();
Expand Down
24 changes: 24 additions & 0 deletions ui/views/controls/label_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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."));
Expand Down

0 comments on commit f3997a2

Please sign in to comment.