941611 - Incorrect preferred width of italic characters

Status: RESOLVED FIXED

(Core :: MathML, defect, P5)

Description

[Frédéric Wang (:fredw)]

Follow-up of bug 415413.

table-width-3.html fails on Mac and Windows 7 and 8. However I can't see the bug on my Windows 7 virtual machine.

See attachment 8335148 [details] for a testcase.

Attachment 8335333 [details] shows the incorrect rendering on Mac.

Comment 1

[Frédéric Wang (:fredw)]

Based on the discussion on bug 941607 and bug comment 96, I'm wondering if this has something to do with the interframe spacing code... However, this works for me on Windows and Linux so I can't really test it.

Comment 2

[Frédéric Wang (:fredw)]

James has also been able to reproduce this on Windows 8.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Frédéric Wang (:fredw) from comment #0)
> Follow-up of bug 415413.
> 
> table-width-3.html fails on Mac and Windows 7 and 8. However I can't see the
> bug on my Windows 7 virtual machine.

Your Win7 VM probably doesn't use hardware acceleration, so you get GDI font rendering and no fractional-pixel glyph widths. You might find that setting gfx.font_rendering.directwrite.enabled to true (so that you get DW font rendering even without h/w acc) will enable you to reproduce the issue.

Comment 4

[Jonathan Kew [:jfkthame]]

On OS X, at least, a bit of tracing shows something curious: the <mi> elements containing a single "f" report an intrinsic width (in nsLayoutUtils::IntrinsicForContainer) of 1647 appunits (or 27.45px), but nsMathMLTokenFrame::Reflow for those "f"s returns aDesiredSize with width 1649 appunits (27.483333px).

I haven't had time yet to track down why they're measuring different widths, but I suspect this discrepancy is the root of the problem: we're calculating a width for the table cell using the 1647-appunit width of the <mi>s, but then laying out the cell's content using the 1649-appunit width, and it doesn't quite fit.

Comment 5

[James Kitchener (:jkitch)]

Got it.  The cause is rounding.  

nsMathMLTokenFrame::Reflow() ultimately calls nsTextFrame::ComputeTightBounds() which rounds up.
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#7336
(RoundOut function is defined immediately above ComputeTightBounds)

nsMathMLContainerFrame::GetMinWidth() ultimately calls nsTextFrame::GetPrefWidthTightBounds() which rounds down.
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#7359

The question is which rounding is correct.  

I'll submit try runs later tonight with each of the possible cases to see it test failures indicate the solution.

Comment 6

[Jonathan Kew [:jfkthame]]

That's at least part of it; I think this may also be a problem:
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLFrame.h#183

When we add the result of aBoundingMetrics.rightBearing - aBoundingMetrics.width to the character's advance, each of these values having been rounded to appunits separately, the result may not match the GetPrefWidthTightBounds() that was based on directly rounding (whether it's up or down) the XMost of the glyph's rect. This isn't just a question of the kind of rounding, but of rounding (to appunits) happening too early in a calculation, leading to an accumulation of rounding errors.

Comment 7

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

(In reply to James Kitchener (:jkitch) from comment #5)
> nsMathMLContainerFrame::GetMinWidth() ultimately calls
> nsTextFrame::GetPrefWidthTightBounds() which rounds down.
> https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.
> cpp#7359

This should round up.

Comment 8

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> That's at least part of it; I think this may also be a problem:
> http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLFrame.
> h#183
> 
> When we add the result of aBoundingMetrics.rightBearing -
> aBoundingMetrics.width to the character's advance, each of these values
> having been rounded to appunits separately, the result may not match the
> GetPrefWidthTightBounds() that was based on directly rounding (whether it's
> up or down) the XMost of the glyph's rect. This isn't just a question of the
> kind of rounding, but of rounding (to appunits) happening too early in a
> calculation, leading to an accumulation of rounding errors.

Perhaps we could add special cases to determine the italic correction more accurately when possible (i.e. for text frames here)?  We could also use the MATH table later for individual glyphs and stretchy operators (cf bug 407059, where I tried to compute the advance width because otherwise the italic correction is 0 and the scripts are not attached to largeop)

Comment 9

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Your Win7 VM probably doesn't use hardware acceleration, so you get GDI font
> rendering and no fractional-pixel glyph widths. You might find that setting
> gfx.font_rendering.directwrite.enabled to true (so that you get DW font
> rendering even without h/w acc) will enable you to reproduce the issue.

That does not help to reproduce the issue for me.

Comment 10

[Frédéric Wang (:fredw)]

Attachment: Make nsTextFrame::GetPrefWidthTightBounds round up

So do you get better results by rounding up in GetPrefWidthTightBounds?

I'm wondering whether the right way to solve Jonathan's problem would be to add to nsBoundingMetrics a new nscoord member to store the italic correction. In general, this will be aBoundingMetrics.rightBearing - aBoundingMetrics.width but for MathML text we could refine this and compute it at a lower level. For example by computing mBoundingBox.XMost() - mAdvanceWidth or, for some glyphs, by directly picking the value from the MATH table when it is specified.

Comment 11

[James Kitchener (:jkitch)]

The patch is an improvement, in that the <mi>f</mi> elements no longer overflow onto the next line, but table-width-3 and -4 do not yet pass.

Try run from a few days ago.
https://tbpl.mozilla.org/?tree=Try&rev=c9695a212ef3

The extreme left and extreme right of the italic f characters encroach slightly onto the white cell outline.  Changing the zoom level appears to affect the visibility of the encroachment.

(Curiously enough, table-width-1 also fails for me locally in this manner, but not on tryserver).

Comment 12

[Frédéric Wang (:fredw)]

(In reply to James Kitchener (:jkitch) from comment #11)
> The patch is an improvement

That's great. So I guess this means Jacques Distler's original testcase renders as expected, right?

https://bug415413.bugzilla.mozilla.org/attachment.cgi?id=301094

If that does not cause any new problems then I think we can take that small change asap. The unexpected line wrapping and significant overflow were the the most serious issues of bug 415413. The small overflow detected by the reftest won't be noticeable by users.

@Jacques: can you please try again with http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c9695a212ef3/try-macosx64/

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to James Kitchener (:jkitch) from comment #11)
> The patch is an improvement, in that the <mi>f</mi> elements no longer
> overflow onto the next line, but table-width-3 and -4 do not yet pass.
> 
> Try run from a few days ago.
> https://tbpl.mozilla.org/?tree=Try&rev=c9695a212ef3
> 
> The extreme left and extreme right of the italic f characters encroach
> slightly onto the white cell outline.  Changing the zoom level appears to
> affect the visibility of the encroachment.

This is an artifact of the glyph antialiasing. Depending on lots of factors - platform, font, size, display settings, etc. - the antialiased rasterization of a glyph may end up "bleeding" slightly outside the nominal bounding box and touching an adjacent column of pixels at the extreme left and/or right.

In some testcases, we've worked around this type of issue by adding a couple of pixels of CSS padding to the element with the text, so that if antialiasing touches any pixels just outside the glyph bounding box, they'll still be within the frame's padding box and avoid affecting the rest of the test. Maybe we can do that for these testcases, provided there's a way to add such padding without affecting what we actually want to test.

Comment 14

[Frédéric Wang (:fredw)]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> Maybe we can do that for these testcases, provided there's a way to
> add such padding without affecting what we actually want to test.

The goals of these tests is to check that the preferred width computation of various MathML elements is correct by verifying that the equations don't overflow. I believe we can just add some 1px padding to the table cells in order to tolerate a small antialiasing error.

Comment 15

[Frédéric Wang (:fredw)]

New Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=f26634ad27f6

Comment 16

[James Kitchener (:jkitch)]

Attachment: Make nsTextFrame::GetPrefWidthTightBounds round up

No code changes since the previous patch, just a comment warning not to change it back.

Left and right padding of one pixel has been added to table-width-1, -3 and -4 and the latter two are now tested on all platforms.  I added it to -1 as well because it causes test failures on my specific test environment, even if it doesn't cause any on try.

(In reply to Frédéric Wang (:fredw) from comment #12)
> That's great. So I guess this means Jacques Distler's original testcase
> renders as expected, right?
> 

It does (on Windows at least).

Comment 17

[Frédéric Wang (:fredw)]

Comment on attachment 8362027 [details] [diff] [review]
Make nsTextFrame::GetPrefWidthTightBounds round up

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

Looks good to me, thanks.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c60f3e28cbb9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c60f3e28cbb9