Chromium Code Reviews
|
|
DescriptionRe-enable TouchActionTest.Simple and fix the test on Android.
Force the line height on the touch-action-tests to be 1 (matches the
font height). The tests are quite dependent on the line height of
text above it; forcing the line height to be 1 fixes the issue
previously encountered on Android because it was has slightly
different line spacing font metrics than on Ubuntu.
BUG=411038
TEST=webkit_unit_tests --gtest_filter=TouchActionTest.*
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196499
Patch Set 1 #Patch Set 2 : Force font face to be Arial #Patch Set 3 : Remove extra whitespace #
Total comments: 2
Patch Set 4 : Use fixed line height #
Messages
Total messages: 17 (5 generated)
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org
LGTM with nit, though I'm not sure why your change as is causes the test to pass. Does "font-size:20px Arial" actually set the font to Arial? https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... File Source/web/tests/data/touch-action-tests.css (right): https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... Source/web/tests/data/touch-action-tests.css:64: font-size: 20px Arial; I think you want "font:", not "font-size", don't you?
https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... File Source/web/tests/data/touch-action-tests.css (right): https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... Source/web/tests/data/touch-action-tests.css:64: font-size: 20px Arial; On 2015/06/03 17:54:53, tdresser wrote: > I think you want "font:", not "font-size", don't you? If it's text metrics differences causing instability then you may want 'Font: Ahem' - it is designed to have completely stable metrics across all platforms.
On 2015/06/03 18:03:03, Rick Byers wrote: > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > File Source/web/tests/data/touch-action-tests.css (right): > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > Source/web/tests/data/touch-action-tests.css:64: font-size: 20px Arial; > On 2015/06/03 17:54:53, tdresser wrote: > > I think you want "font:", not "font-size", don't you? > > If it's text metrics differences causing instability then you may want 'Font: > Ahem' - it is designed to have completely stable metrics across all platforms. Using the Ahem font doesn't actually fix it. The problem is that when the line height is calculated it is using the font metrics line spacing. It appears the font's line spacing is different across android and linux. Forcing it to a line-height: 1 seems to address the issue.
On 2015/06/03 20:05:34, Dave Tapuska wrote: > On 2015/06/03 18:03:03, Rick Byers wrote: > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > File Source/web/tests/data/touch-action-tests.css (right): > > > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > Source/web/tests/data/touch-action-tests.css:64: font-size: 20px Arial; > > On 2015/06/03 17:54:53, tdresser wrote: > > > I think you want "font:", not "font-size", don't you? > > > > If it's text metrics differences causing instability then you may want 'Font: > > Ahem' - it is designed to have completely stable metrics across all platforms. > > Using the Ahem font doesn't actually fix it. > > The problem is that when the line height is calculated it is using the font > metrics line spacing. It appears the font's line spacing is different across > android and linux. > > Forcing it to a line-height: 1 seems to address the issue. LGTM
On 2015/06/03 20:21:40, tdresser wrote: > On 2015/06/03 20:05:34, Dave Tapuska wrote: > > On 2015/06/03 18:03:03, Rick Byers wrote: > > > > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > > File Source/web/tests/data/touch-action-tests.css (right): > > > > > > > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > > Source/web/tests/data/touch-action-tests.css:64: font-size: 20px Arial; > > > On 2015/06/03 17:54:53, tdresser wrote: > > > > I think you want "font:", not "font-size", don't you? > > > > > > If it's text metrics differences causing instability then you may want > 'Font: > > > Ahem' - it is designed to have completely stable metrics across all > platforms. > > > > Using the Ahem font doesn't actually fix it. > > > > The problem is that when the line height is calculated it is using the font > > metrics line spacing. It appears the font's line spacing is different across > > android and linux. > > > > Forcing it to a line-height: 1 seems to address the issue. > > LGTM Is this maybe just because the page is missing a viewport meta tag? Without wodth=device-width we'll get font boosting on Android, right?
On 2015/06/03 21:03:06, Rick Byers wrote: > On 2015/06/03 20:21:40, tdresser wrote: > > On 2015/06/03 20:05:34, Dave Tapuska wrote: > > > On 2015/06/03 18:03:03, Rick Byers wrote: > > > > > > > > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > > > File Source/web/tests/data/touch-action-tests.css (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > > > Source/web/tests/data/touch-action-tests.css:64: font-size: 20px Arial; > > > > On 2015/06/03 17:54:53, tdresser wrote: > > > > > I think you want "font:", not "font-size", don't you? > > > > > > > > If it's text metrics differences causing instability then you may want > > 'Font: > > > > Ahem' - it is designed to have completely stable metrics across all > > platforms. > > > > > > Using the Ahem font doesn't actually fix it. > > > > > > The problem is that when the line height is calculated it is using the font > > > metrics line spacing. It appears the font's line spacing is different across > > > android and linux. > > > > > > Forcing it to a line-height: 1 seems to address the issue. > > > > LGTM > > Is this maybe just because the page is missing a viewport meta tag? Without > wodth=device-width we'll get font boosting on Android, right? But whatever, LGTM
On 2015/06/03 21:03:55, Rick Byers wrote: > On 2015/06/03 21:03:06, Rick Byers wrote: > > On 2015/06/03 20:21:40, tdresser wrote: > > > On 2015/06/03 20:05:34, Dave Tapuska wrote: > > > > On 2015/06/03 18:03:03, Rick Byers wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > > > > File Source/web/tests/data/touch-action-tests.css (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1162083003/diff/40001/Source/web/tests/data/t... > > > > > Source/web/tests/data/touch-action-tests.css:64: font-size: 20px Arial; > > > > > On 2015/06/03 17:54:53, tdresser wrote: > > > > > > I think you want "font:", not "font-size", don't you? > > > > > > > > > > If it's text metrics differences causing instability then you may want > > > 'Font: > > > > > Ahem' - it is designed to have completely stable metrics across all > > > platforms. > > > > > > > > Using the Ahem font doesn't actually fix it. > > > > > > > > The problem is that when the line height is calculated it is using the > font > > > > metrics line spacing. It appears the font's line spacing is different > across > > > > android and linux. > > > > > > > > Forcing it to a line-height: 1 seems to address the issue. > > > > > > LGTM > > > > Is this maybe just because the page is missing a viewport meta tag? Without > > wodth=device-width we'll get font boosting on Android, right? > > But whatever, LGTM That is what I thought; however I couldn't get my font boosting debug lines to execute. The TextAutoSizer code is complex in terms of it chooses what blocks it operates on; and if the blocks have explicit height it avoids them. All of the blocks in this layout seem to have height; 0px. set... so I don't think it fires on this page; although that was my first thought...
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162083003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162083003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196499 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
