need to be able to get a "tight" bounding box that ignores ClearType pixels

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

11 years ago
Once bug #445087 is landed, the "tight" bounding box of text frames will include a couple of potential ClearType antialiasing pixels that extend beyond the perceived visual edges of the glyphs. This is necessary to avoid clipping ClearType-smoothed text, as reported in that bug.

Unfortunately, this disrupts MathML layout, because the MathML code uses the bounding box to decide the relative positioning/spacing of elements. As a result, glyphs become much too spread-out, especially at small sizes where the additional couple of pixels are a significant amount relative to the overall spacing. See attachment #361625 [details] (original MathML display before correcting the ClearType clipping issue) and attachment #361626 [details] (after ClearType padding is added to the glyph bounds).

To fix this, we need to differentiate two kinds of "tight bounds" for text, one that includes every touched pixel (to avoid the ClearType clipping issue), and another that corresponds to the perceived shape that should be considered for math spacing.
Assignee

Comment 1

11 years ago
This patch modifies gfxFont::Measure so that instead of a boolean (tight or non-tight bounds), it takes an option for the kind of bounds required. The options are specified as an enum, with LOOSE_INK_EXTENTS corresponding to the old "false" for tight bounds, TIGHT_INK_EXTENTS corresponding to the old "true" (but note that the ClearType padding from bug #445087 changed the results of this on Windows), and TIGHT_HINTED_OUTLINE_EXTENTS providing extents that ignore the potential antialiasing pixels, for use in MathML spacing.

Currently, only the gfxWindowsFont implementation actually makes a distinction between the two kinds of TIGHT extents, as this has been a problem only with ClearType antialiasing.

Note that the implementation of TIGHT_HINTED_OUTLINE_EXTENTS is relatively inefficient, as it instantiates a new temporary gfxFont (and associated cairo font) each time it is used; this is to avoid returning the wrong cached extents from the gfxFont. If we ever need to make heavy use of this option, we could consider caching this instead of creating and discarding it each time, but at present MathML and first-letter are the only users of this option.
Assignee

Updated

11 years ago
Attachment #363160 - Flags: review?(roc)
Comment on attachment 363160 [details] [diff] [review]
initial patch to implement different kinds of "tight bounds"

+            // NB: The default implementation does not differentiate
+            // between this and TIGHT_INK_EXTENTS, but Windows
+            // needs to because of ClearType.

We should really be distinguishing TIGHT_INK_EXTENTS and TIGHT_HINTED_OUTLINE_EXTENTS on all platforms, right? So this comment should say that and just say that currently Windows is the only platform on which we currently do distinguish them.

+        gfxFont *tempFont = new gfxWindowsFont(GetFontEntry(), GetStyle(),
+                                               CAIRO_ANTIALIAS_NONE);

Use nsRefPtr to avoid 'delete', and check for null.
Attachment #363160 - Flags: review?(roc) → review+
Assignee

Comment 3

11 years ago
Should we take this for 1.9.1, because of the link with bug #445087, which is itself a 1.9.1 blocker? When that lands, it will cause a regression in MathML layout on Windows (as shown in comments and attachments there), which this patch resolves. Not sure if MathML spacing is important enough to make this block, or should it merely be "wanted"?
Attachment #363160 - Attachment is obsolete: true
Attachment #363803 - Flags: superreview?(dbaron)
Is there a particular reason you asked me to review this?  I'm not particularly knowledgeable about this code, so I'd think roc's review would be sufficient here.
Attachment #363803 - Flags: superreview?(dbaron) → superreview+
Your opinion on 1.9.1-worthiness would still be useful though.

I think we should take it on 1.9.1.
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/6e6ce14a0c64
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Assignee

Comment 7

11 years ago
(In reply to comment #4)
> Is there a particular reason you asked me to review this?  I'm not particularly
> knowledgeable about this code, so I'd think roc's review would be sufficient
> here.

Sorry to bother you! Just trying to get the process right, as I'm still fairly new around here. I had the impression that r and sr are generally supposed to be different people; and although this is a pretty straightforward patch it does touch files in content/ and layout/ as well as gfx/, as it's changing an interface. Roc and I had discussed the strategy on irc previously, so requesting sr from elsewhere seemed more meaningful.

Updated

11 years ago
Depends on: 480906
Is this already covered by tests?  Drivers are wondering...
Flags: in-testsuite?
We don't have tests for this specifically. But actually we don't need this on 1.9.1 since we decided not to fix bug 445087 for 1.9.1.
Flags: in-testsuite? → in-testsuite-
Whiteboard: [needs 191 approval]
Is it impossible to test in an automated fashion (trying to understand why in-testsuite was set to -)
This is not a content-facing change, so it would have to be some kind of unit test, and a fragile one at that since exact behaviour depends on fonts and platform font rasterizers.
You need to log in before you can comment on or make changes to this bug.