Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2866643002:
[android omnibox] Move width calculation to onMeasure step (Closed)
|
Created:
3 years, 7 months ago by Kevin Bailey Modified:
3 years, 7 months ago Reviewers:
Ted C CC:
chromium-reviews, jdonnelly+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[android omnibox] Move width calculation to onMeasure step
The Android text renderer runs in the ::measure() step. If the View
width is changed after this, in the layout or draw step, the text
is not re-laid out; it is truncated.
This change moves up the calculation.
BUG=718432
Review-Url: https://codereview.chromium.org/2866643002
Cr-Commit-Position: refs/heads/master@{#469869}
Committed: https://chromium.googlesource.com/chromium/src/+/d98981b9ab09719407adcb63f639276590f4263b
Patch Set 1 #Patch Set 2 : Incorporated Ted's fix #Messages
Total messages: 19 (10 generated)
Description was changed from ========== [android omnibox] Add minor padding so text isn't truncated The Android text renderer runs in the ::measure() step. If the View width is changed after this, in the layout or draw step, the text is not re-laid out; it is truncated. This change pre-empts that truncation. BUG=718432 ========== to ========== [android omnibox] Add minor padding so text isn't truncated The Android text renderer runs in the ::measure() step. If the View width is changed after this, in the layout or draw step, the text is not re-laid out; it is truncated. This change pre-empts that truncation. BUG=718432 ==========
krb@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Ted, I'm not thrilled with this solution; maybe you can help make it better. Without the padding that I'm adding, the dictionary definition gets truncated. It's not being covered by the arrow; it's actually truncated. I can cause the same effect by giving the layout step a shorter width. The trouble is that text rendering is done in the ::measure() call (not surprising) but not re-done if the layout width changes. So it would appear that the width is changing after the measure step, but it's unclear to me where. The "r - l" on line 860 is very suspicious to me. As far as I can see, it should remain "r", the offset, not "r - l", the width. "l" in this case is 26, very close to the 30-35 that I'm having to add. Nevertheless, removing this subtraction doesn't help, and the original truncated text really is pretty close to the arrow, so I'm not confident of the change. It would make much more sense if 'refineWidth' were approximated here, and changed later (but my logging indicates that it isn't so, 126 throughout.) Do you have a better theory?
On 2017/05/05 01:28:34, Kevin Bailey wrote: > Hi Ted, > > I'm not thrilled with this solution; maybe you can help make it better. > > Without the padding that I'm adding, the dictionary definition gets truncated. > It's not being covered by the arrow; it's actually truncated. I can cause the > same effect by giving the layout step a shorter width. > > The trouble is that text rendering is done in the ::measure() call (not > surprising) but not re-done if the layout width changes. > > So it would appear that the width is changing after the measure step, but it's > unclear to me where. The "r - l" on line 860 is very suspicious to me. As far as > I can see, it should remain "r", the offset, not "r - l", the width. "l" in this > case is 26, very close to the 30-35 that I'm having to add. > > Nevertheless, removing this subtraction doesn't help, and the original truncated > text really is pretty close to the arrow, so I'm not confident of the change. It > would make much more sense if 'refineWidth' were approximated here, and changed > later (but my logging indicates that it isn't so, 126 throughout.) > > Do you have a better theory? How do you trigger definitions? I tried typing it in the omnibox "definition of socialism" and it didn't show up. I confirmed the layouts seem to be right by coloring the background: https://codereview.chromium.org/2862273002 With that said, I think you are right that the layout and measure as mismatched. I "think" this should fix it: https://codereview.chromium.org/2862983003 But we'd need to try with a definition to make sure as it is hard to be sure with a single line text.
On 2017/05/05 16:13:37, Ted C wrote: > How do you trigger definitions? I tried typing it in the omnibox "definition > of socialism" and it didn't show up. chrome://flags/#new-omnibox-answer-types
On 2017/05/05 16:26:43, Justin Donnelly wrote: > On 2017/05/05 16:13:37, Ted C wrote: > > How do you trigger definitions? I tried typing it in the omnibox "definition > > of socialism" and it didn't show up. > > chrome://flags/#new-omnibox-answer-types Hmm...it still doesn't show up for me (restarted, verified the flag was set)
On 2017/05/05 16:30:10, Ted C wrote: > On 2017/05/05 16:26:43, Justin Donnelly wrote: > > On 2017/05/05 16:13:37, Ted C wrote: > > > How do you trigger definitions? I tried typing it in the omnibox > "definition > > > of socialism" and it didn't show up. > > > > chrome://flags/#new-omnibox-answer-types > > Hmm...it still doesn't show up for me (restarted, verified the flag was > set) Sorry, I should have also mentioned that it only works on Canary or Dev. If you're using a local build, you'll need these Finch command-line flags: http://g3doc/analysis/uma/g3doc/finch/testing-with-a-chromium-build.
Description was changed from ========== [android omnibox] Add minor padding so text isn't truncated The Android text renderer runs in the ::measure() step. If the View width is changed after this, in the layout or draw step, the text is not re-laid out; it is truncated. This change pre-empts that truncation. BUG=718432 ========== to ========== [android omnibox] Move width calculation to onMeasure step The Android text renderer runs in the ::measure() step. If the View width is changed after this, in the layout or draw step, the text is not re-laid out; it is truncated. This change moves up the calculation. BUG=718432 ==========
Hi Ted, I verified the fix as well. It also brings back the ellipsis.
lgtm
The CQ bit was checked by krb@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.
The CQ bit was checked by krb@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": 20001, "attempt_start_ts": 1494077028951530,
"parent_rev": "5fbc36a64a6337938eb70e3fc4d0296f3a30cc97", "commit_rev":
"d98981b9ab09719407adcb63f639276590f4263b"}
Message was sent while issue was closed.
Description was changed from ========== [android omnibox] Move width calculation to onMeasure step The Android text renderer runs in the ::measure() step. If the View width is changed after this, in the layout or draw step, the text is not re-laid out; it is truncated. This change moves up the calculation. BUG=718432 ========== to ========== [android omnibox] Move width calculation to onMeasure step The Android text renderer runs in the ::measure() step. If the View width is changed after this, in the layout or draw step, the text is not re-laid out; it is truncated. This change moves up the calculation. BUG=718432 Review-Url: https://codereview.chromium.org/2866643002 Cr-Commit-Position: refs/heads/master@{#469869} Committed: https://chromium.googlesource.com/chromium/src/+/d98981b9ab09719407adcb63f639... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d98981b9ab09719407adcb63f639... |
