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

Latest less.js (2.1.1) behaves async in IE #2317

Closed
lukeapage opened this issue Nov 28, 2014 · 8 comments · Fixed by #2575
Closed

Latest less.js (2.1.1) behaves async in IE #2317

lukeapage opened this issue Nov 28, 2014 · 8 comments · Fixed by #2575

Comments

@lukeapage
Copy link
Member

Latest less.js (2.1.1) behaves async in IE (ver 11) even when async:false is set explicitly.
The file processed is complex less with multiple @import commands that take about 1 second to load and process on dev machine, during which time IE shows style-free html. Firefox and Chrome behave correctly. Had to revert to 1.7.5 which behaves correctly.
Perhaps related to the recent promises changes? Perhaps setTimeout(function(){}, 0); allows IE to proceed in parallel and partially render the page?

@lukeapage
Copy link
Member Author

reported by @efbiaiinzinz

Is it IE11 only or every browser?

@indreka
Copy link

indreka commented Nov 29, 2014

Tested more browsers:
Async appears in IE11 (unstyled html visible for 1-2 seconds for big less file with multiple big imports), also appears when I put IE into IE10 mode.
Works correctly in IE9 mode (and also IE8 and IE7 mode).
Also works correctly in Chrome.
Same unstyled html view appears for a short time in Firefox.

Using 1.7.5 the page render is correctly blocked in all browsers until all less is downloaded and processed.

@lukeapage
Copy link
Member Author

Thanks, I can investigate it next, but it might be a week or so as am busy
this w.e.

@indreka
Copy link

indreka commented Nov 29, 2014

Thanks for the fast response. We were trying to create cachebusting functionality (plugin) for @import statements (append ?_=timestamp before requesting the files to skip the cache clearing needs) when we discovered it, but it's not a major issue that you should spend your weekend on :)

@indreka
Copy link

indreka commented Feb 5, 2015

Latest less.js (2.3.1) still works quirky in the Firefox. Works correctly in Chrome and surprisingly also works correctly in IE11.
Examples of page loading in FF:
Plain html without ay style:
image
Partial style:
image
Final rendering:
image

In IE and Chrome, the page stays blank until less is processed and then does the final rendering at once.

@chipx86
Copy link
Contributor

chipx86 commented Apr 27, 2015

We've hit this problem as well, and can verify this behavior on Safari and Firefox (haven't tried IE yet). Chrome works fine. I've tried every version from 2.0 beta 1 on through 2.5, and this problem exists in all of them.

So, I bisected the tree, and this appears to have been introduced in d31be57.

This problem is more than just annoying. It's actually caused some very bad interactions with some JavaScript on the page, which expected CSS to be loaded properly at the time of execution. Unfortunately, that means we're stuck on 1.7.5 for the time-being (not a huge deal, though we are hitting some bugs with it that have been fixed since..).

I spent some time investigating this change, the referenced bug numbers, and the pull request introducing it. The intent, it seems, was to add a function (less.registerStylesheets) that other callers can call that can return a Promise and then allow the caller to call less.refresh or whatever it wants. That seems like a great addition, but it's unclear whether it was intended to change the initial load behavior.

I've changed this locally (just to test) to move the bulk of less.registerStylesheets out into a less.registerStylesheetsImmediately, and to have less.registerStylesheets wrap that in a Promise. Then, the initial page load calls less.registerStylesheetsImmediately, followed by less.refresh, without using promises. This returns to the original loading behavior from 1.7.5 in Firefox. I'm happy to supply a pull request for that, if the general idea sounds good.

Unfortunately, that didn't solve the issue entirely for Safari (though it definitely improved it). More bisecting led me to commit 544bd3a. It's 4:15AM though, and I'm falling asleep, so that's as far as I'm getting tonight.

Hopefully that helps someone in diagnosing the cause. Would love to see this fixed :)

@chipx86
Copy link
Contributor

chipx86 commented Apr 27, 2015

Following up on this.

The Safari-specific issue I mentioned was noticed when I had tested my fix on top of commit d31be57. Having now ported my patch up to 2.5.0, I realize that the Safari issue had already been fixed in some prior commit, which is great.

I'm working on a patch for the above solution, assuming it sounds fine.

chipx86 added a commit to chipx86/less.js that referenced this issue Apr 28, 2015
Starting in 2.0, stylesheet loading became asynchronous, through the
usage of promises for both calculating the list of stylesheets and the
initial call to less.refresh(). This resulted in visual issues while
loading on some browsers (noticed in Firefox and Safari), along with
breakages of any custom JavaScript that depended on the computed style
of elements on the page, due to race conditions.

This change preserves the promise for initial page loading, in order to
retain support for less.pageLoadFinished, but immediately executes the
stylesheet scan (through a new less.registerStylesheetsImmediately
function) and the less.refresh() call. That resulting behavior matches
versions of less prior to 2.0.

This unveiled a regression in registering functions, both in the browser
and in unit tests, that was not previously noticed due to the
asynchronous load. Registered functions would have a 'less' variable set
to the less options, and not less itself, when not going through the
asynchronous loading mode. This meant that both unit tests and
real-world function registration would break when the sync page loading
was fixed. Overriding window.less to point to the actual less module and
not less.options during bootstrap fixes this.

This fixes less#2317.
@chipx86
Copy link
Contributor

chipx86 commented Apr 28, 2015

Just a note on the change I have up for review: There's a bug it uncovered in GitHub's diff viewer, causing a line to disappear from the change. Right before this line:

less.pageLoadFinished = less.refresh(less.env === 'development');

There's:

less.registerStylesheetsImmediately();

Feel free to verify from my tree. Already reporting it to GitHub.

chipx86 added a commit to chipx86/less.js that referenced this issue Apr 28, 2015
Starting in 2.0, stylesheet loading became asynchronous, through the
usage of promises for both calculating the list of stylesheets and the
initial call to less.refresh(). This resulted in visual issues while
loading on some browsers (noticed in Firefox and Safari), along with
breakages of any custom JavaScript that depended on the computed style
of elements on the page, due to race conditions.

This change preserves the promise for initial page loading, in order to
retain support for less.pageLoadFinished, but immediately executes the
stylesheet scan (through a new less.registerStylesheetsImmediately
function) and the less.refresh() call. That resulting behavior matches
versions of less prior to 2.0.

This unveiled a regression in registering functions, both in the browser
and in unit tests, that was not previously noticed due to the
asynchronous load. Registered functions would have a 'less' variable set
to the less options, and not less itself, when not going through the
asynchronous loading mode. This meant that both unit tests and
real-world function registration would break when the sync page loading
was fixed. Overriding window.less to point to the actual less module and
not less.options during bootstrap fixes this.

This fixes less#2317.
chipx86 added a commit to chipx86/less.js that referenced this issue Apr 28, 2015
Starting in 2.0, stylesheet loading became asynchronous, through the
usage of promises for both calculating the list of stylesheets and the
initial call to less.refresh(). This resulted in visual issues while
loading on some browsers (noticed in Firefox and Safari), along with
breakages of any custom JavaScript that depended on the computed style
of elements on the page, due to race conditions.

This change preserves the promise for initial page loading, in order to
retain support for less.pageLoadFinished, but immediately executes the
stylesheet scan (through a new less.registerStylesheetsImmediately
function) and the less.refresh() call. That resulting behavior matches
versions of less prior to 2.0.

This unveiled a regression in registering functions, both in the browser
and in unit tests, that was not previously noticed due to the
asynchronous load. Registered functions would have a 'less' variable set
to the less options, and not less itself, when not going through the
asynchronous loading mode. This meant that both unit tests and
real-world function registration would break when the sync page loading
was fixed. Overriding window.less to point to the actual less module and
not less.options during bootstrap fixes this.

This fixes less#2317.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants