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

VERIFIED FIXED

Status

()

Core
Layout: Text
P3
major
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: ehoogeveen, Assigned: jfkthame)

Tracking

Trunk
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 541647 [details]
test file to reliably demonstrate the ClearType rendering glitch

Test-case
(Reporter)

Comment 2

6 years ago
Created attachment 541648 [details]
screenshot

Screenshot
(Reporter)

Updated

6 years ago
Severity: normal → major
Priority: -- → P3
(Reporter)

Updated

6 years ago
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Attachment #541647 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 3

6 years ago
Apologies, I thought I had set it to text/html. Must have mis-clicked.
(Assignee)

Updated

6 years ago
Assignee: nobody → jfkthame
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

6 years ago
Created attachment 541831 [details] [diff] [review]
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....
Attachment #541831 - Flags: review?(roc)
(Assignee)

Comment 5

6 years ago
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/jkew@mozilla.com-544845e4fae7/try-win32/.
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.
(Assignee)

Comment 7

6 years ago
(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.
(Reporter)

Comment 8

6 years ago
I can confirm that the testcase from comment #1 is fixed in the Tryserver build.
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]:
-----------------------------------------------------------------
Attachment #541831 - Flags: review?(roc) → review+
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/mozilla-central/rev/592403064072
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

6 years ago
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.