1094571 - test font loading when unicode-range is included

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Daggett (:jtd)]

For the unicode-range implementation we need a set of tests to assure confirm which fonts are or are not loaded, when unicode-range is used. I tried doing this with the Font Loading API, which is the natural thing to use here, but there are still outstanding bugs with our implementation that make this difficult (see bug 1086207).

I filed this under CSS since the test should live in layout/style/test.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, add woff versions of markA .. markD fonts

For use with tests to be submitted, add woff versions of markA .. markD

Comment 2

[John Daggett (:jtd)]

Attachment: patch, add unicode-range loadtests

These tests use the ready promise of the FontFaceSet to check whether the correct fonts were loaded or not loaded for a given font family with faces that use unicode-range.

We have a number of failures related to multiple font loads (e.g. src: url(A),url(B) case where A does not exist).

Note: Chrome fails in some of the same places.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, add unicode-range loadtests

Argh, original patch omitted needed mochitest.ini changes.

Comment 4

[John Daggett (:jtd)]

Attachment: patch, add unicode-range loadtests

I reworked the test structure a bit to add (1) a small timeout after the first ready checks and (2) a second ready check to assure that problems associated with one test don't leak into the results of the next test. This seems to make the tests a bit more stable but I'm still seeing what appear to be load timing dependent variations across test runs (i.e. number of fails varies).

Comment 5

[Cameron McCormack (:heycam)]

A few notes from my progress today.

1. I found that the "failsafe" Promise usage in checkFontsBeforeLoad and checkFontsAfterLoad never get a chance to run, since the assert_equals just before it will throw an exception if the assertion  fails (unlike Mochitest's assertions).

2. I have some patches for bug 1145506 (which I'm yet to clean up and upload) that gets the test to ~320 passes.

3. The remaining failures are all in the multiple @font-face rule subtests (including the ones with all default unicode-ranges), and the problem is that between each entry in the src list that we try to load, we set the FontFaceSet back to "loaded".  Is that expected, that we will have a brief period where the gfxUserFontEntry is STATUS_LOADED, before we find that the first src doesn't have the character we're after and call LoadNextSrc() again at some point?  I don't really know the process where character matching influences font loading.

Comment 6

[John Daggett (:jtd)]

(In reply to Cameron McCormack (:heycam) from comment #5)

> 1. I found that the "failsafe" Promise usage in checkFontsBeforeLoad and
> checkFontsAfterLoad never get a chance to run, since the assert_equals just
> before it will throw an exception if the assertion  fails (unlike
> Mochitest's assertions).

That's removed in the latest attached version in which I instead added an extra use of the ready promise (really ready?) to try and catch weird load-state issues before the next test begins.

> 3. The remaining failures are all in the multiple @font-face rule subtests
> (including the ones with all default unicode-ranges), and the problem is
> that between each entry in the src list that we try to load, we set the
> FontFaceSet back to "loaded".  Is that expected, that we will have a brief
> period where the gfxUserFontEntry is STATUS_LOADED, before we find that the
> first src doesn't have the character we're after and call LoadNextSrc()
> again at some point?  I don't really know the process where character
> matching influences font loading.

That seems wrong. When loading a single face with multiple sources, FontFace.status and FontFaceSet.status should never be "loaded" until after the final source succeeds or fails. There should never be a loading-loaded-loading transition in between sources.

Comment 7

[John Daggett (:jtd)]

er, by final source I really mean "a given source succeeds or the final one fails".

Comment 8

[Cameron McCormack (:heycam)]

(In reply to John Daggett (:jtd) from comment #6)
> That seems wrong. When loading a single face with multiple sources,
> FontFace.status and FontFaceSet.status should never be "loaded" until after
> the final source succeeds or fails. There should never be a
> loading-loaded-loading transition in between sources.

Indeed.  So the reason why this is happening is that the FontFace::SetState(Loaded) call, which calls CheckForLoadingFinished(), is done before the nsPresContext::UserFontSetChanged() call (which queues a reflow and is what would cause MightHavePendingFontLoads() to return true).  I think we can solve this just by calling CheckForLoadingFinished() slightly later.

Comment 9

[Cameron McCormack (:heycam)]

Handling that issue in bug 1145937.

Comment 10

[Cameron McCormack (:heycam)]

Tests are passing for me locally on Mac:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d850e5ac6e31

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8579876 [details] [diff] [review]
patch, add unicode-range loadtests

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

r=me with comments addressed, and if you can explain how the ordering of the testdata fonts arrays works.

::: layout/style/test/test_unicode_range_loading.html
@@ +71,5 @@
> +    content: "CCC" },
> +  { test: "multiple fonts with overlapping ranges, all with default ranges, first one supports character used",
> +    fonts: [{ family: "a", src: "markB", descriptors: { }, loaded: false},
> +            { family: "a", src: "markA", descriptors: { }, loaded: false},
> +            { family: "a", src: "markC", descriptors: { }, loaded: true}],

I'm a bit confused.  The order of the objects in these arrays appears to be the reverse of their required appearance in the style sheet.  Is that right?  But it looks like in addFontFaceRules that you are inserting them in order (using |n| as the second argument to insertRule)?

@@ +118,5 @@
> +  featureSettings: "font-feature-settings"
> +};
> +
> +var kBaseFontURL;
> +if ("SpecialPowers" in window) {

Does this let you run the test outside of mochitest?

@@ +156,5 @@
> +  for (var d in fontdata.descriptors) {
> +    desc.push(mapDescriptorNames[d] + ": " + fontdata.descriptors[d]);
> +  }
> +  var ff = "@font-face { " + desc.join(";") + " }";
> +  //console.log(ff);

I think this and the uncommented console.log lines should be removed.

@@ +282,5 @@
> +      var i = 0;
> +      fd.forEach(function(f) {
> +        assert_true(f.font instanceof FontFace, "font needs to be an instance of FontFace object");
> +        if (f.data.loaded) {
> +          assert_equals(f.font.status, "loaded", "font not loaded - font " + i + " " + f.data.src + " " 

Trailing space.

@@ +285,5 @@
> +        if (f.data.loaded) {
> +          assert_equals(f.font.status, "loaded", "font not loaded - font " + i + " " + f.data.src + " " 
> +                        + JSON.stringify(f.data.descriptors) + " for content " + testdata.content);
> +        } else {
> +          assert_equals(f.font.status, "unloaded", "font loaded - font " + i + " " + f.data.src + " " 

And here.

@@ +301,5 @@
> +    var ready = getReady();
> +    return ready.then(function() {
> +      checkFontsAfterLoad(name, testdata, fd, false);
> +    }).then(function() {
> +      return setTimeoutPromise(10).then(function() {

I think probably setTimeout(0) was fine, for the purpose of just going around the event loop.  Using a value like 10 feels like it could introduce flakiness (if the test were failing).

@@ +344,5 @@
> +
> +  var fontTypes = ["", "twourl", "localfirst"];
> +
> +  unicodeRangeTests.forEach(function(testdata, i) {
> +    fontTypes.forEach(function(ft) {  

Trailing space.

Comment 12

[Cameron McCormack (:heycam)]

Don't forget to fails-if() the test for Linux pending bug 1056479.

Comment 13

[John Daggett (:jtd)]

(In reply to Cameron McCormack (:heycam) from comment #11)
> ::: layout/style/test/test_unicode_range_loading.html
> @@ +71,5 @@
> > +    content: "CCC" },
> > +  { test: "multiple fonts with overlapping ranges, all with default ranges, first one supports character used",
> > +    fonts: [{ family: "a", src: "markB", descriptors: { }, loaded: false},
> > +            { family: "a", src: "markA", descriptors: { }, loaded: false},
> > +            { family: "a", src: "markC", descriptors: { }, loaded: true}],
> 
> I'm a bit confused.  The order of the objects in these arrays appears to be
> the reverse of their required appearance in the style sheet.  Is that right?
> But it looks like in addFontFaceRules that you are inserting them in order
> (using |n| as the second argument to insertRule)?

The rules are in the same order they would be in as @font-face rules in a stylesheet. So the "first" one loaded is the last one defined, in this case "markC". Was your confusion about the comment or do you think the test is incorrect in some way?

> > +var kBaseFontURL;
> > +if ("SpecialPowers" in window) {
> 
> Does this let you run the test outside of mochitest?

Yes, this is so that when serving the test normally I can put the fonts in a separate directory. The support-files construct in mochitests seem to require everything in the same folder unfortunately.

Comment 14

[Cameron McCormack (:heycam)]

(In reply to John Daggett (:jtd) from comment #13)
> (In reply to Cameron McCormack (:heycam) from comment #11)
> > ::: layout/style/test/test_unicode_range_loading.html
> > @@ +71,5 @@
> > > +    content: "CCC" },
> > > +  { test: "multiple fonts with overlapping ranges, all with default ranges, first one supports character used",
> > > +    fonts: [{ family: "a", src: "markB", descriptors: { }, loaded: false},
> > > +            { family: "a", src: "markA", descriptors: { }, loaded: false},
> > > +            { family: "a", src: "markC", descriptors: { }, loaded: true}],
> > 
> > I'm a bit confused.  The order of the objects in these arrays appears to be
> > the reverse of their required appearance in the style sheet.  Is that right?
> > But it looks like in addFontFaceRules that you are inserting them in order
> > (using |n| as the second argument to insertRule)?
> 
> The rules are in the same order they would be in as @font-face rules in a
> stylesheet. So the "first" one loaded is the last one defined, in this case
> "markC". Was your confusion about the comment or do you think the test is
> incorrect in some way?

I didn't realise we go through the @font-face rules in reverse order when kicking off loads.  Makes sense though, since you want the last one to win.  Can you do s/first one/first one loaded (the last @font-face rule)/?

Comment 15

[John Daggett (:jtd)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ede1b9edd741
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace25a84812f

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ede1b9edd741
https://hg.mozilla.org/mozilla-central/rev/ace25a84812f