New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-fonts] Avoid race condition #11082

Merged
merged 2 commits into from Aug 21, 2018

Conversation

@jugglinmike
Contributor

jugglinmike commented May 19, 2018

@gsnedders @Ms2ger I'm not certain that my rationale is sound, so I've over-explained a bit to help you show me if/where I've gone wrong.


The modified test primes the document's font source by declaring a
font-face rule and deferring tests until the font has been loaded. The
sub-tests are expressed in terms of a FontFace which is dynamically
generated via a <style> tag but which shares the same URL.

Prior to this patch, the test assumed that the declaration of the second
FontFace would be reflected in a synchronously-triggered reflow (via
access of the offsetWidth property). This is not guaranteed because
each parse of a <style> element creates a new FontFace object [1] which
must be loaded [2] with the potentially CORS-enabled fetch method of the
HTML specification [3].

Practically speaking, this caused the Firefox and Chrome browsers to
consistently fail the sub-tests.

Update the test to define a unique FontFace for every subtest and to
explicitly wait for it to be available for rendering. Annotate the test
as "long" to accomodate the additional time spent waiting.

[1] > A CSS @font-face rule automatically defines a corresponding
> FontFace object, which is automatically placed in the document’s
> font source when the rule is parsed.

https://www.w3.org/TR/css-font-loading-3/#font-face-css-connection

[2] > When a user-agent needs to load a font face, it must do so by
> calling the load() method of the corresponding FontFace object.

https://www.w3.org/TR/css-font-loading-3/#font-face-set-css

[3] > For font loads, user agents must use the potentially CORS-enabled
> fetch method defined by the [HTML5] specification for URL's
> defined within @font-face rules.

https://www.w3.org/TR/css-fonts-3/#font-fetching-requirements
@svgeesus

This comment has been minimized.

Contributor

svgeesus commented May 19, 2018

This test is really about CSS font loading so suggesting review by the editor, @tabatkins

@emilio

This comment has been minimized.

Contributor

emilio commented Jun 1, 2018

cc @jwatt, which is looking at similar tests right now IIRC :)

@jwatt

This comment has been minimized.

Contributor

jwatt commented Jun 1, 2018

I'm working on other tests in this directory, but it's @jfkthame who's been looking at this one. If Tab is too busy maybe he or I can review this pull request in the next couple of days.

@jfkthame

This comment has been minimized.

Contributor

jfkthame commented Jun 1, 2018

AFAICS, this makes things work somewhat better in Firefox, but there are still issues that I'm not sure how best to resolve.

(1) The descriptorPriorityCases still fail on first run, but pass if the test file is reloaded. I think this is because once_fonts_are_ready is only checking the readiness of the reference fonts "W100", "W200", "W300". At the time it is checked, nothing has triggered loading of any faces of the descriptorPriorityTest family; the @font-face rules are present but no content is using that family, so no loads have been initiated, and document.fonts.ready resolves. Only then does the test apply the descriptorPriorityTest family to the test element; but that will trigger a new font load, and nothing guarantees that will complete before the element's width is inspected.

(2) The function createFontFaceRules creates two @font-face rules, but the code here only triggers loading of one face, and waits for it to be ready. That's not enough to make verifyFont work reliably, as it may need to use either of the two faces.

"Descriptor mathcing priority: " + testCase.description
);
});
function createFontFaceRules(fontFaceFamily, descriptorName, expectedMatch, unexpectedMatch) {
const el = document.createElement("span");
el.innerText = "A";

This comment has been minimized.

@jfkthame

jfkthame Jun 1, 2018

Contributor

Instead of just setting el.innerText to a single "A", doing something like this:

        const expect = expectedMatch.split(" ")[0];
        const unexpect = unexpectedMatch.split(" ")[0];
        el.innerHTML = `<span style="${descriptorName}:${expect}">A</span><span style="${descriptorName}:${unexpect}">A</span>`;

seems to help quite a bit, by trying to ensure that loading will be triggered for both faces. (In principle, it doesn't guarantee this, as both the expect and unexpect property values could still map to the same face.)

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Jun 12, 2018

Thank you for the review, @jfkthame!

AFAICS, this makes things work somewhat better in Firefox, but there are still issues that I'm not sure how best to resolve.

I'd been testing using the stable release of Firefox, but I really ought to be using Nightly. In this case, the flakiness is only observable in that release.

I think this is because once_fonts_are_ready is only checking the readiness of the reference fonts "W100", "W200", "W300". At the time it is checked, nothing has triggered loading of any faces of the descriptorPriorityTest family; the @font-face rules are present but no content is using that family, so no loads have been initiated, and document.fonts.ready resolves.

I've added <span> elements for each of the faces of the descriptorPriorityTest family. I was reluctant to do this because test concerns how font faces are applied, and I didn't want to rely on the behavior under test. After giving it some more thought, this seems acceptable because the test is specifically about the selection between multiple alternatives.

seems to help quite a bit, by trying to ensure that loading will be triggered for both faces

Oh, good point! And thanks for writing up the demonstration. I've extended it just a bit to create two separate elements (the detection mechanism reacts to any change in offsetWidth, and we don't want to proceed if only one of the two families has been applied).

This seems to resolve the flakiness in Firefox Nightly. Would you mind taking another look?

@jfkthame

This comment has been minimized.

Contributor

jfkthame commented Jun 12, 2018

Would you mind taking another look?

I don't seem to see any such updates here; the PR just seems to have your original commit from 3 weeks ago. Am I looking in the wrong place?

@wpt-pr-bot

This comment has been minimized.

Collaborator

wpt-pr-bot commented Jun 12, 2018

There are no owners for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Jun 12, 2018

Nope, I just forgot to push up my commit. Sorry about that!

"@font-face { font-family: " + fontFaceFamily + "; src: url('./resources/csstest-weights-200-kerned.ttf'); " + descriptorName + ": " + unexpectedMatch + "; }";
return Promise.all([
load(fontFaceFamily, descriptorName, expectedMatch.split(" ")[0]),

This comment has been minimized.

@jfkthame

jfkthame Jun 13, 2018

Contributor

Actually, the use of .split(" ")[0] here is too crude for the case of font-style, as it will result in stripping oblique <angle1> <angle2> down to a simple oblique, completely losing the custom angle.

Possible fix: pass the expectedMatch or unexpectedMatch unmodified into load(), and within there, process it with something like this:

        if (value.indexOf("deg") > 0) {
          value = "oblique " + value.split(" ")[1];
        } else {
          value = value.split(" ")[0];
        }

That causes a bunch more of the tests to pass reliably in Firefox. There are still a few residual failures, which result from losing the distinction between the expected and unexpected resources because of only considering the range-start here. So for a more complete solution, I think you need to create a pair of spans for each of the rules, and apply the range-start property to one of them and the range-end property to the other.

Taking all that into account, I came up with this for the load() function:

    function load(family, name, value) {
        const el1 = document.createElement("span");
        const el2 = document.createElement("span");
        el1.innerText = "A";
        el2.innerText = "A";
        let value1, value2;
        if (value.indexOf("deg") > 0) {
          value1 = "oblique " + value.split(" ")[1];
          value2 = "oblique " + (value.split(" ")[2] || value.split(" ")[1]);
        } else {
          value1 = value.split(" ")[0];
          value2 = value.split(" ")[1] || value1;
        }
        el1.style[name] = value1;
        el2.style[name] = value2;
        document.body.appendChild(el1);
        document.body.appendChild(el2);
        const initialWidth1 = el1.offsetWidth;
        const initialWidth2 = el2.offsetWidth;
        return new Promise((resolve) => {
            el1.style.fontFamily = family;
            el2.style.fontFamily = family;
            (function check() {
                if (el1.offsetWidth !== initialWidth1 && el2.offsetWidth !== initialWidth2) {
                    el1.remove();
                    el2.remove();
                    resolve();
                } else {
                    requestAnimationFrame(check);
                }
            }());
        });
    }

That finally makes the matching tests run reliably for me.

promise_test(() => {
return once_fonts_are_ready
.then(() => verifyFont("descriptorPriorityTest", testCase.weight, testCase.style, testCase.stretch, testCase.expectedFamily));
},
"Descriptor mathcing priority: " + testCase.description

This comment has been minimized.

@jfkthame

jfkthame Jun 13, 2018

Contributor

BTW, while you're here, maybe you could also fix the typo? s/mathcing/matching/

This comment has been minimized.

@jugglinmike

jugglinmike Jun 14, 2018

Contributor

Sure

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Jun 14, 2018

The setup criteria for these tests is deceptively complex. Thanks for helping me through it, @jfkthame.

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Jun 27, 2018

@jfkthame would you mind taking another look at this?

@svgeesus

This comment has been minimized.

Contributor

svgeesus commented Aug 16, 2018

@jfkthame do the latest changes satisfy all your review comments?

@jfkthame

This comment has been minimized.

Contributor

jfkthame commented Aug 20, 2018

Yes, looks much better - thanks!

@Ms2ger

Ms2ger approved these changes Aug 20, 2018

r=jfkthame

@Ms2ger

This comment has been minimized.

Contributor

Ms2ger commented Aug 20, 2018

Can you squash the fixup commits, please? I think that'll be all that's still needed, then.

jugglinmike added some commits May 17, 2018

[css-fonts] Avoid race condition
The modified test primes the document's font source by declaring a
`font-face` rule and deferring tests until the font has been loaded. The
sub-tests are expressed in terms of a FontFace which is dynamically
generated via a `<style>` tag but which shares the same URL.

Prior to this patch, the test assumed that the declaration of the second
FontFace would be reflected in a synchronously-triggered reflow (via
access of the `offsetWidth` property). This is not guaranteed because
each parse of a `<style>` element creates a new FontFace object [1] which
must be loaded [2] with the potentially CORS-enabled fetch method of the
HTML specification [3].

Practically speaking, this caused the Firefox and Chrome browsers to
consistently fail the sub-tests.

Update the test to define a unique FontFace for every subtest and to
explicitly wait for it to be available for rendering. Annotate the test
as "long" to accomodate the additional time spent waiting.

[1] > A CSS @font-face rule automatically defines a corresponding
    > FontFace object, which is automatically placed in the document’s
    > font source when the rule is parsed.

    https://www.w3.org/TR/css-font-loading-3/#font-face-css-connection

[2] > When a user-agent needs to load a font face, it must do so by
    > calling the load() method of the corresponding FontFace object.

    https://www.w3.org/TR/css-font-loading-3/#font-face-set-css

[3] > For font loads, user agents must use the potentially CORS-enabled
    > fetch method defined by the [HTML5] specification for URL's
    > defined within @font-face rules.

    https://www.w3.org/TR/css-fonts-3/#font-fetching-requirements
@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Aug 20, 2018

Certainly. The original version of this branch is available here:

master...bocoup:improve-css-test-prebase

@jugglinmike jugglinmike force-pushed the bocoup:improve-css-test branch from 0ed75a4 to 5aa1fc8 Aug 20, 2018

@Ms2ger Ms2ger merged commit d829f20 into web-platform-tests:master Aug 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Aug 21, 2018

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment