666893 - Windows (ClearType): make all area painted by text be part of text frame's ink overflow area (DirectWrite in GDI mode version)

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1

Followup for bug 475968 to fix the DirectWrite side of things.

The patch in that bug was made under the assumption that only GDI was affected. However, it appears that the problem is not limited to GDI, but also shows up with the 'GDI Classic' rendering mode set in the DirectWrite API. Bug 642589 gave users the ability to set gfx.font_rendering.cleartype_params.rendering_mode = 2 to force DirectWrite to use this rendering mode, and bug 661471 landing recently ensured that commonly used web fonts always use this mode. As such, the fix for bug 475968 should be extended to work for DirectWrite as well.

Attached are the original testcase from bug 475968 that shows this problem, and a screenshot of the problem happening using DirectWrite.

Reproducible: Always

Steps to Reproduce:
1. Run the testcase using DirectWrite with the force_gdi_classic* options enabled.

Actual Results:  
Vertical lines appear between 'Click here!' and 'Enter'.

Expected Results:  
Space between 'Click here!' and 'Enter' should be blank.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: test file to reliably demonstrate the ClearType rendering glitch

Test-case

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: screenshot

Screenshot

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Apologies, I thought I had set it to text/html. Must have mis-clicked.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, add padding pixels because of ClearType bleed when using GDI Classic mode

This essentially duplicates the GDI patch from bug 475968 in the DWrite font backend, but only when we're using GDI Classic rendering mode; in DW Natural mode, there isn't the same tendency for antialiasing to paint significantly outside the nominal ink bounds - or at least it's so slight that we haven't observed problems arising from it.

Pushed to tryserver to see how the tests fare....

Comment 5

[Jonathan Kew [:jfkthame]]

No problems on unit tests (which is as I'd expect, given that we already do the same thing in some other backends). Tryserver build is available at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-544845e4fae7/try-win32/.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 541831 [details] [diff] [review]
patch, add padding pixels because of ClearType bleed when using GDI Classic mode

Review of attachment 541831 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxDWriteFonts.cpp
@@ +627,5 @@
> +    // and the underlying cairo font may be antialiased,
> +    // we can't trust Windows to have considered all the pixels
> +    // so we need to add "padding" to the bounds.
> +    // (see bugs 475968, 439831, compare also bug 445087)
> +    if (aBoundingBoxType == LOOSE_INK_EXTENTS &&

Shouldn't this be aBoundingBoxType != TIGHT_HINTED_OUTLINE_EXTENTS?

Seems like this is needed for TIGHT_INK_EXTENTS as well.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to comment #6)
> Comment on attachment 541831 [details] [diff] [review] [review]
> patch, add padding pixels because of ClearType bleed when using GDI Classic
> mode
> 
> Review of attachment 541831 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxDWriteFonts.cpp
> @@ +627,5 @@
> > +    // and the underlying cairo font may be antialiased,
> > +    // we can't trust Windows to have considered all the pixels
> > +    // so we need to add "padding" to the bounds.
> > +    // (see bugs 475968, 439831, compare also bug 445087)
> > +    if (aBoundingBoxType == LOOSE_INK_EXTENTS &&
> 
> Shouldn't this be aBoundingBoxType != TIGHT_HINTED_OUTLINE_EXTENTS?
> 
> Seems like this is needed for TIGHT_INK_EXTENTS as well.

This matches what we do in the GDI font code. For TIGHT_INK_EXTENTS, padding is added to the glyph extents within cairo, so we don't need to do it here. (See _cairo_dwrite_scaled_font_init_glyph_metrics for the DWrite case, or _cairo_win32_scaled_font_init_glyph_metrics for the GDI equivalent. This has to be done within cairo to avoid clipping the glyph images when they're painted.)

So it's only when we're doing "loose" measurement, so we're not getting actual glyph sidebearings etc. from cairo, that we need to add padding at the gfxFont level in case the glyphs at the edges of the run "bleed" slightly beyond their typographic origin/advance.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

I can confirm that the testcase from comment #1 is fixed in the Tryserver build.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 541831 [details] [diff] [review]
patch, add padding pixels because of ClearType bleed when using GDI Classic mode

Review of attachment 541831 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 10

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/592403064072

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110702 Firefox/7.0a1

Using today's nightly I no longer see the rendering glitches in the testcase from comment #1; marking as verified fixed. Thanks!