467084 - Downloaded Ahem font does not completely cover background

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Bill Gianopoulos [:WG9s]]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What date trunk builds?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Bill Gianopoulos [:WG9s]]

(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.

Comment 4

[Bill Gianopoulos [:WG9s]]

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

Comment 5

[philippe (part-time)]

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).

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase that might be easier to debug

This makes both boxes non-white.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... 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.)

Comment 9

[Bill Gianopoulos [:WG9s]]

(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.

Comment 10

[Bill Gianopoulos [:WG9s]]

And the winner is:

The first bad revision is:
changeset:   21916:cf88eac1c192
user:        L. David Baron <[e-mail]>
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+

Comment 11

[Bill Gianopoulos [:WG9s]]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 15

[Bill Gianopoulos [:WG9s]]

(In reply to comment #14)
> Created an attachment (id=350559) [details]
> patch

This patch also appears to fix the issue reported in bug 458863.

Comment 16

[Bill Gianopoulos [:WG9s]]

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

Comment 17

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

+  // 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?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> 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 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: additional patch: fix discontinuity in select code

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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