Closed Bug 467084 Opened 16 years ago Closed 16 years ago

Downloaded Ahem font does not completely cover background

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: wgianopoulos, Assigned: dbaron)

References

()

Details

(Keywords: fixed1.9.1, regression)

Attachments

(3 files, 2 obsolete files)

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.
Flags: wanted1.9.1?
Flags: blocking1.9.2?
Blocks: 70132
No longer blocks: 441473
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.
Keywords: regression
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).
OS: Windows XP → All
Hardware: PC → All
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+
Blocks: 457821
Summary: Downloaded Ahem font does not completely cover background under Windows → Downloaded Ahem font does not completely cover background
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...
Assignee: nobody → dbaron
Attached patch patch (obsolete) (deleted) — Splinter Review
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.
Attached patch patch (obsolete) (deleted) — Splinter Review
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.
Attachment #350553 - Attachment is obsolete: true
Attachment #350559 - Flags: superreview?(roc)
Attachment #350559 - Flags: review?(roc)
Flags: blocking1.9.1?
(In reply to comment #14) > Created an attachment (id=350559) [details] > patch This patch also appears to fix the issue reported in bug 458863.
Blocks: 458863
I had suspected this might also correct the issue reported in bug 458878. Oddly, it does not.
Component: GFX: Thebes → Layout: Misc Code
QA Contact: thebes → layout.misc-code
+ // 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.
Attached patch patch (deleted) — Splinter Review
Attachment #350559 - Attachment is obsolete: true
Attachment #350690 - Flags: superreview?(roc)
Attachment #350690 - Flags: review?(roc)
Attachment #350559 - Flags: superreview?(roc)
Attachment #350559 - Flags: review?(roc)
Attachment #350691 - Flags: superreview?(roc)
Attachment #350691 - Flags: review?(bzbarsky)
Attachment #350690 - Flags: superreview?(roc)
Attachment #350690 - Flags: superreview+
Attachment #350690 - Flags: review?(roc)
Attachment #350690 - Flags: review+
Attachment #350691 - Flags: superreview?(roc) → superreview+
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P1
Attachment #350691 - Flags: review?(bzbarsky) → review+
Comment on attachment 350691 [details] [diff] [review] additional patch: fix discontinuity in select code r=bzbarsky
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Landed (without the select patch) on 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/770b8eaaec95
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: