Bug 91386 - Layout Test fast/text/descent-clip-in-scaled-page.html is failing on linux since it was added
Summary: Layout Test fast/text/descent-clip-in-scaled-page.html is failing on linux si...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-16 07:05 PDT by Vsevolod Vlasov
Modified: 2012-08-01 09:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.97 KB, patch)
2012-07-31 13:54 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
for landing (7.57 KB, patch)
2012-07-31 14:16 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cq-02 (562.31 KB, application/zip)
2012-07-31 16:49 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2012-07-16 07:05:24 PDT
The following layout test is failing on linux

fast/text/descent-clip-in-scaled-page.html

Probable cause:

http://trac.webkit.org/changeset/122651/
Comment 2 Vsevolod Vlasov 2012-07-16 07:08:24 PDT
Skipped: http://trac.webkit.org/changeset/122721
Comment 3 Tony Chang 2012-07-16 10:24:51 PDT
wangxianzhu, can you investigate?  I wonder how it passed the EWS bots . . .
Comment 4 Xianzhu Wang 2012-07-16 10:29:27 PDT
(In reply to comment #3)
> wangxianzhu, can you investigate?  I wonder how it passed the EWS bots . . .

OK. I might be flaky.
Comment 5 Tony Chang 2012-07-16 10:32:14 PDT
Why would it be flaky?

It may be possible that we're not resetting fontconfig properly between tests. You may be able to repro by running other tests before your test.
Comment 6 Tony Chang 2012-07-19 10:45:34 PDT
Xianzhu, is it possible that the expected result is wrong?  Doesn't your patch move the baseline up a pixel?

This test is consistently failing.  If we can't figure out why the test is failing, I think we should rollout the whole patch since it's not doing what we're expecting.
Comment 7 Tony Chang 2012-07-19 10:57:35 PDT
(In reply to comment #6)
> Xianzhu, is it possible that the expected result is wrong?  Doesn't your patch move the baseline up a pixel?

Actually, that's not what's happening since only the bottom line is different.

If I understand your patch correctly, it only moves the baseline for glyphs that overflow. Does that result in words with a different baseline for a few letters?

Also, it looks like the test is failing on Mac because it also does subpixel text rendering and the bottom of the text (a line of grey pixels) is clipped. I'm starting to think this workaround for only Chromium Linux & Android is incorrect.
Comment 8 Tony Chang 2012-07-19 10:58:53 PDT
For reference, the original bug is https://bugs.webkit.org/show_bug.cgi?id=88684 .
Comment 9 Xianzhu Wang 2012-07-19 11:19:16 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Xianzhu, is it possible that the expected result is wrong?  Doesn't your patch move the baseline up a pixel?
> 
> Actually, that's not what's happening since only the bottom line is different.
> 
> If I understand your patch correctly, it only moves the baseline for glyphs that overflow. Does that result in words with a different baseline for a few letters?
>

The baseline movement applies to all glyphs of a particular font displayed in the same size. The similar patch has been in chromium-android downstream for nearly one year (without the layout test) and it does resolve the issue of font bottom truncation.

I guess the failure on chromium-linux is caused by layout before layoutTestController.setSubpixelTextPositioning(). I'll verify that and if it is, fix the test.

> Also, it looks like the test is failing on Mac because it also does subpixel text rendering and the bottom of the text (a line of grey pixels) is clipped. I'm starting to think this workaround for only Chromium Linux & Android is incorrect.

The method of the change might also applies to Mac, but the change itself applies only to chromium-linux and chromium-android because it is Skia and Harfbuzz specific. For Mac it needs another fix in its graphics code. However I think the final fix should be WebCore's support of sub-pixel ascent/descent.
Comment 10 Tony Chang 2012-07-26 13:36:26 PDT
(In reply to comment #9)
> I guess the failure on chromium-linux is caused by layout before layoutTestController.setSubpixelTextPositioning(). I'll verify that and if it is, fix the test.

Xianzhu, have you had a chance to investigate this?
Comment 11 Xianzhu Wang 2012-07-26 13:44:40 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > I guess the failure on chromium-linux is caused by layout before layoutTestController.setSubpixelTextPositioning(). I'll verify that and if it is, fix the test.
> 
> Xianzhu, have you had a chance to investigate this?

Sorry for no progress by now. Hopefully I will get a chance in this or next week.
Comment 12 Tony Chang 2012-07-26 13:51:47 PDT
Out of curiosity, how does iOS handle this?
Comment 13 Xianzhu Wang 2012-07-31 13:54:08 PDT
Created attachment 155628 [details]
Patch
Comment 14 Xianzhu Wang 2012-07-31 13:54:50 PDT
Comment on attachment 155628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155628&action=review

> LayoutTests/fast/text/descent-clip-in-scaled-page-expected.html:10
> +       enabled. */

Note: these comments were copied from platform/chromium-linux/fast/text/chromium-linux-text-subpixel-positioning.html.
Comment 15 Tony Chang 2012-07-31 14:03:30 PDT
Comment on attachment 155628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155628&action=review

>> LayoutTests/fast/text/descent-clip-in-scaled-page-expected.html:10
>> +       enabled. */
> 
> Note: these comments were copied from platform/chromium-linux/fast/text/chromium-linux-text-subpixel-positioning.html.

Nit: It doesn't really seem necessary to copy this comment into the -expected.html file.
Comment 16 Tony Chang 2012-07-31 14:03:51 PDT
Did you find anything out about iOS?
Comment 17 Xianzhu Wang 2012-07-31 14:06:49 PDT
(In reply to comment #12)
> Out of curiosity, how does iOS handle this?

Though the bug is visible on chromium-linux using the Ahem font, it is invisible when using other normal fonts. It seems that the other fonts has some spacing below the descent part of the glyphs within the line box. The descent in the font metrics is larger than the solid descent shown in the glyph images.

The fonts on Android, just like the Ahem font, seem to have no or small vertical spacing below the descent, so it's visible when the descent is clipped.

Wondering if iOS has its own solution to the issue, or just because its fonts are not like the Android fonts.
Comment 18 Xianzhu Wang 2012-07-31 14:16:13 PDT
Created attachment 155633 [details]
for landing
Comment 19 Xianzhu Wang 2012-07-31 14:17:16 PDT
Comment on attachment 155628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155628&action=review

>>> LayoutTests/fast/text/descent-clip-in-scaled-page-expected.html:10
>>> +       enabled. */
>> 
>> Note: these comments were copied from platform/chromium-linux/fast/text/chromium-linux-text-subpixel-positioning.html.
> 
> Nit: It doesn't really seem necessary to copy this comment into the -expected.html file.

Done.
Comment 20 Xianzhu Wang 2012-07-31 14:26:28 PDT
(In reply to comment #17)
> (In reply to comment #12)
> > Out of curiosity, how does iOS handle this?
> 
> Though the bug is visible on chromium-linux using the Ahem font, it is invisible when using other normal fonts. It seems that the other fonts has some spacing below the descent part of the glyphs within the line box. The descent in the font metrics is larger than the solid descent shown in the glyph images.
> 
> The fonts on Android, just like the Ahem font, seem to have no or small vertical spacing below the descent, so it's visible when the descent is clipped.
> 
> Wondering if iOS has its own solution to the issue, or just because its fonts are not like the Android fonts.

Another factor is the VDMX table in the font. The problematic rounding of ascent/descent only occurs when the font doesn't contain the table (which is the case for some Android fonts and the Ahem font).
Comment 21 Tony Chang 2012-07-31 14:43:45 PDT
(In reply to comment #20)
> (In reply to comment #17)
> > The fonts on Android, just like the Ahem font, seem to have no or small vertical spacing below the descent, so it's visible when the descent is clipped.
> > 
> > Wondering if iOS has its own solution to the issue, or just because its fonts are not like the Android fonts.
> 
> Another factor is the VDMX table in the font. The problematic rounding of ascent/descent only occurs when the font doesn't contain the table (which is the case for some Android fonts and the Ahem font).

Maybe this is a bug in the Android fonts?  How does layout using the native Android SDK (i.e., the Java API) handle this?
Comment 22 Xianzhu Wang 2012-07-31 14:51:49 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #17)
> > > The fonts on Android, just like the Ahem font, seem to have no or small vertical spacing below the descent, so it's visible when the descent is clipped.
> > > 
> > > Wondering if iOS has its own solution to the issue, or just because its fonts are not like the Android fonts.
> > 
> > Another factor is the VDMX table in the font. The problematic rounding of ascent/descent only occurs when the font doesn't contain the table (which is the case for some Android fonts and the Ahem font).
> 
> Maybe this is a bug in the Android fonts?  How does layout using the native Android SDK (i.e., the Java API) handle this?

I don't think it's a bug of Android fonts. There are just differences. The problem is that the descent was rounded and caused problems when displaying Android fonts. Android just doesn't round the descent so there is no problem.
Comment 23 WebKit Review Bot 2012-07-31 16:49:36 PDT
Comment on attachment 155633 [details]
for landing

Rejecting attachment 155633 [details] from commit-queue.

New failing tests:
platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html
Full output: http://queues.webkit.org/results/13387958
Comment 24 WebKit Review Bot 2012-07-31 16:49:39 PDT
Created attachment 155675 [details]
Archive of layout-test-results from gce-cq-02

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: gce-cq-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 25 Xianzhu Wang 2012-08-01 09:14:48 PDT
Comment on attachment 155633 [details]
for landing

Try CQ again. I think the failure was just flakyness unrelated to this change.
Comment 26 WebKit Review Bot 2012-08-01 09:55:10 PDT
Comment on attachment 155633 [details]
for landing

Clearing flags on attachment: 155633

Committed r124340: <http://trac.webkit.org/changeset/124340>
Comment 27 WebKit Review Bot 2012-08-01 09:55:14 PDT
All reviewed patches have been landed.  Closing bug.