1422530 - Low priority webfonts in font-family declaration are downloaded before they are needed

Status: RESOLVED FIXED

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

Description

[munter]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

I have two webfonts: subset.woff and full.woff. The first contains a subset of the second. Both of these fonts are declared as differently named font-faces: 'font__subset' and 'font'. In order to take advantage of the missing glyphs fallthrough and on-demand loading of priority ordered font-family, I declare font-family: font__subset, font, serif;

A minimum example can be seen on https://ff-subfont.surge.sh/


Actual results:

Firefox loads subset.woff and full.woff seemingly in parallel


Expected results:

subset.woff should have loaded, and only if the page contained glyphs that were not in this font file, the next font in priority order should be loaded.

When I inline the subset I do not see Firefox downloading the second webfont. I assume the current implementation loads lower priority fonts when it sees a higher priority font is not loaded yet, regardless of its current loading state. So on initial page load the first order font will start loading, then the second order one will immediately start loading before the first one finishes

Comment 1

[munter]

The system seems to automatically add the UA of my current browser, which was Chrome, not Firefox. The Firefox UA that showed the error is "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:57.0) Gecko/20100101 Firefox/57.0"

Comment 2

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

Jonathan might be able to provide a little more information here.

However, a few questions might help first:

(In reply to munter from comment #0)
> I have two webfonts: subset.woff and full.woff. The first contains a subset
> of the second. Both of these fonts are declared as differently named
> font-faces: 'font__subset' and 'font'. In order to take advantage of the
> missing glyphs fallthrough and on-demand loading of priority ordered
> font-family, I declare font-family: font__subset, font, serif;
> 
> A minimum example can be seen on https://ff-subfont.surge.sh/

Is "Cookie" here the subset of Sacramento?  That seems pretty confusing, given that you've prioritized a local font named Cookie-Regular over that?

Or is this URL no longer a testcase for this bug?

> When I inline the subset I do not see Firefox downloading the second
> webfont.

By "inline the subset", do you mean make the woff file a data URL and put the data URL in the style sheet?

Comment 3

[munter]

Here's a more clear example. I also added unicode-ranges to hopefully make the behavior more obvious.

New test case: http://unicode-range-fallbacks.surge.sh/

The page has an 'A'-character, which should get the Roboto font, since that is the first font in the `font-family` value.
The page also contains a cow face emoji which doesn't exist in Roboto, nor in Open Sans, which is the second font to attempt loading if the first is missing relevant glyphs.

Both fonts have unicode-ranges applied which make it clear that the cow face glyph doesn't exist in either web font.

Firefox loads both Roboto and Open Sans, even though the unicode range tells it the glyph will not be found here.

Expected behavior: Only the Roboto font should be loaded.

Firefox test run: https://www.webpagetest.org/result/180524_YW_1d97b9ca45c56dbedc62dcabaeb72ffa/1/details/#waterfall_view_step1

Edge behaves the same as Firefox: https://www.webpagetest.org/result/180524_E3_bf2af29ff6669d6998d33fd30a378838/1/details/#waterfall_view_step1

Chrome exibits the expected behavior: https://www.webpagetest.org/result/180524_VE_b4524b8559ab0151019fd9cd1e278aa3/1/details/#waterfall_view_step1

Comment 4

[Jonathan Kew [:jfkthame]]

I can confirm the example from comment 3 appears to request both Roboto and Open Sans font resources. It doesn't even require the cow-emoji character to trigger this; just the "A" is sufficient.

This behavior seems incorrect: the second font in the font-family list shouldn't load unless the first one doesn't support the required characters.

If the resource for the first font-family loads synchronously (which is the case if the source is local() or url(data:...) in the @font-face rule), then we don't request the resource for the second family. But in the common case of a remote url() source, it seems that we are initiating a load for the second font-family while the (async) load for the first is still pending. That's not the intended behavior, AFAIK.

I suppose in some cases this would actually perform better -- if it turns out that the first family can't satisfy the required character repertoire, we'll have already started loading the next -- but OTOH it means we may be fetching a resource that will not be needed/used at all, wasting the user's bandwidth.

The load is not necessarily even triggered by the specific text content (while we're trying to match available fonts to the characters present in the text), but may come from layout asking for font metrics to compute line-height, baseline position, etc. This eventually leads us to gfxFontGroup::GetFirstValidFont, which walks through the font-family list trying each family in turn until it finds something usable. I think this is too eager to initiate loading of @font-face resources, ignoring the possibility that an earlier font in the family list is already being loaded.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to munter from comment #3)
> Here's a more clear example. I also added unicode-ranges to hopefully make
> the behavior more obvious.
> 
> New test case: http://unicode-range-fallbacks.surge.sh/
> 
> The page has an 'A'-character, which should get the Roboto font, since that
> is the first font in the `font-family` value.
> The page also contains a cow face emoji which doesn't exist in Roboto, nor
> in Open Sans, which is the second font to attempt loading if the first is
> missing relevant glyphs.
> 
> Both fonts have unicode-ranges applied which make it clear that the cow face
> glyph doesn't exist in either web font.
> 
> Firefox loads both Roboto and Open Sans, even though the unicode range tells
> it the glyph will not be found here.
> 
> Expected behavior: Only the Roboto font should be loaded.

As noted above, it's not the cow-face emoji that is triggering the font loading here, it's the initial requirement to have a font for line metrics, etc. First, we look for Roboto, but it requires a network load; so we fire off that load (asynchronously), but meanwhile move on to the next font-family name. That one also requires a network load, so we trigger that as well, and then (temporarily) use metrics from the system Arial font until the resources are available.

For the scenario where one font resource is a subset, while the other is the full font, I think it would actually be better to write this somewhat differently. Don't declare two separate font families; declare a single family with multiple resources:

    @font-face {
        font-family: foo;
        src: url(foo-complete.woff);
        unicode-range: U+0000-FFFF; /* or whatever */
    }
    @font-face {
        font-family: foo;
        src: url(foo-ascii.woff);
        unicode-range: U+0000-007F;
    }

Then you only need to specify the one family in the font-family property (plus a suitable generic fallback, for the case where downloadable fonts are unavailable etc).

As it happens, in this case Firefox does do the expected thing: the second @font-face rule will take priority over the first (according to the usual CSS convention where later declarations supersede earlier ones), and only that resource will be loaded, unless a character is encountered that is outside its repertoire (but may be supported by the first resource).

The issue here only arises, AFAICS, when multiple distinct @font-face families are used in the font-family property, at which point a pending resource load for an earlier family in the list doesn't prevent us also starting to load a resource for a later family. But within a single family (as suggested here), only the required resource(s) will be loaded.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Don't initiate new font downloads for a fontGroup where we've already set mSkipDrawing because we're waiting on a resource to be available

Perhaps we should try doing something like this. The idea here is that if the fontGroup is currently suppressing painting, because it's waiting for a resource download to complete, it shouldn't fire off any new downloads for other families in the list. This will prevent the redundant download in the example here, and similar cases, avoiding the waste of bandwidth (and possible perf hit due to multiple loads competing for it). Note however that this may _hurt_ performance in the case where it turns out that we actually do need to use a downloadable font resource for a family that is _not_ first in the font-family list, because we won't initiate that fetch until the earlier family has completed downloading (and been found to lack the character we needed). But for a well-written site, a scenario like that should be pretty rare; it doesn't make much sense to have multiple @font-face families in the same font-family list, unless they're covering (for example) different scripts, in which case unicode-range should help us to download the right one(s).

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8987483 [details] [diff] [review]
Don't initiate new font downloads for a fontGroup where we've already set mSkipDrawing because we're waiting on a resource to be available

Tryserver says this doesn't immediately break anything, at least:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2e974b9245d7a5f7799dc30fcab1cab9cd49ef5

The performance impact could be either positive or negative, depending on how sites structure their webfont rules and usage, as noted above; but on balance I think it should make things better for well-written sites.

Comment 8

[Cameron McCormack (:heycam)]

I agree with your assessment that it's rarer that a font list has multiple downloadable fonts listed in it, unless it's for different scripts / unicode-ranges.

What's the behaviour here when the suppression timeout has elapsed and we fall back to a system font?  Will we kick off all the other loads at that point, or will we wait until the first font has finished loading?

Comment 9

[Jonathan Kew [:jfkthame]]

As I understand it, we should kick off the next font load at that point, because mSkipDrawing will no longer be true.

I was in two minds as to whether to let it work that way, or decouple the suppression of later-in-the-list font loads until the first load has fully completed (or failed). On the one hand, if the problem is a very narrow/congested pipe causing slow downloads, kicking off another load may just add to the congestion (though it's still better than the situation today, where we kick off the extra loads immediately!); but on the other hand, if there's a specific problem with the first resource (e.g. it's coming from a temporarily-crippled server), and maybe it's never going to complete, then the sooner we fetch an alternative, the better.

So in the end, I went for what seemed the simplest option. I figure we could still refine it further once we get experience with how it behaves in practice.

Comment 10

[Cameron McCormack (:heycam)]

Thanks, I think that makes sense.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8987483 [details] [diff] [review]
Don't initiate new font downloads for a fontGroup where we've already set mSkipDrawing because we're waiting on a resource to be available

Review of attachment 8987483 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but consider whether we should add a test for this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b885ee1e1b2
Don't initiate new font downloads for a fontGroup where we've already set mSkipDrawing because we're waiting on a resource to be available. r=heycam

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/0b885ee1e1b2