Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767163003:
Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight(). (Closed)
|
Created:
3 years, 9 months ago by tapted Modified:
3 years, 8 months ago Reviewers:
msw CC:
chromium-reviews, tfarina, lgarron+watch_chromium.org, derat+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionVertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight().
Currently the text is only vertically centered in the non-multi-line
case.
Test by comparing the effect of increasing the display rectangle versus
increasing the line height. These should behave the same, except in the
latter case the selection highlight should also increase in height.
BUG=704404
Review-Url: https://codereview.chromium.org/2767163003
Cr-Commit-Position: refs/heads/master@{#461361}
Committed: https://chromium.googlesource.com/chromium/src/+/9ec27b9e456d7d7f4899a2d9e7dc4efd8a1d1e5b
Patch Set 1 : awesomer test #
Total comments: 7
Patch Set 2 : respond to comments #
Dependent Patchsets: Messages
Total messages: 38 (31 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight() cl format with the fix + logging cl format working prgress BUG= ========== to ========== Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight(). Currently the text is only vertically centered in the non-mulit-line case. Test by comparing the effect of increasing the display rectangle versus increasing the line height. These should behave the same, except in the latter case the selection highlight should also increase in height. BUG=704404 ==========
tapted@chromium.org changed reviewers: + msw@chromium.org
Hi Mike, please take a look. https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4482: EXPECT_EQ(normal_baseline + baseline_shift, test_api()->lines()[0].baseline); (without the fix, these are the first two lines that fail, since the offset moves) https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4488: EXPECT_EQ(gfx::Vector2d(), current_selection_bounds.OffsetFromOrigin()); (which means this also fails - the selection bounds shifts down ~7 pixels, so no longer aligns with the display rectangle) https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4496: EXPECT_EQ(normal_baseline + baseline_shift, test_api()->lines()[0].baseline); (for the multiline case, this is the only case that would fail - the selection bounds is "right", but only because the text is aligned to the top)
Nice! lgtm with minor qs on the test. https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4425: // Note this is equivalent to RenderText::GetDisplayTextBaseline(). q: why not just use that here and on 4457, 4482, and 4496? (that might even make some EnsureLayout calls here unnecessary) https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4459: // The actual shift depends on font metrics, but it should be within a pixel Can we use the actual font metrics to make this a bit more resilient?
Description was changed from ========== Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight(). Currently the text is only vertically centered in the non-mulit-line case. Test by comparing the effect of increasing the display rectangle versus increasing the line height. These should behave the same, except in the latter case the selection highlight should also increase in height. BUG=704404 ========== to ========== Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight(). Currently the text is only vertically centered in the non-multi-line case. Test by comparing the effect of increasing the display rectangle versus increasing the line height. These should behave the same, except in the latter case the selection highlight should also increase in height. BUG=704404 ==========
https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4425: // Note this is equivalent to RenderText::GetDisplayTextBaseline(). On 2017/03/31 17:19:27, msw wrote: > q: why not just use that here and on 4457, 4482, and 4496? > (that might even make some EnsureLayout calls here unnecessary) Done (added to TestApi + replaced the EnsureLayout calls). https://codereview.chromium.org/2767163003/diff/120001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:4459: // The actual shift depends on font metrics, but it should be within a pixel On 2017/03/31 17:19:27, msw wrote: > Can we use the actual font metrics to make this a bit more resilient? We'd have to repeat all the calculations done in DetermineBaselineCenteringText(..), but this step is mainly to ensure that the "centering" part of those calculations is actually centering (within some tolerance). Updated the comment: // The actual shift depends on font metrics, and the calculations done in // RenderText::DetermineBaselineCenteringText(). Do a sanity check that the // "centering" part is happening within some tolerance by ensuring the shift // is within a pixel of (kDelta / 2). That is, 7, 8 or 9 pixels (for a delta // of 16). An unusual font in future may need more leeway.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2767163003/#ps140001 (title: "respond to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491184124545570,
"parent_rev": "1970247d68492c3fa208468e34aab2d7803c192b", "commit_rev":
"9ec27b9e456d7d7f4899a2d9e7dc4efd8a1d1e5b"}
Message was sent while issue was closed.
Description was changed from ========== Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight(). Currently the text is only vertically centered in the non-multi-line case. Test by comparing the effect of increasing the display rectangle versus increasing the line height. These should behave the same, except in the latter case the selection highlight should also increase in height. BUG=704404 ========== to ========== Vertically center multi-line RenderTextHarfBuzz that uses SetMinLineHeight(). Currently the text is only vertically centered in the non-multi-line case. Test by comparing the effect of increasing the display rectangle versus increasing the line height. These should behave the same, except in the latter case the selection highlight should also increase in height. BUG=704404 Review-Url: https://codereview.chromium.org/2767163003 Cr-Commit-Position: refs/heads/master@{#461361} Committed: https://chromium.googlesource.com/chromium/src/+/9ec27b9e456d7d7f4899a2d9e7dc... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9ec27b9e456d7d7f4899a2d9e7dc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
