Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Performance of invalidating all stylesheets #85

Closed
paulirish opened this issue Aug 21, 2015 · 10 comments
Closed

Performance of invalidating all stylesheets #85

paulirish opened this issue Aug 21, 2015 · 10 comments
Assignees
Milestone

Comments

@paulirish
Copy link

Background

Browsers are built to optimize appending new styles to the end. That's why when you apply an inline style, browsers don't have to recompute the styles of the world in order to apply it.
The model is built to cheaply apply additive style rules; the browser takes the current CSSOM (basically) and applies the new css on top of it. It's cool.

Because CSS order matters, if new CSS is added in the middle (let's say halfway down), then the browser invalidates the style computation of 50% of the styles and has to recompute due to the potential cascade changes.

Current state

loadCSS injects the script like so:

var ref = before || window.document.getElementsByTagName( "script" )[ 0 ];
ref.parentNode.insertBefore( ss, ref );

This injection is better than adding to the very top of the head, but there's a good chance it is before other styles.

In discussions with Flipkart, the performance disadvantage they encountered with this was significant. I would expect similar results amongst publishers with plenty of DOM, the folks that typically would be eager to use loadCSS.

Proposal

I'd recommend placing the injected sheet at the end of all recognized styles. (Bonus: subsequent loadCSS calls will be generally placed after eachother, which is probably what folks would expect)

So I'd go for something like this:

var elems = document.querySelectorAll('style,link[rel=stylesheet]');
var ref = elems.item(elems.length - 1);
ref.parentNode.insertBefore(ss, ref.nextSibling);

It finds all external stylesheets and inline styles, gets the last one (in DOM order), then adds our ss right after it.


hows that sound?

@scottjehl
Copy link
Member

Hey @paulirish

Thanks for the detailed writeup. Sounds reasonable to me for the default. That said, we've been recommending the use of the before argument in loadCSS to specify a particular element to insert the CSS before, and we recommend passing in the script element that holds the loadCSS function as one easy way to do that (since it's in a dependable place in the head.

Here's an example of that usage: https://github.com/filamentgroup/loadCSS#optional-arguments

It comes with the benefit of knowing that browser extensions can't make your inject location inconsistent too, which is nice.

Given that that option is there and recommended, would you say we should keep that recommendation in place or just go with this new default you've recommended above?

Thanks

@whjvenyl
Copy link

So I'd go for something like this:

var elems = document.querySelectorAll('style,link[rel=stylesheet]');
var ref = elems.item(elems.length - 1);
ref.parentNode.insertBefore(ss, ref.nextSibling);
It finds all external stylesheets and inline styles, gets the last one (in DOM order), then adds our ss right after it.

Not having any other style-definitions on a page beside the web-font we try to load, this code would fail.

@scottjehl
Copy link
Member

Fair point, @whjvenyl, even if that's a rare case.

Including script in that lookup should cover most cases I'd think.

Speaking of rare cases, we'll want this script to continue working in browsers that don't have querySelector support too, so the actual lookup for this will be a little more verbose...

Which leads me back to wondering if it might be best to simply start requiring the before argument and letting folks define their insertion point specifically. This can be done pretty easily by adding an ID to the script element that contains loadCSS, as our docs show, or by adding a meta tag or some other element as a placeholder reference for insertion.

Thoughts on requiring that arg instead of making the script cleverer, @paulirish ?

scottjehl added a commit that referenced this issue Aug 24, 2015
Contains comment updates as well.
@scottjehl scottjehl mentioned this issue Aug 24, 2015
Closed
@scottjehl
Copy link
Member

Okay, @paulirish : how's this PR look?
#87

@scottjehl scottjehl modified the milestone: 1.0.0 Aug 24, 2015
@paulirish
Copy link
Author

hey scott, thanks.

I think we could do slightly better. I'm concerned that just requiring before won't reduce the chance that browser styles are recalculated. Plus requiring the insertion point kinda sucks for users.

How about...

  • attempt to place after last stylesheet.
  • If no results or no qSA, fall back to before last script.
  • if before provided, place before given element

@scottjehl
Copy link
Member

@paulirish sounds reasonable, and I like the idea of keeping the 2nd arg optional. Cool, cool.

@scottjehl scottjehl self-assigned this Aug 27, 2015
@scottjehl
Copy link
Member

@paulirish any pro/con to the item() method in your example over say, [] reference syntax? That one's actually new to me (thanks! :) )

scottjehl added a commit that referenced this issue Aug 27, 2015
…lesheet or script in the page if qsa is supported. Otherwise it inserts after the last script. In either case, the insertBefore is performed in the reference element's nextSibling, which serves as an "insertAfter" regardless of whether the reference element has a nextSibling or not. Despite the default behavior change, we've preserved the behavior of the "before" argument, if provided, so that the link is still inserted before that specific element in that case. We may want to standardize on inserting "after" in a later release. Fixes #85.
scottjehl added a commit that referenced this issue Aug 27, 2015
@scottjehl scottjehl mentioned this issue Aug 27, 2015
Merged
@scottjehl
Copy link
Member

@paulirish I was looking to pack this up for 0.2 and had a quick question. The QSA check is a small bummer to me because it leaves the potential for a busted CSS cascade in non-qsa browsers, unless the before argument is there to specify a specific placement.

Of course, non-qsa browsers are rarer now, but I think as a bare minimum, loadCSS should still try to preserve cascade order in a browser that doesn't have qsa, which means before is still needed with the current code.

That got me thinking that maybe there is a non-qsa approach to what we're trying to do. For example, something like this could give us an acceptable node to append after... basically, the last node in the body or head.

var scope = document.body || document.getElementsByTagName( "head" )[ 0 ];
var lastElement = scope.childNodes[ scope.childNodes.length - 1 ]; 

Alternatively, a lookup like document.getElementsByTagName('*'); could get us to the last element node too (but I dunno if that lookup is slow or perhaps aliased internally).

Anyway, have any thoughts on the snippet above in place of the qsa usage?

@scottjehl
Copy link
Member

new related issue #100

@scottjehl
Copy link
Member

Okay @paulirish your change is in version 0.2.0, along with that newer fallback so that it'll inject after all scripts and styles even if qsa is not supported. thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants