Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
384953003:
RenderText: handle center-aligned text in UpdateCachedBoundsAndOffset() (Closed)
|
Created:
6 years, 5 months ago by mohsen Modified:
6 years, 5 months ago Reviewers:
msw CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRenderText: handle center-aligned text in UpdateCachedBoundsAndOffset()
In case display width is less than content width,
UpdateCachedBoundsAndOffset() does not handle center-aligned text
properly when content width is reduced (e.g. by deleting some
characters) or display width is increased.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284897
Patch Set 1 #
Total comments: 5
Patch Set 2 : Used SetDisplayOffset() in UpdateChachedBoundsAndOffset() #
Total comments: 8
Patch Set 3 : Cleaned up comments #Patch Set 4 : Added test #
Total comments: 15
Patch Set 5 : Improved test + nits #Patch Set 6 : Used cursor bounds instead of display offset #Patch Set 7 : Disabled on mac #Patch Set 8 : Rebased + Fixed off-by-one problem #Patch Set 9 : Fixed off-by-one problem, more #
Total comments: 7
Patch Set 10 : nits #Patch Set 11 : Added explanation for display offset range calculations #Messages
Total messages: 18 (0 generated)
Please take a look...
https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1344: const int negate_rtl = horizontal_alignment_ == ALIGN_RIGHT ? -1 : 1; nit: consider combining the two scale factors. https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1345: const int double_center = horizontal_alignment_ == ALIGN_CENTER ? 2 : 1; nit: consider |scale_center| https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1347: if (display_width > (content_width + offset)) { I don't think this condition will capture all the necessary cases. Consider a 10px display width, with a 50px text width, offset by -10px. Now, increase the display width to 21px. This condition will not fire, because 21 < 50 + 2 * (-10), but that'll leave some extra empty space on the right... I don't understand the |double_center| scale factor for |offset| and wonder if we need to consider the value of |GetAlignmentOffset|. Can you take a closer look and more clearly explain what's going on (especially if I'm incorrect)?
Please take another look... https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1347: if (display_width > (content_width + offset)) { On 2014/07/11 22:33:04, msw wrote: > I don't think this condition will capture all the necessary cases. Consider a > 10px display width, with a 50px text width, offset by -10px. Now, increase the > display width to 21px. This condition will not fire, because 21 < 50 + 2 * > (-10), but that'll leave some extra empty space on the right... > > I don't understand the |double_center| scale factor for |offset| and wonder if > we need to consider the value of |GetAlignmentOffset|. Can you take a closer > look and more clearly explain what's going on (especially if I'm incorrect)? In your example, no extra empty space will be shown on the right. Suppose center of textfield is fixed at point 0. If |display_width| is 21 and |content_width| is 50, textfield will cover [-10.5, 10.5] and text will cover [-25, 25], when there is no offset. Now adding offset of -10 shifts the text 10 pixels to left, so it will cover [-35, 15] which is still covering the entire textfield. As for |double_center| scale factor, maybe it's best to think of it as follows: in case of center-aligned text, if we divide both text and textfield in half and only consider their right halves, it's like they're left-aligned and all calculations for left-aligned texts would work. The above explanation works when offset is less than zero. If offset is greater than zero, then we should use calculations for right-aligned texts. I haven't handled this case here (my mistake) and adding it will make this code more complicated. Instead, I'm going to switch to using SetDisplayOffset() function which handles all these cases.
Can you add a test that this CL fixes? https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/1/ui/gfx/render_text.cc#newcod... ui/gfx/render_text.cc:1347: if (display_width > (content_width + offset)) { On 2014/07/14 20:57:01, mohsen wrote: > On 2014/07/11 22:33:04, msw wrote: > > I don't think this condition will capture all the necessary cases. Consider a > > 10px display width, with a 50px text width, offset by -10px. Now, increase the > > display width to 21px. This condition will not fire, because 21 < 50 + 2 * > > (-10), but that'll leave some extra empty space on the right... > > > > I don't understand the |double_center| scale factor for |offset| and wonder if > > we need to consider the value of |GetAlignmentOffset|. Can you take a closer > > look and more clearly explain what's going on (especially if I'm incorrect)? > > In your example, no extra empty space will be shown on the right. Suppose center > of textfield is fixed at point 0. I think this is wrong, the alignment offset > If |display_width| is 21 and |content_width| > is 50, textfield will cover [-10.5, 10.5] and text will cover [-25, 25], when > there is no offset. Now adding offset of -10 shifts the text 10 pixels to left, > so it will cover [-35, 15] which is still covering the entire textfield. > > As for |double_center| scale factor, maybe it's best to think of it as follows: > in case of center-aligned text, if we divide both text and textfield in half and > only consider their right halves, it's like they're left-aligned and all > calculations for left-aligned texts would work. > > The above explanation works when offset is less than zero. If offset is greater > than zero, then we should use calculations for right-aligned texts. I haven't > handled this case here (my mistake) and adding it will make this code more > complicated. Instead, I'm going to switch to using SetDisplayOffset() function > which handles all these cases. Ah, I didn't account for GetAlignmentOffset. https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (left): https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ol... ui/gfx/render_text.cc:1342: // Reduce the pan offset to show additional overflow text when the display Does the new code still do this via min/max clamping in SetDisplayOffset? https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1317: // |display_offset_|. Then calculate the change in offset needed to to move nit: fix "to to" https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1323: // TODO(xji): when the character overflow is a RTL character, currently, nit: consolidate these two as TODO(bidi) above the if/else https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1328: // Pan to show the cursor when it overflows to the right. nit: remove the "pan to show" comments, the top comment is sufficient.
Added test and cleaned up comments. https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (left): https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ol... ui/gfx/render_text.cc:1342: // Reduce the pan offset to show additional overflow text when the display On 2014/07/16 01:41:43, msw wrote: > Does the new code still do this via min/max clamping in SetDisplayOffset? Yes. https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1317: // |display_offset_|. Then calculate the change in offset needed to to move On 2014/07/16 01:41:43, msw wrote: > nit: fix "to to" Done. https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1323: // TODO(xji): when the character overflow is a RTL character, currently, On 2014/07/16 01:41:43, msw wrote: > nit: consolidate these two as TODO(bidi) above the if/else Done. Please check if the result is satisfactory. https://codereview.chromium.org/384953003/diff/20001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1328: // Pan to show the cursor when it overflows to the right. On 2014/07/16 01:41:43, msw wrote: > nit: remove the "pan to show" comments, the top comment is sufficient. Done.
https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1322: // TODO(bidi): In left-aligned text, when overflow character is an RTL nit: use this one-liner: "// TODO(bidi): Show RTL glyphs at the cursor position for ALIGN_LEFT, etc." https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1568: render_text->SetFontList(FontList("Arial, 13px")); nit: can you avoid setting the font? https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1575: // Move cursor to the beginning of text and check the offset. nit: "Move the cursor" here and at line 1588. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1577: int expected_offset = nit: inline the |expected_offset| calculations into the EXPECT_EQ macros. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1579: EXPECT_EQ(expected_offset, render_text->GetUpdatedDisplayOffset().x()); 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? https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1581: // Make text shorter (by removing a character), put the cursor at the nit: "Make the text" here and at line 1594. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1594: // Make text shorter (by removing a character), put cursor at the end, and 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...
https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1322: // TODO(bidi): In left-aligned text, when overflow character is an RTL On 2014/07/16 21:55:54, msw wrote: > nit: use this one-liner: "// TODO(bidi): Show RTL glyphs at the cursor position > for ALIGN_LEFT, etc." Done. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1568: render_text->SetFontList(FontList("Arial, 13px")); On 2014/07/16 21:55:54, msw wrote: > nit: can you avoid setting the font? Sure, Done. That was what I saw other tests do. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1575: // Move cursor to the beginning of text and check the offset. On 2014/07/16 21:55:54, msw wrote: > nit: "Move the cursor" here and at line 1588. Done. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1577: int expected_offset = On 2014/07/16 21:55:54, msw wrote: > nit: inline the |expected_offset| calculations into the EXPECT_EQ macros. Done. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1579: EXPECT_EQ(expected_offset, render_text->GetUpdatedDisplayOffset().x()); On 2014/07/16 21:55:54, msw wrote: > 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? 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. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1581: // Make text shorter (by removing a character), put the cursor at the On 2014/07/16 21:55:54, msw wrote: > nit: "Make the text" here and at line 1594. Done. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1594: // Make text shorter (by removing a character), put cursor at the end, and On 2014/07/16 21:55:54, msw wrote: > 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... Done, it's cleaner and more to the point of the test.
Thanks, this is looking better, but I still hope we can avoid explicitly computing an offset in the test itself, and instead check some more obvious resulting state. https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/384953003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1579: EXPECT_EQ(expected_offset, render_text->GetUpdatedDisplayOffset().x()); On 2014/07/16 23:36:21, mohsen wrote: > On 2014/07/16 21:55:54, msw wrote: > > 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? > > 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. 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.
GetGlyphBounds() returns a glyph bound relative to the text itself, not the display rect, and is not useful for our purpose. I switched to using cursor bounds which seems more appropriate.
On 2014/07/17 15:56:12, mohsen wrote: > GetGlyphBounds() returns a glyph bound relative to the text itself, not the > display rect, and is not useful for our purpose. I switched to using cursor > bounds which seems more appropriate. RenderText::ToViewPoint does coordinate conversion, but this lgtm!
Can you take another look, please, specifically changes in RenderText::SetDisplayOffset().
lgtm with nits. https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:916: min_offset = -(extra_content - cursor_width + 1) / 2 - cursor_width; nit: It's a bit odd that ALIGN_[LEFT|RIGHT] consider the extra cursor width part of the text width to be offset, but ALIGN_CENTER doesn't. I suppose that may be reasonable (at least when the content width is smaller than the display width), but I think this deserves a comment at least. https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1083: TEST_F(RenderTextTest, DisplayOffsetForCenteredTextChangingAtEnds) { nit: consider CenteredDisplayOffset or similar. https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1111: // Widen the display rect and, by checking the cursor bounds, make sure not nit: s/not/no
https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:916: min_offset = -(extra_content - cursor_width + 1) / 2 - cursor_width; On 2014/07/22 22:00:27, msw wrote: > nit: It's a bit odd that ALIGN_[LEFT|RIGHT] consider the extra cursor width part > of the text width to be offset, but ALIGN_CENTER doesn't. I suppose that may be > reasonable (at least when the content width is smaller than the display width), > but I think this deserves a comment at least. I'm not getting exactly what you mean here. The extra pixel for cursor is considered in all cases (it is considered in GetcontentWidth() if cursor is enabled). But, for ALIGN_CENTER we need to subtract it, do the calculations, and then add it back. https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1083: TEST_F(RenderTextTest, DisplayOffsetForCenteredTextChangingAtEnds) { On 2014/07/22 22:00:27, msw wrote: > nit: consider CenteredDisplayOffset or similar. Done. https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1111: // Widen the display rect and, by checking the cursor bounds, make sure not On 2014/07/22 22:00:27, msw wrote: > nit: s/not/no Done.
https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:916: min_offset = -(extra_content - cursor_width + 1) / 2 - cursor_width; On 2014/07/22 23:38:35, mohsen wrote: > On 2014/07/22 22:00:27, msw wrote: > > nit: It's a bit odd that ALIGN_[LEFT|RIGHT] consider the extra cursor width > part > > of the text width to be offset, but ALIGN_CENTER doesn't. I suppose that may > be > > reasonable (at least when the content width is smaller than the display > width), > > but I think this deserves a comment at least. > > I'm not getting exactly what you mean here. The extra pixel for cursor is > considered in all cases (it is considered in GetcontentWidth() if cursor is > enabled). But, for ALIGN_CENTER we need to subtract it, do the calculations, and > then add it back. That's exactly what I'm saying is confusing, and warrants an explanatory comment, that you "need to subtract it, do the calculations, and then add it back." Add a comment explaining why you need to do that.
The CQ bit was checked by mohsen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/384953003/400001
On 2014/07/23 03:08:17, msw wrote: > https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc > File ui/gfx/render_text.cc (right): > > https://codereview.chromium.org/384953003/diff/360001/ui/gfx/render_text.cc#n... > ui/gfx/render_text.cc:916: min_offset = -(extra_content - cursor_width + 1) / 2 > - cursor_width; > On 2014/07/22 23:38:35, mohsen wrote: > > On 2014/07/22 22:00:27, msw wrote: > > > nit: It's a bit odd that ALIGN_[LEFT|RIGHT] consider the extra cursor width > > part > > > of the text width to be offset, but ALIGN_CENTER doesn't. I suppose that may > > be > > > reasonable (at least when the content width is smaller than the display > > width), > > > but I think this deserves a comment at least. > > > > I'm not getting exactly what you mean here. The extra pixel for cursor is > > considered in all cases (it is considered in GetcontentWidth() if cursor is > > enabled). But, for ALIGN_CENTER we need to subtract it, do the calculations, > and > > then add it back. > > That's exactly what I'm saying is confusing, and warrants an explanatory > comment, that you "need to subtract it, do the calculations, and then add it > back." Add a comment explaining why you need to do that. That makes sense. Done.
Message was sent while issue was closed.
Change committed as 284897 |
