633299 - Adding new @font-face declarations causes existing fonts to briefly revert to core fonts

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Dylan Tack]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-us) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4
Build Identifier: 4.0b12pre (2001-02-10)

If a new @font-face declaration is added with javascript after the page has already loaded, any existing fonts briefly flash as they revert to a built-in font.

It my testing, the fix committed for the "FOUT" at https://bugzilla.mozilla.org/show_bug.cgi?id=499292 doesn't address this issue - even after rebuilding Firefox from mercurial the flash still occurs.

Reproducible: Always

Steps to Reproduce:
1.View the attached test case.
2.Click the button.
Actual Results:  
When the button is clicked, the H1 element "Demo" flashes the default font (Times).

Expected Results:  
The H1 element is not expected to flash.  Note that the new style rules being loaded only target H2, and not H1, and do not alter the first @font-face declaration. 

This was discovered while working on a site designed to browse a large font library, so the obvious workaround of loading all @font-face declarations statically (i.e. without javascript) isn't possible.

Comment 1

[Dylan Tack]

Attachment: Test case which demonstrates the flash

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, that seems odd.  Do we discard the old font set as soon as we see the new @font-face, and not after the font loads?

Comment 3

[Sean McBride]

I've run into this bug as well. I made my own test case which you can find here: http://seanmcb.com/typekit/firefox-double-fout.html

In my experience, the flash doesn't show up consistently, but it does show up pretty frequently. It makes it hard to make any sort of font previewing interface where fonts are loaded into the page as they're needed (since every other web font on the page will briefly flash to a fallback font).

Comment 4

[Jonathan Kew [:jfkthame]]

This happens because whenever the collection of @font-face rules changes, the code in nsPresContext::FlushUserFontSet() will create a new font set and insert a new set of entries into it, which means we have a fresh new set of gfxProxyFontEntry objects. Then we reflow, and font resolution finds those proxies and asks them to load their fonts..... even those that correspond to @font-face rules that didn't change. :(

While this doesn't particularly affect the typical use of @font-face on most pages - we build the font set one, and it sticks around - it does suck pretty badly for people doing dynamic stuff as described in the comments above.

Comment 5

[Sean McBride]

This progressive loading of fonts is likely to be common for any page that allows selecting between a set of fonts, since it's wasteful to download all the fonts when the page first loads since many of them may not be selected.

In the future, I can also see normal webpages that are designed to load content on the page progressively choosing to load the fonts associated with those pieces of content progressively as well. (If a piece of content is below the fold, maybe I load that content and the fonts used there after the initial page has loaded.)

So, it's not the most common use case, but going forward I think it won't be completely obscure either.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sean, we don't load the fonts eagerly.  So even if you stylesheet _specifies_ all the fonts at load they won't actually be loaded until used.  Hence there is no "download all the fonts when the page first loads" issue.  As I recall Webkit does something similar.

> If a piece of content is below the fold

Then it can still affect positioning of content above the fold, so if you lay it out with the wrong font and then switch, things will suddenly move around.

I agree this should be fixed because people will do this even if it's not needed or counterproductive, obviously.  But your two use cases are in fact making assumptions that are just wrong.

Comment 7

[Sean McBride]

> we don't load the fonts eagerly.  So even if you stylesheet _specifies_
all the fonts at load they won't actually be loaded until used.

This is normally true, but the way that Typekit / Google Web Font Loader load fonts involves injecting spans with the fonts offscreen so that we can check the size and fire font events when they've finished loading (or fail to load). So, when using these loading methods, all of the fonts *are* loaded right away.

> Then it can still affect positioning of content above the fold, so if you lay
it out with the wrong font and then switch, things will suddenly move around.

I was thinking more along the lines of not rendering the content below the fold until you had scrolled down. Think "infinite scrolling" style websites like the Skittles website, for example: http://skittles.com/

Usually, content farther down the page doesn't affect the positioning of content further up the page, right?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> So, when using these loading methods, all of the fonts *are* loaded right away.

I see.  That's rather unfortunate.... :(

> Usually, content farther down the page doesn't affect the positioning of
> content further up the page, right?

It depends.  Any time a table is involved it does, for example.   Similar for flexboxes, auto-width floats or abs pos elements, and so forth.

Comment 9

[Sean McBride]

> I see.  That's rather unfortunate.... :(

I dunno if unfortunate is the right word. It's just another way to use @font-face: "I want to use the font now and I want to know when it's rendering and ready for use." (For example, you might be doing a mathematical layout that requires you to be able to calculate sizes based on the font's metrics.) It just means that if you do want to use other fonts later, you'll be dynamically adding @font-face rules.

Another use case for dynamically adding @font-face rules is when the set of potential fonts to use is very large. For example: if we built a page that could preview any font in the Typekit library. Firefox lazy-loads @font-face rules for me, but we still wouldn't add a @font-face rule for every font in the library to the default CSS because (1) that's a HUGE number of @font-face rules and (2) other browsers might not be smart enough to lazy-load those resources like Firefox does.

It sounds like we both agree that the flash when dynamically adding @font-face rules should be fixed, so that's all that really matters. I just wanted to point out some use cases when dynamically adding @font-face rules makes sense.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> "I want to use the font now and I want to know when it's rendering and ready
> for use."

That's fine; it just sounded like this was being done for _all_ fonts in the stylesheet, not just the ones the page wants to use.

It also sounded like we should have some way to tell when the fonts load without having to resort "measure the metrics" stuff.... ;)

Comment 11

[Sean McBride]

> It also sounded like we should have some way to tell when the fonts load
> without having to resort "measure the metrics" stuff.... ;)

Yes definitely! There was a thread on the www-style list about that which I contributed to a while ago. Having native font events (both CSS pseudo-classes and JavaScript events to attach callbacks to) for when individual fonts and all the fonts on a page are loaded and rendering would be really useful.

But that seems to be outside the scope of this bug. :)

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to comment #10)
> It also sounded like we should have some way to tell when the fonts load
> without having to resort "measure the metrics" stuff.... ;)

That sounds like bug 471915, which is definitely something we want (but it's not actually spec'd or anything yet....)

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: patch, don't discard font entries for @font-face rules that haven't changed

The basic idea here is to avoid throwing away and re-creating font entries for existing rules, just because a rule (or stylesheet) has been added or deleted. To do this, we have to track the association between rules and font entries; this is done by nsUserFontSet, which provides the link between CSS rules and GFX fonts.

With this patch, the testcase here no longer flickers when the styles are updated.

Comment 14

[Jonathan Kew [:jfkthame]]

BTW, this depends on fixing bug 623711, so that the font entries associated with @font-face rules don't get discarded in case of failure to actually load the resource.

Comment 15

[Jonathan Kew [:jfkthame]]

<ping> review?

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 514915 [details] [diff] [review]
patch, don't discard font entries for @font-face rules that haven't changed

Switching r? to dbaron, as this relates to how we handle the CSS rules.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: patch, don't discard font entries for @font-face rules that haven't changed - refreshed

Merged to current m-c; minor fixup as the original patch no longer compiled.

Comment 18

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

Comments so far, though I haven't really gotten into the details yet:

You should remove nsPresContext::mFontFaceRules, which this patch makes
unused.

Both the old and the new code are broken when @font-face rules are 
mutated internally (i.e., their descriptors are changed).  We should
probably fix that at some point.

Instead of having a BeginMutation/EndMutation/InsertRule API, it seems 
like it would be better to have a single API that did this in a single
method call:
>+        mUserFontSet->BeginMutation();
>+        for (PRUint32 i = 0, i_end = rules.Length(); i < i_end; ++i) {
>+          mUserFontSet->InsertRule(rules[i].mRule, rules[i].mSheetType);
>+        }
>+        changed = mUserFontSet->EndMutation();
... which would then mean that mOldRules would no longer need to be a
member variable -- it could just be on the stack in the implementation
of that method, which seems to make more sense to me.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: updated per comment #18

Thanks for the initial comments. Yes, it's much tidier like this.

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to comment #18)
> Both the old and the new code are broken when @font-face rules are 
> mutated internally (i.e., their descriptors are changed).  We should
> probably fix that at some point.

We should create some testcases for this and file a separate bug, I think. Yes, it'd be good to fix it, though nobody has complained yet!

Comment 21

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

Comment on attachment 525047 [details] [diff] [review]
updated per comment #18

In your new AddFontFace (and also the existing AddFontFace), you can
remove the null-check "if (family)", since operator new is now
infallible.

In nsUserFontSet::InsertRule:
>+    FontFaceRuleRecord& ruleRec = oldRules[i];
I think this can and probably should be |const|.

This patch seems to introduce the possibility of an empty user font set
-- although I'm not sure you intended that.
nsStyleSet::AppendFontFaceRules returns false on allocation failure.
So you've changed the behavior for allocation failure in
AppendFontFaceRules from no-change to delete-user-font-set -- I'm not
sure that I care about the difference.  But, perhaps more importantly,
you've changed the behavior when going from having @font-face rules to
not having them to introduce a case where we now have an empty user font
set.  I tend to think it's probably better to add a check that
rules.Length() == 0, and if so, release the existing user font set.
It's more consistent with the old code, and it seems better to avoid
having a very-rarely-tested case where we can have a user font set
representing no @font-face rules.  So I'd suggest fixing
FlushUserFontSet to also, right around this code:
>+      if (!mUserFontSet && rules.Length() > 0) {
>+        mUserFontSet = new nsUserFontSet(this);
>+        if (!mUserFontSet) {
>+          return;
>+        }
>+        NS_ADDREF(mUserFontSet);
>+      }
check for the inverse case and release any existing font set if there
are no rules, e.g., by doing:
  if (rules.Length() == 0) {
    NS_IF_RELEASE(mUserFontSet);
    changed = PR_TRUE;
  }

Given the new AddFontFace call early in nsUserFontSet::InsertRule
and the possibility for dynamic changes of descriptors that we're
not currently handling, I tend to think you should also move the
check for fontfamily.IsEmpty() much earlier in nsUserFontSet::InsertRule
-- i.e., take this:
>+    // If there is no family name, this rule cannot contribute a
>+    // usable font, so there is no point in processing it further.
>+    return;
out of the else that it's in, and put it in an if(fontfamily.IsEmpty())
just below -- and then drop the fontfamily.IsEmpty() check near the end
of the function around the "old" AddFontFace call.

>+  struct FontFaceRuleRecord {
>+    nsRefPtr<nsCSSFontFaceRule>  mRule;
>+    nsRefPtr<gfxFontEntry>       mFontEntry;
>+    PRUint8                      mSheetType;
>+  };

Maybe this should inherit from nsFontFaceRuleContainer or have
one as a member, instead of mRule and mSheetType?  I could go either
way, though.

r=dbaron with that

Comment 22

[Jonathan Kew [:jfkthame]]

Pushed, with updates as per comment 21.
http://hg.mozilla.org/mozilla-central/rev/3f82f25d7334

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Backed out

http://hg.mozilla.org/mozilla-central/rev/ccc7f1ec8076

(Please fix trailing whitespace before relanding.)

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to comment #21)
>   if (rules.Length() == 0) {
>     NS_IF_RELEASE(mUserFontSet);
>     changed = PR_TRUE;
>   }

This was a good idea but not quite right - it caused  'changed'  to be true in the (common) case when there simply aren't any rules, which triggers lots of unnecessary reflow, and also tickles a bunch of extra assertions, leading to crashtest orangeness.

I'll be relanding with something along the lines of
  if (rules.Length() == 0 && mUserFontSet) {
    mUserFontSet->Destroy();
    NS_RELEASE(mUserFontSet);
    changed = PR_TRUE;
  }
instead, after re-testing on try.

Comment 25

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

As long as you don't make a new user font set when rules.Length() is 0, sounds good.

Comment 26

[Jonathan Kew [:jfkthame]]

Landed on m-c (on 4/12, but failed to annotate the bug then):
http://hg.mozilla.org/mozilla-central/rev/0138798a072a