Skip to content
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

curl-css shouldn't wait for readystate == 'complete' #273

Open
gerges-zz opened this issue Jul 15, 2014 · 7 comments
Open

curl-css shouldn't wait for readystate == 'complete' #273

gerges-zz opened this issue Jul 15, 2014 · 7 comments

Comments

@gerges-zz
Copy link

The curl.js CSS plugin currently waits for document.readyState == 'complete' before resolving it's deferred. In the comments, it's mentioned that Chrome won't apply styles until this time. I think this wait may be over the top. If a page contains many high resolution images, Chrome would likely have had the time to download/parse/apply the styles long before complete is fired. I'm proposing one of the following options

  1. The readystate call is changed to include readystate == 'interactive' (since the document claims to be in an interactive state, allowing background images to continue loading)
  2. The call itself is removed as the load event fires once the stylesheet and all of its imported content has been loaded and parsed. (This is immediately before the styles start being applied to the content)

I was hoping to get some feedback before I worked on a PR, let me know what you think.

@gerges-zz gerges-zz changed the title curl-css shouldn't wait for readystate == 'complete curl-css shouldn't wait for readystate == 'complete' Jul 15, 2014
@unscriptable
Copy link
Member

Hey @gerges,

Thanks so much for offering to do a PR! 🤘

The last time I checked (which was at least a year ago), readyState === 'complete' was the only way because Chrome didn't support readyState === 'interactive'. Feel free to test recent versions of Chrome to see if it does support the interactive phase and if the styles are actually applied at that time.

I'm a bit concerned about Android, too. I can't test it, atm. Perhaps if Chrome supports interactive, then the code could check if readyState is interactive or complete?

I don't understand point 2). Is there a problem?

Also, users can set the cssNoWait: true config option if their JavaScript doesn't rely on specific CSS rules and the page doesn't have FOUC issues. https://github.com/cujojs/curl/wiki/Plugins#plugin-options

@gerges-zz
Copy link
Author

Hey @unscriptable,

So option 2 (and my avoiding cssNoWait) comes from my desire to wait for link onload, but not document complete. In my head if we're at the point where onload has fired we are close enough to link application that curls internal deferred could be resolved.

Even if the interactive readyState happened before some styles where applied, this is still the point where the browser gives control to the user an it makes the most sense to me to resolve the deferred at this point. Even without curl this is the point where the browser considers things "ready enough" to move forward.

If you weren't comfortable breaking backwards compatibility by changing this maybe a secondary config value could be used? cssResloveOnLoad or something?

@unscriptable
Copy link
Member

Even if the interactive readyState happened before some styles where applied

Hey @gerges, I understand this may work for your use case, but returning control to user code at this point will break some other people's code. For instance, code that checks myNode.offsetHeight, would return the wrong value.

If you can devise a test case that shows that the css resource resolves at the right time (i.e. between link onload and document completion), then I welcome your PR. :)

The test/issue37.html file shows an example of how to ensure that a css file is loaded. I think you'll have to slow the document down a lot, though, to be able to verify your code executes in the correct time window. Is there a way to slow down the document deterministically?

@gerges-zz
Copy link
Author

Ahhh, I think I understand your motivation better. As long as CSS resources
are loaded before JS I had assumed (like in the HEAD) that the browser
would choose to block execution of scripts until relevant styles had
loaded. I'll do some tests.

Just to clarify my thinking, a readyState of interactive is when
DomContentLoaded is fired. If everything was in the head, it's the browser
that's protecting you from an incorrect offsetHeight calculation, not
binding to ready (or whatevers cool these days)
On Jul 16, 2014 4:48 AM, "John Hann" notifications@github.com wrote:

Even if the interactive readyState happened before some styles where
applied

Hey @gerges https://github.com/gerges, I understand this may work for
your use case, but returning control to user code at this point will break
some other people's code. For instance, code that checks
myNode.offsetHeight, would return the wrong value.

If you can devise a test case that shows that the css resource resolves at
the right time (i.e. between link onload and document completion), then I
welcome your PR. :)

The test/issue37.html file shows an example of how to ensure that a css
file is loaded. I think you'll have to slow the document down a lot,
though, to be able to verify your code executes in the correct time window.
Is there a way to slow down the document deterministically?


Reply to this email directly or view it on GitHub
#273 (comment).

@gerges-zz
Copy link
Author

Hey @unscriptable,

I didn't have a lot of time today but made a simple test. I've got an express server running with two handlers /static/{path} which loads resources immediately and /timed-static/{millisToWait}/{path} which waits the number of millis before sending it's response.

The test page does the following

  • Include an image in the page which waits 5 seconds before completing
  • Use curl to require a css file which waits 2.5 seconds before completing

I've also patched curl-css to immediately see if the styles are applied when the onload handler is triggered...

        link.onload = function () {
            // @gerges -- THIS CODE IS MODIFIED FOR INSTRUMENTATION
            if (document.getElementById("bar").offsetTop > 200) {
                tracker.trackEvent("link-onload::with-styles-applied");
            } else {
                tracker.trackEvent("link-onload::with-styles-not-applied");
            }
            // @gerges -- END MODIFICATIONS

tracker is a simple event tracker I've written which logs out to the page. Here's what I'm seeing in chrome...

Events were tracked in the following order: ready-state-interactive, link-onload (with-styles-applied), ready-state-complete, curl-deferred-resolved (with-styles-applied)

So at this point it looks like in Chrome when the link.onload is called the browser has already applied styles (or is immediately doing so). Out of curiosity I completely removed the call to waitForDocumentComplete in curl-css and saw the following in Chrome

Events were tracked in the following order: ready-state-interactive, link-onload (with-styles-applied), curl-deferred-resolved (with-styles-applied), ready-state-complete

Here's the code I was using to test. https://github.com/gerges/events-test

Do you think this is an acceptable test? If so I'll script it up and send it to BrowserStack to see how other browsers behave.

@bradleyayers
Copy link

If I understand this correctly Chrome has fixed its behaviour (which was the only known browser to have a problem), which would suggest we can remove the work around in curl that waits for readyState === 'complete'?

(I'm hitting this problem with curl at the moment.)

@unscriptable
Copy link
Member

Would anybody like to submit a PR that removes the readyState work-around?

apetrushin added a commit to apetrushin/curl that referenced this issue Jan 15, 2016
apetrushin added a commit to apetrushin/curl that referenced this issue Jan 15, 2016
apetrushin added a commit to apetrushin/curl that referenced this issue Jan 15, 2016
unscriptable added a commit that referenced this issue Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants