This testcase from bug 457194, https://bugzilla.mozilla.org/attachment.cgi?id=340544, fails under Windows trunk builds. If I install the Ahem font on my system and modify the test to use the installed font rather than @font-face, the test passes.

What date trunk builds?

In particular, I want to make sure that you're testing in builds that have both of these: http://hg.mozilla.org/mozilla-central/rev/cf88eac1c192 http://hg.mozilla.org/mozilla-central/rev/77825d347650 ... please note the push times in the "pushlog" links.

(In reply to comment #2) > In particular, I want to make sure that you're testing in builds that have both > of these: > http://hg.mozilla.org/mozilla-central/rev/cf88eac1c192 > http://hg.mozilla.org/mozilla-central/rev/77825d347650 > ... please note the push times in the "pushlog" links. I am seeing this issue with the latest trunk nightly. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081128 Minefield/3.1b3pre ID:20081128040718 Built from http://hg.mozilla.org/mozilla-central/rev/527249758771 That should include both of those. I also saw this issue with the last build from yesterday. I have not tested earlier builds to see if it has a recent cause. Oddly, this does not fail under Acid3.

It appears this is a recent regression. I am tracking down the regression range now.

It fails equally on Mac. The white square is 2px higher than it should be (that is: there is a 2px tall pink line under it).

Both the white square and the pink rectangle should be 100px high. What heights are they actually? Or is it just that they're misaligned relative to each other?

This makes both boxes non-white.

... and it looks like it's an issue of misalignment. I could imagine this happening because in the old code, the font metrics of the downloaded font didn't get used at all, and now that bug 457821 is fixed they've gotten through to some but not all parts of the layout. (Bug 458169 comment 13 blames bug 457821 for this bug, but that wasn't mentioned here for some reason.)

(In reply to comment #8) comment 13 blames bug 457821 for this bug, but that wasn't > mentioned here for some reason.) I was waiting for my hg bisect work to finish to narrow it down to an individual changeset.

And the winner is: The first bad revision is: changeset: 21916:cf88eac1c192 user: L. David Baron <dbaron@dbaron.org> date: Tue Nov 25 15:22:39 2008 -0800 summary: Check that the user font set matches before returning an entry from the font cache. (Bug 457821) r=jdaggett sr=roc a=blocking1.9.1+

It appears that reverting just the chage to this if confdition: diff -r 95803938905c gfx/src/nsDeviceContext.cpp --- a/gfx/src/nsDeviceContext.cpp Fri Nov 28 12:12:15 2008 +0100 +++ b/gfx/src/nsDeviceContext.cpp Fri Nov 28 20:34:42 2008 -0500 @@ -479,17 +479,17 @@ // First check our cache // start from the end, which is where we put the most-recent-used element nsIFontMetrics* fm; PRInt32 n = mFontMetrics.Count() - 1; for (PRInt32 i = n; i >= 0; --i) { fm = static_cast<nsIFontMetrics*>(mFontMetrics[i]); nsIThebesFontMetrics* tfm = static_cast<nsIThebesFontMetrics*>(fm); - if (fm->Font().Equals(aFont) && tfm->GetUserFontSet() == aUserFontSet) { + if (fm->Font().Equals(aFont)) { nsCOMPtr<nsIAtom> langGroup; fm->GetLangGroup(getter_AddRefs(langGroup)); if (aLangGroup == langGroup.get()) { if (i != n) { // promote it to the end of the cache mFontMetrics.MoveElement(i, n); } tfm->GetThebesFontGroup()->UpdateFontList(); is sufficient to avoid this issue. However, from the checkin comment, it would appear that if change was the major point of the patch.

The problem here is that the user font set isn't getting passed through the relevant code path at all. (nsLayoutUtils::SetFontFromStyle -> nsRenderingContext::SetFont -> nsDeviceContext::GetMetricsFor) I'll try to cough up a patch to pass it through...

This fixes it. I still need to test that the test works on all platforms. I might need to allow for a pixel of error for antialiasing, although I hope not. Mostly this is pretty straightforward. I made the aUserFontSet parameter to some methods mandatory where it was optional before, and in those cases moved it before the out-parameter to follow the convention that the out parameter goes last. In a handful of changes, I did something a little more substantive: * in accessible/src/html/nsHyperTextAccessible.cpp I removed the creation and setting of a font on a rendering context that was otherwise unused. I suspect it's for an old way of doing things. The alternative would have been to make either nsPresContext::GetUserFontSet or nsPresContext::GetMetricsFor callable outside of layout, neither of which I wanted to do, and this turned out to be very easy. * I made nsMathMLContainerFrame pass the langGroup of the pres context through for its ReflowError stuff (but I didn't add changes to pass the lang group through for any other MathML code) * I did the same for nsListControlFrame, by using nsLayoutUtils. * In nsPageFrame, I avoided doing the same GetMetricsFor twice by using the other SetFont variant.

The reftest passes on Linux (with karlt's patch), Mac, and Windows, but I needed one extra change to accessible/ to get the patch to compile on Windows.

(In reply to comment #14) > Created an attachment (id=350559) [details] > patch This patch also appears to fix the issue reported in bug 458863.

I had suspected this might also correct the issue reported in bug 458878. Oddly, it does not.

+ // It seems like we want to pass null for the user font set since this + // is a font family name that we're going to give to somebody else. + // That said, this forces creation of an extra font metrics object + // that wouldn't otherwise need to be created. + if (NS_FAILED(rc->SetFont(font->mFont, visibility->mLangGroup, nsnull))) { Shouldn't we pass the user font set here? I don't see why we'd want to return an incorrect font family name. OK, whoever's calling this API won't be able to take that name and actually use that font, but is it really better to lie? if (aNumOptions > 0) { // Try the first option nsCOMPtr<nsIContent> option = GetOptionContent(0); if (option) { - nsIFrame * optFrame = PresContext()->PresShell()-> + nsIFrame * fontFrame = PresContext()->PresShell()-> GetPrimaryFrameFor(option); - if (optFrame) { - styleFont = optFrame->GetStyleFont(); - } } } You've introduced a bug here --- you're shadowing the definition of fontFrame so this entire conditional is effectively a no-op. Can you add a test for this path?

> You've introduced a bug here --- you're shadowing the definition of fontFrame > so this entire conditional is effectively a no-op. Can you add a test for this > path? So I don't want to add a test for it, since as far as I can tell the only code that depends on it is buggy. Instead, I want to remove that codepath. In particular, the only codepath that hits that bug is nsListControlFrame::CalcHeightOfARow. All other callers pass 0 to CallFallbackRowHeight. The code in CalcHeightOfARow only calls CallFallbackRowHeight if GetMaxOptionHeight returns 0. The original rationale for this (expressed in a comment introduced in bug 32383 and removed in bug 314879) was to deal with empty options. However, options have had min-height:1em since 1999: http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/style/ua.css&rev=3.179 which is before this code was introduced by rods: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/forms&command=DIFF_FRAMESET&file=nsListControlFrame.cpp&rev2=1.135&rev1=1.134 to fix bug 32383. So as far as I can tell all that code does is introduce a discontinuity when authors switch from: option { min-height: 0; height: 0.05px; } to: option { min-height: 0; height: 0; } I'll fix the bug in my own patch and then post a followup patch to fix the discontinuity.

Comment on attachment 350691 [details] [diff] [review] additional patch: fix discontinuity in select code r=bzbarsky

Fixed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/f7e51a5e6655 http://hg.mozilla.org/mozilla-central/rev/045908bd6528 Still needs to land on 1.9.1.

Landed (without the select patch) on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/770b8eaaec95