In Acid3 on Mac, after applying the patch for bug 441473 I still get a faint purple line where the Ahem em-square glyph should be covering the background completely. I'll attach a reduced testcase for this (which requires the patch for bug 441473) to work. The basic problem is that we get these metrics in the gfxFont: emHeight = 100, emAscent = 80.198019801980195, emDescent = 19.801980198019805, maxHeight = 101, maxAscent = 81, where emAscent should be exactly 80 and emDescent should be exactly 20. The atsMetrics are ascent = 0.800003052, descent = -0.199996948, The problem is at mMetrics.maxAscent = NS_ceil(atsMetrics.ascent * size); Because ascent > 0.8 (since 0.8 is not exactly representable in machine floating point, probably), this sets maxAscent to 81. From then on problems are inevitable.

Isn't the same as bug 456317 ?

I added NS_ceil there for bug 96041. The reason: our glyph extents cache is optimized for the case where the glyph extents are contained in the box whose top and bottom edges are based on the maxAscent and maxDescent. But ATSGlyphScreenMetrics::height is an unsigned integer, so it's getting rounded somewhere in ATSUI, so I was afraid if we let maxAscent/maxDescent take their natural values a lot of glyphs would stick out of the font-box due to the pixel rounding of their heights. Removing that NS_ceil doesn't seem to cause any problems though; if I instrument the "fall back to tight extents" path properly, we're not hitting it for some simple test pages (planet.mozilla.org, slashdot.org, both tested with zooming out 4 times). philippe: yes, I think it is. Oops. Oh well, this bug has more information now.

This fixes it.

This should be checked in along with John's fonts patch to ensure that Acid3 looks nice.

I've backed out the patch that was landed for this since it was causing a mochitest failure: *** 33285 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_offsets.html | scrollbox scrollHeight - got 125, expected 126

This also broke underline positioning. Some places are relying on maxAscent and maxDescent being an integer number of device pixels. That's desirable because it means line heights will normally be an integer number of device pixels, which will make sure all lines appear to be the same height. So I think the right way to fix this is to keep forcing those to be integers but reorganize the code so that machine precision issues don't change the results when exact arithmetic would produce an integer.

This is better. Round scaled ascent and descent to the nearest 1/1024 of a device pixel before taking NS_ceil.

I have some worries about this test: 1) The biggest problem is that strict file: protocol rules mean that a reftest in bugs/ can't refer to the font in fonts/. Not sure what to about this other than copy Ahem.ttf to every directory where we need it (that's more or less what I've done for images, I guess). 2) I'm not sure what, if anything, guarantees that the document's onload event fires after all fonts have loaded. I think we should be hooking up to the document's loadgroup, but as far as I know, we aren't. We should have a bug about that and deal with it before 1.9.1 ships. Nevertheless the reftest snapshot seems to be taken after the font loads and the test passes for me. 3) If I reload the test pages very quickly many times, font loads start to fail with WARNING: ATSFontGetPostScriptName err = -984: file /Users/roc/mozilla-trunk/gfx/thebes/src/gfxQuartzFontCache.mm, line 160. I think we need a bug about that too.

I also spent quite a lot of time trying to write a test for the regression in underline positioning that we saw when the first version of this patch landed. However, I couldn't figure out how to write a reliable cross-platform reftest.

(In reply to comment #11) > 1) The biggest problem is that strict file: protocol rules mean that a reftest > in bugs/ can't refer to the font in fonts/. Not sure what to about this other > than copy Ahem.ttf to every directory where we need it (that's more or less > what I've done for images, I guess). You mean url(../fonts/Ahem.ttf) isn't permitted? > 2) I'm not sure what, if anything, guarantees that the document's onload event > fires after all fonts have loaded. I think we should be hooking up to the > document's loadgroup, but as far as I know, we aren't. We should have a bug > about that and deal with it before 1.9.1 ships. Nevertheless the reftest > snapshot seems to be taken after the font loads and the test passes for me. The reftests that Akira set up on bug 452870 use <html class="reftest-wait"> and then clear via script it after a timeout. But I think you're right, we need to gate the onload event while fonts are downloading. > 3) If I reload the test pages very quickly many times, font loads start to fail > with > WARNING: ATSFontGetPostScriptName err = -984: file > /Users/roc/mozilla-trunk/gfx/thebes/src/gfxQuartzFontCache.mm, line 160. I > think we need a bug about that too. That's a sanity check, so something in the download process failed. Definitely a bug.

This test might work to ensure that line heights and underline offsets are consistent. Needs to be tested on various platforms. I suppose that if it doesn't work across platforms, we should probably make it (or some improved version of it) work.

> You mean url(../fonts/Ahem.ttf) isn't permitted? Correct.

If reftest uses a separate profile you could just weaken the rules in that profile...

Currently reftest works fine in the default profile and it would troublesome to break that.

Print Preview under Windows also positions the font incorrectly. Turning on colours and backgrounds helps make this more obvious. I couldn't figure out how to turn those on under OSX to compare :-/

Pushed d7bd49d81084.