fix: head.ready not triggered because of wrong asset.state for css #273

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@xhaggi
xhaggi commented Nov 15, 2013

Some browsers (e.g. Safari) don't fire the onload event for loaded css, so the asset.state will never updated to LOADED. If allLoaded() is called the css assets are in state = LOADING. This PR adds a workaround for this issue without using the img hack (#240)

@xhaggi xhaggi fix: head.ready not triggered because of wrong asset.state for css
Some browsers (e.g. Safari) don't fire the onload event for loaded css,
so the asset.state will never updated to LOADED. If allLoaded() is
called the css assets are in state = LOADING. This PR adds a workaround
for this issue without using the img hack (#240)
c5fba92
@xhaggi
xhaggi commented Nov 15, 2013

@robert-hoffmann the current version 1.0.1 is broken for Safari, because if you load css it never fire the head.ready() event.

@robert-hoffmann
Member

Nice alternative, i'll examine it a bit deeper.

Another one that was proposed created img elements to attach a load handler: #240 but it failed in some tests so i put it aside to concentrate on other things.

Maybe it's possible to only apply the fix if we are sure the browser does not support the load event on css ..need to grab a old browser and see if this is testable ..maybe something like

var supportsLoad = ("onload" in document.createElement("link"));
@robert-hoffmann
Member

Ok i did some better tests on #240, and it fails ..so closed.

This leaves your solution.

I like the idea, but have a few issues with it for now:

  • is attached to all css files, no matter if the browser supports the onload event or not
    • potential multiple triggering of load event ?
  • missing re-try counter
    • if the script fails to attach it could potentially loop indefinitely
@xhaggi
xhaggi commented Nov 15, 2013
  • onload: is there a way to safety check if onload is supported?
  • retry-counter: indeed, this should be done before this goes in
@xhaggi xhaggi fixup: improve stuff (squashed later)
* identify asset loaded by styleSheet.href
* fix potential of trigger process multiple times
* adds retry check
edee0de
@xhaggi
xhaggi commented Nov 15, 2013

@robert-hoffmann sorry too early pushed ;) .. you find improvements in the separate commit

@robert-hoffmann
Member

This still won't be enough. Another problem is object references ..remember loadAsset() can be called multiple times

var retry = 0; //
var cssLoadInterval = setInterval(function() {
        if(asset.state === LOADED || retry++ === 100)
                return clearInterval(cssLoadInterval);

        for(var i=0;i<document.styleSheets.length;i++) {
                var styleSheet = document.styleSheets[i];
                if(styleSheet.href === asset.url) {
                        clearInterval(cssLoadInterval);
                        process({ "type": "load" });
                }
        }
}, 10);

In the above code var retry, var cssLoadInterval, asset.state, and asset.url will all change on subsequent calls (or maybe i'm too paranoid ..didn't test it actually). So i think it's gonna be a bit more tricky than that..

On a good note, i just managed to get the timeout handler working correctly for head.load() ..this means we can fire the callback after a given time, and for css this can mean it can always be called ..a bit late, but nonetheless.

@xhaggi
xhaggi commented Nov 16, 2013

@robert-hoffmann there is no issue with references here.
I put some logging within the interval and modify only one of the assets to be LOADED (state = 4)

Retry: 0
URL: https://abs.twimg.com/a/1384470786/t1/css/t1_more.bundle.css
State: 3
Object { name="t1_more.bundle.css", url="https://abs.twimg.com/a.../css/t1_more.bundle.css", state=3}

Retry: 0
URL: https://github.global.ssl.fastly.net/assets/github-09acd31812af6ce17b5f432a7597c5849393330e.css
State: 3
Object { name="github-09acd31812af6ce1...32a7597c5849393330e.css", url="https://github.global.s...32a7597c5849393330e.css", state=3}

Retry: 0
URL: https://abs.twimg.com/a/1384470786/t1/css/t1_core_logged_out.bundle.css
State: 4
Object { name="t1_core_logged_out.bundle.css", url="https://abs.twimg.com/a...e_logged_out.bundle.css", state=3}

Retry: 1
URL: https://abs.twimg.com/a/1384470786/t1/css/t1_more.bundle.css
State: 3
Object { name="t1_more.bundle.css", url="https://abs.twimg.com/a.../css/t1_more.bundle.css", state=3}

Retry: 1
URL: https://github.global.ssl.fastly.net/assets/github-09acd31812af6ce17b5f432a7597c5849393330e.css
State: 3
Object { name="github-09acd31812af6ce1...32a7597c5849393330e.css", url="https://github.global.s...32a7597c5849393330e.css", state=3}

Retry: 1
URL: https://abs.twimg.com/a/1384470786/t1/css/t1_core_logged_out.bundle.css
State: 4
Object { name="t1_core_logged_out.bundle.css", url="https://abs.twimg.com/a...e_logged_out.bundle.css", state=3}

Retry: 2
URL: https://abs.twimg.com/a/1384470786/t1/css/t1_more.bundle.css
State: 3
Object { name="t1_more.bundle.css", url="https://abs.twimg.com/a.../css/t1_more.bundle.css", state=3}

Retry: 2
URL: https://github.global.ssl.fastly.net/assets/github-09acd31812af6ce17b5f432a7597c5849393330e.css
State: 3
Object { name="github-09acd31812af6ce1...32a7597c5849393330e.css", url="https://github.global.s...32a7597c5849393330e.css", state=3}

Retry: 2
URL: https://abs.twimg.com/a/1384470786/t1/css/t1_core_logged_out.bundle.css
State: 4
Object { name="t1_core_logged_out.bundle.css", url="https://abs.twimg.com/a...e_logged_out.bundle.css", state=3}
@robert-hoffmann
Member

ok, thank's for testing this ! I'm off for the WE, but will relook into this monday ..for now it seems to be looking good.

I just wish there was a way to test if a browser supported onload on css or not, even a compatibility list would be cool, so we could maybe write a feature detection and base the usage or not of the hack on that.

@xhaggi
xhaggi commented Nov 20, 2013

@robert-hoffmann Did you relook at this?

@robert-hoffmann
Member

No sorry, not yet, been wrapped up with some paperwork

@robert-hoffmann
Member

Ok, i did those tests you showed me, and things seem pretty fine here too.

However i changed the code around a bit as follows

/* Parts inspired from: https://github.com/cujojs/curl
******************************************************/
function loadAsset(asset, callback) {
    callback = callback || noop;

    function error(event) {
        event = event || win.event;

        // release event listeners
        ele.onload = ele.onreadystatechange = ele.onerror = null;

        // do callback
        callback();

        // need some more detailed error handling here
    }

    function process(event) {
        event = event || win.event;

        // IE 7/8 (2 events on 1st load)
        // 1) event.type = readystatechange, s.readyState = loading
        // 2) event.type = readystatechange, s.readyState = loaded

        // IE 7/8 (1 event on reload)
        // 1) event.type = readystatechange, s.readyState = complete

        // event.type === 'readystatechange' && /loaded|complete/.test(s.readyState)

        // IE 9 (3 events on 1st load)
        // 1) event.type = readystatechange, s.readyState = loading
        // 2) event.type = readystatechange, s.readyState = loaded
        // 3) event.type = load            , s.readyState = loaded

        // IE 9 (2 events on reload)
        // 1) event.type = readystatechange, s.readyState = complete
        // 2) event.type = load            , s.readyState = complete

        // event.type === 'load'             && /loaded|complete/.test(s.readyState)
        // event.type === 'readystatechange' && /loaded|complete/.test(s.readyState)

        // IE 10 (3 events on 1st load)
        // 1) event.type = readystatechange, s.readyState = loading
        // 2) event.type = load            , s.readyState = complete
        // 3) event.type = readystatechange, s.readyState = loaded

        // IE 10 (3 events on reload)
        // 1) event.type = readystatechange, s.readyState = loaded
        // 2) event.type = load            , s.readyState = complete
        // 3) event.type = readystatechange, s.readyState = complete

        // event.type === 'load'             && /loaded|complete/.test(s.readyState)
        // event.type === 'readystatechange' && /complete/.test(s.readyState)

        // Other Browsers (1 event on 1st load)
        // 1) event.type = load, s.readyState = undefined

        // Other Browsers (1 event on reload)
        // 1) event.type = load, s.readyState = undefined

        // event.type == 'load' && s.readyState = undefined

        // !doc.documentMode is for IE6/7, IE8+ have documentMode
        if (event.type === "load" || (/loaded|complete/.test(ele.readyState) && (!doc.documentMode || doc.documentMode < 9))) {
            // remove timeouts
            win.clearTimeout(asset.errorTimeout);
            win.clearTimeout(asset.cssTimeout);

            // release event listeners
            ele.onload = ele.onreadystatechange = ele.onerror = null;

            // do callback   
            callback();
        }
    }

    function isCssLoaded() {
        // should we test again ?
        if (asset.state !== LOADED && asset.cssRetries < 30) {

            // loop through stylesheets
            for (var i = 0, l = doc.styleSheets.length; i < l; i++) {

                // do we have a match ?
                if (doc.styleSheets[i].href === asset.url) {
                    process({ "type": "load" });
                    return;
                }
            }

            // increment & try again
            asset.cssRetries++;
            asset.cssTimeout = win.setTimeout(isCssLoaded, 250);
        }
    }

    var ele;
    var ext = getExtension(asset.url);

    if (ext === "css") {
        ele      = doc.createElement("link");
        ele.type = "text/" + (asset.type || "css");
        ele.rel  = "stylesheet";
        ele.href = asset.url;

        /* onload supported for CSS on unsupported browsers
         * Safari windows 5.1.7, FF < 10
         */

        // Set counter to zero
        asset.cssRetries = 0;
        asset.cssTimeout = win.setTimeout(isCssLoaded, 500);         
    }
    else {
        ele      = doc.createElement("script");
        ele.type = "text/" + (asset.type || "javascript");
        ele.src  = asset.url;
    }

    ele.onload  = ele.onreadystatechange = process;
    ele.onerror = error;

    /* Good read, but doesn't give much hope !
     * http://blog.getify.com/on-script-loaders/
     * http://www.nczonline.net/blog/2010/12/21/thoughts-on-script-loaders/
     * https://hacks.mozilla.org/2009/06/defer/
     */

    // ASYNC: load in parallel and execute as soon as possible
    ele.async = false;
    // DEFER: load in parallel but maintain execution order
    ele.defer = false;

    // timout for asset loading
    asset.errorTimeout = win.setTimeout(function () {
        error({ type: "timeout" });
    }, 7e3);

    // use insertBefore to keep IE from throwing Operation Aborted (thx Bryan Forbes!)
    var head = doc.head || doc.getElementsByTagName("head")[0];
    // but insert at end of head, because otherwise if it is a stylesheet, it will not override values
    head.insertBefore(ele, head.lastChild);
}

The reason i switched to timeout() is because setinterval() is know to bail out and break on the slightest error.

The reason i start with a 500ms timeout is to give the browser some time to load in the 1st place, and also i consider that by default the browser DOES support the onload event ..like this, things are only a tad slower on nonsupporting browsers ..so if after the initial 500ms things still aren't loaded we check again every 250ms, up to a total of about 7secs in all.

Let me know what you think and if you see anything wrong with this code.

Thanks

@robert-hoffmann
Member

Slight change in the above code (after unit tests)

doc.styleSheets[i].href === asset.url

becomes

doc.styleSheets[i].href === ele.href

because a local file named ./test.css once assigned to a link element becomes http://...../test.css

Same item:
ele.href = "http://...../test.css"
asset.url = "./test.css"

@robert-hoffmann
Member

Here's the test code. Please test with this file and let me know if all is fine for you: http://headjs.com/dist/1.0.0/head.js

@xhaggi
xhaggi commented Nov 20, 2013

thanks .. i don't have time to test it this week .. will do it next week

@robert-hoffmann
Member

All good in 1.0.3 ..thank's for the help !

@xhaggi xhaggi deleted the unknown repository branch Nov 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment