Bug 88684 – [Chromium] Sometimes bottom of text is truncated when page has a fractional scale

• Component: Platform
• Status: RESOLVED FIXED

[Xianzhu Wang]

The issue was found on chromium-android. The following code causes the issue:

In SimpleFontData::platformInit() (Source/WebCore/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp)
        SkScalar height = -metrics.fAscent + metrics.fDescent + metrics.fLeading;
        ascent = SkScalarRound(-metrics.fAscent);
        descent = SkScalarRound(height) - ascent;

When the page has a fractional scale, the ascent and descent part of the fonts might be fractional. If the descent part is scaled down, the bottom of the text might be truncated when displayed.

We added the following code to fix the issue:

        // If the descent is rounded down, the descent part of the glyph may be truncated
        // when displayed. To avoid that, borrow 1 unit from the ascent.
        if (descent < SkScalarToFloat(metrics.fDescent) && ascent >= 1) {
            descent++;
            ascent--;
        }

Will create a formal patch soon. It'll need some time to create a layout test for it.

[Xianzhu Wang]

Created attachment 151557 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151557 [details]
Patch

Attachment 151557 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13201141

New failing tests:
css2.1/20110323/absolute-non-replaced-width-002.htm
css2.1/20110323/absolute-non-replaced-width-001.htm
WebViewTest.AutoResizeMinimumSize
css2.1/20110323/absolute-non-replaced-width-003.htm
css2.1/t0803-c5501-imrgn-t-00-b-ag.html
css2.1/t0803-c5502-mrgn-r-01-c-a.html
css2.1/t0804-c5508-ipadn-b-03-b-a.html
css2.1/20110323/absolute-non-replaced-width-007.htm
css2.1/20110323/absolute-non-replaced-height-002.htm
css2.1/t0804-c5507-padn-r-01-c-a.html
css2.1/20110323/absolute-non-replaced-width-004.htm
css2.1/20110323/absolute-non-replaced-width-011.htm
css2.1/20110323/absolute-non-replaced-height-009.htm
css2.1/t0803-c5504-mrgn-l-01-c-a.html
css2.1/20110323/absolute-non-replaced-width-012.htm
css2.1/20110323/absolute-non-replaced-width-010.htm
WebViewTest.AutoResizeMaxSize
css2.1/t0803-c5503-imrgn-b-00-b-a.html
WebViewTest.AutoResizeInBetweenSizes
css2.1/20110323/absolute-non-replaced-width-005.htm
css2.1/20110323/absolute-non-replaced-width-014.htm
css3/zoom-coords.xhtml
css2.1/20110323/absolute-non-replaced-width-013.htm
css2.1/20110323/absolute-non-replaced-height-007.htm
css2.1/t0803-c5505-mrgn-01-e-a.html
css2.1/t0803-c5505-mrgn-03-c-ag.html
css2.1/20110323/absolute-non-replaced-width-006.htm
css2.1/20110323/absolute-non-replaced-width-009.htm
css2.1/20110323/absolute-non-replaced-width-008.htm
css2.1/t0804-c5506-padn-t-00-b-a.html

[WebKit Review Bot]

Created attachment 151563 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Kenichi Ishibashi]

Comment on attachment 151557 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:105
> +        }

The comment on line 88 says that the code is designed to match SimpleFontDataChromiumWin.cpp exactly. Does this patch outdate the comment? If so, please update the comment.

> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page.html:17
> +    <div>pppp</div>

Please add description of the purpose of this test and what is the expectation. Such information is useful for gardeners.

[Xianzhu Wang]

Created attachment 151734 [details]
Patch

[Xianzhu Wang]

Comment on attachment 151557 [details]
Patch

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

The new patch only adjusts descent when subpixel text positioning is enabled. If subpixel is not enabled, the positioning of the text might be shifted to left/up so the bottom truncation might not occur. And this also reduces the effect of existing layout tests.

>> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:105
>> +        }
> 
> The comment on line 88 says that the code is designed to match SimpleFontDataChromiumWin.cpp exactly. Does this patch outdate the comment? If so, please update the comment.

Done.

>> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page.html:17
>> +    <div>pppp</div>
> 
> Please add description of the purpose of this test and what is the expectation. Such information is useful for gardeners.

Done.

[Kenichi Ishibashi]

Comment on attachment 151734 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        * platform/graphics/skia/SimpleFontDataSkia.cpp:

Please update ChangeLog too.

> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:272
> +    if (m_style.useAntiAlias == FontRenderStyle::NoPreference)
> +         m_style.useAntiAlias = useSkiaAntiAlias;
> +
> +    if (!m_style.useHinting)
> +        m_style.hintStyle = SkPaint::kNo_Hinting;
> +    else if (m_style.useHinting == FontRenderStyle::NoPreference)
> +        m_style.hintStyle = skiaHinting;
> +
> +    if (m_style.useBitmaps == FontRenderStyle::NoPreference)
> +        m_style.useBitmaps = useSkiaBitmaps;
> +    if (m_style.useAutoHint == FontRenderStyle::NoPreference)
> +        m_style.useAutoHint = useSkiaAutoHint;
> +    if (m_style.useSubpixelPositioning == FontRenderStyle::NoPreference)
> +        m_style.useSubpixelPositioning = useSkiaSubpixelPositioning;
> +    if (m_style.useAntiAlias == FontRenderStyle::NoPreference)
> +        m_style.useAntiAlias = useSkiaAntiAlias;
> +    if (m_style.useSubpixelRendering == FontRenderStyle::NoPreference)
> +        m_style.useSubpixelRendering = useSkiaSubpixelRendering;

These default values are reasonable? Note that blackberry port uses Skia.

[Xianzhu Wang]

Created attachment 151988 [details]
Patch

[Xianzhu Wang]

Comment on attachment 151734 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        * platform/graphics/skia/SimpleFontDataSkia.cpp:
> 
> Please update ChangeLog too.

Done. Sorry for missing in the last patch.

>> Source/WebCore/platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:272
>> +        m_style.useSubpixelRendering = useSkiaSubpixelRendering;
> 
> These default values are reasonable? Note that blackberry port uses Skia.

The patch doesn't change the default values, but only moves the default value handling code from setupPaint() into querySystemForRenderStyle() so that the new added fontRenderStyle() can get the actual styles. So it should not affect blackberry port.

[Adam Barth]

Comment on attachment 151988 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html: Added.
> +        * platform/chromium/fast/text/descent-clip-in-scaled-page.html: Added.

Is there something chromium-specific about this test?  It seems like it should just go in the cross-platform fast/text directory.

> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14
> +    if (window.layoutTestController)
> +        window.layoutTestController.setTextSubpixelPositioning(true);
> +    if (window.internals)
> +        window.internals.settings.setPageScaleFactor(1.7, 0, 0);

The expected.html file looks exactly the same as the test HTML file.  I don't understand how we're getting value from this test.  Perhaps we need to use a pixel test (rather than a ref test) in this case?

[Xianzhu Wang]

Comment on attachment 151988 [details]
Patch

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

>> LayoutTests/ChangeLog:9
>> +        * platform/chromium/fast/text/descent-clip-in-scaled-page.html: Added.
> 
> Is there something chromium-specific about this test?  It seems like it should just go in the cross-platform fast/text directory.

The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux.

>> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14
>> +        window.internals.settings.setPageScaleFactor(1.7, 0, 0);
> 
> The expected.html file looks exactly the same as the test HTML file.  I don't understand how we're getting value from this test.  Perhaps we need to use a pixel test (rather than a ref test) in this case?

The only difference is 'overflow: hidden' :)
The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div.

[Xianzhu Wang]

Created attachment 152001 [details]
Patch (moved test case from platform/chromium to platform/chromium-linux because it's only applicable to chromium-linux and chromium-android)

[Adam Barth]

> The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux.

I understand that's what the code change is for, but the test seem applicable to all ports.

> >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14
> >> +        window.internals.settings.setPageScaleFactor(1.7, 0, 0);
> > 
> > The expected.html file looks exactly the same as the test HTML file.  I don't understand how we're getting value from this test.  Perhaps we need to use a pixel test (rather than a ref test) in this case?
> 
> The only difference is 'overflow: hidden' :)
> The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div.

Ah, thanks.  Can we add a comment to make that more obvious to dumb people like me?  :)

[Xianzhu Wang]

(In reply to comment #13)
> > The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux.
> 
> I understand that's what the code change is for, but the test seem applicable to all ports.
>

I understand your point, but I'm afraid the test would fail on some ports as subpixel ascent/descent is not supported by all ports. For now my solution is only applicable to chromium-linux/chromium-android when subpixel text positioning is enabled. The test needs the Chromium-only layoutTestController.setSubpixelPositioning() function which only works on chromium-linux and chromium-android to force subpixel text positioning. Otherwise the test might even pass without this patch.

> > >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14
> > >> +        window.internals.settings.setPageScaleFactor(1.7, 0, 0);
> > > 
> > > The expected.html file looks exactly the same as the test HTML file.  I don't understand how we're getting value from this test.  Perhaps we need to use a pixel test (rather than a ref test) in this case?
> > 
> > The only difference is 'overflow: hidden' :)
> > The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div.
> 
> Ah, thanks.  Can we add a comment to make that more obvious to dumb people like me?  :)

(In reply to comment #13)
> > The change is for ports using Skia and HarfBuzz. You reminded me, I should move it into platform/chromium-linux.
> 
> I understand that's what the code change is for, but the test seem applicable to all ports.
> 
> > >> LayoutTests/platform/chromium/fast/text/descent-clip-in-scaled-page-expected.html:14
> > >> +        window.internals.settings.setPageScaleFactor(1.7, 0, 0);
> > > 
> > > The expected.html file looks exactly the same as the test HTML file.  I don't understand how we're getting value from this test.  Perhaps we need to use a pixel test (rather than a ref test) in this case?
> > 
> > The only difference is 'overflow: hidden' :)
> > The test checks if the bottom of text is truncated in overflow:hidden div. If not, it should be displayed the same as in a normal div.
> 
> Ah, thanks.  Can we add a comment to make that more obvious to dumb people like me?  :)

Will do in the next patch, after we agree about where to place the test :)

[Adam Barth]

> I understand your point, but I'm afraid the test would fail on some ports as subpixel ascent/descent is not supported by all ports. For now my solution is only applicable to chromium-linux/chromium-android when subpixel text positioning is enabled. The test needs the Chromium-only layoutTestController.setSubpixelPositioning() function which only works on chromium-linux and chromium-android to force subpixel text positioning. Otherwise the test might even pass without this patch.

That's fine.  We can mark the test as failing on other platforms until they get better and fix their bugs.

I mean, it's not really a big deal where this test goes, but we typically only put tests in the platform directory if there is something intrinsically platform-specific about the test (i.e., not if it's just that other ports aren't able to pass it currently).

[Xianzhu Wang]

(In reply to comment #15)
> 
> That's fine.  We can mark the test as failing on other platforms until they get better and fix their bugs.
> 

The main issue is about the layoutTestController.setSubpixelTextPositioning() call in the test which only exists on Chromium. If I remove the call to make the test generic, then the test won't cover the changed code because subpixel text positioning is disabled by default in DumpRenderTree.

[Adam Barth]

> The main issue is about the layoutTestController.setSubpixelTextPositioning() call in the test which only exists on Chromium. If I remove the call to make the test generic, then the test won't cover the changed code because subpixel text positioning is disabled by default in DumpRenderTree.

That's fine.  There are lots of tests that fail on many ports due to unimplemented LayoutTestController functionality.

[Xianzhu Wang]

Created attachment 152085 [details]
Patch. Make the tests generic. Will update test expectations after try build

[WebKit Review Bot]

Comment on attachment 152085 [details]
Patch. Make the tests generic. Will update test expectations after try build

Attachment 152085 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13200912

[Kenichi Ishibashi]

Comment on attachment 152085 [details]
Patch. Make the tests generic. Will update test expectations after try build

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

LGTM. Please wait formal review.

> LayoutTests/fast/text/descent-clip-in-scaled-page-expected.html:17
> +<body onload="test()">

nit: Remove onload="test()" ?

> LayoutTests/fast/text/descent-clip-in-scaled-page.html:18
> +<body onload="test()">

Ditto.

[Xianzhu Wang]

Created attachment 152309 [details]
Patch

[Xianzhu Wang]

Comment on attachment 152085 [details]
Patch. Make the tests generic. Will update test expectations after try build

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

>> LayoutTests/fast/text/descent-clip-in-scaled-page-expected.html:17
>> +<body onload="test()">
> 
> nit: Remove onload="test()" ?

Done.

>> LayoutTests/fast/text/descent-clip-in-scaled-page.html:18
>> +<body onload="test()">
> 
> Ditto.

Done.

[Adam Barth]

Who should review this change?  Looks like tony@ has review most of the changes to FontPlatformDataHarfBuzz.cpp.

[Tony Chang]

Comment on attachment 152309 [details]
Patch

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

Seems reasonable to me and bashi is happy with it.

> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:104
> +            descent++;
> +            ascent--;

Nit: ++descent and --ascent

[Xianzhu Wang]

Created attachment 152339 [details]
for landing

[Xianzhu Wang]

Comment on attachment 152309 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:104
>> +            ascent--;
> 
> Nit: ++descent and --ascent

Done.

[WebKit Review Bot]

Comment on attachment 152339 [details]
for landing

Clearing flags on attachment: 152339

Committed r122651: <http://trac.webkit.org/changeset/122651>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

(In reply to comment #27)
> (From update of attachment 152339 [details])
> Clearing flags on attachment: 152339
> 
> Committed r122651: <http://trac.webkit.org/changeset/122651>

The new ref test fails on Apple's Mac bots, but my guess is that's expected for ports that disable subpixel layout. Is that right?

[Tony Chang]

(In reply to comment #29)
> (In reply to comment #27)
> > (From update of attachment 152339 [details] [details])
> > Clearing flags on attachment: 152339
> > 
> > Committed r122651: <http://trac.webkit.org/changeset/122651>
> 
> The new ref test fails on Apple's Mac bots, but my guess is that's expected for ports that disable subpixel layout. Is that right?

No, the test is expected to pass on all platforms.  The only difference with the expected file is overflow:hidden, which shouldn't matter because there should be no overflow.

Xianzhu, can you investigate?

[Xianzhu Wang]

(In reply to comment #30)
> (In reply to comment #29)
> > The new ref test fails on Apple's Mac bots, but my guess is that's expected for ports that disable subpixel layout. Is that right?
> 
> No, the test is expected to pass on all platforms.  The only difference with the expected file is overflow:hidden, which shouldn't matter because there should be no overflow.
> 
> Xianzhu, can you investigate?

I think in theory the bottom of text should not be truncated in any case, though my change itself only applies to chromium-linux when subpixel text positioning (a feature of Skia on linux) is enabled. If it fails on Mac, I think we should mark it as failure in expectations/skipped file and file a bug.

[Andy Estes]

Filed <https://bugs.webkit.org/show_bug.cgi?id=91552>.

[Xianzhu Wang]

Created attachment 155634 [details]
Bottom clipped on Android

Just verified that the bug also existed without page scaling. Actually the fractional descent came from the computation of descent for a font without VDMX table, but the clipping is not very visible when the page is not scaled because only few anti-aliased pixels are clipped. When the page is scaled, the solid part of font might be clipped so it becomes visible.