Boris Zbarsky [:bzbarsky] Assignee
	Description • 22 years ago

This is just a mental note to do this....

	Hixie (not reading bugmail)
	Comment 1 • 22 years ago

Any way we can make this work with @import links too?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 22 years ago

Just to clarify, as far as the CSS loader is concerned, a sheet is not done
loading until all things it @imported are done loading.  Therefore if I create a
<style> node with at text node inside it that says "@import url(foo.css);" and I
insert this <style> node in the document, the load event will not fire until
foo.css has finished loading and been parsed (and any @imports from it have been
processed and such).  Basically, the load event will fire at the moment when we
start applying the sheet to the document.

Does that address your question?  Or were you talking about dynamic insertion of
@import rules into an existing fully loaded sheet?

	Daniel Glazman (:glazou) (not active in Mozilla any more)
	Comment 5 • 22 years ago

Ask for it, bz makes it.
Thanks a lot Boris. That will be VERY helpful.

	Peter Van der Beken [:peterv]
	Comment 6 • 22 years ago

Comment on attachment 112601 [details] [diff] [review]
patch

>+    // Fire the load event
>+    if (data->mOwningElement && mDocument) {
>+      nsCOMPtr<nsIDOMEventTarget> eventTarget(do_QueryInterface(data->mOwningElement));
>+      nsCOMPtr<nsIDOMDocumentEvent> doc(do_QueryInterface(mDocument));
>+      if (eventTarget && doc) {

There's not really a need for the first if imho. Just do_QI and then check for
non-null eventTarget and doc. I also prefer

nsCOMPtr<nsIBlah> foo = do_QI(bar);

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 22 years ago

Comment on attachment 112601 [details] [diff] [review]
patch

Hmm... with this patch sheets that are in the "loaded sheets" cache will not
get onload fired on them....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 22 years ago

I think the stuff in bug 188030 will need to land in order to clean up the
sync/async issues before we can do this...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 19 years ago

I'll take another stab at this now that bug 188030 is fixed.

	Mike Moening
	Comment 10 • 17 years ago

Hey Boris,

Will this bug be fixed as part of FireFox 3.0?
Rigth now we have no way of determining if a dynamically loaded style sheet is done loading.
Do you have a work around for this?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 17 years ago

Hmm.  I can try to resurrect this patch.  If it's easy to make it work, and if this gets approval, then it'll make Gecko 1.9.

I don't have a workaround, short of polling computed style or something.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 17 years ago

Hmm.  This is still no quite obvious (though easier than before).  In particular, should a load event fire for an inline stylesheet that just gets parsed and doesn't @import anything?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 13 • 17 years ago

It would make sense to me if we did, for the sake of consistency. And it looks like <script> elements do.

	Mike Moening
	Comment 14 • 17 years ago

Here's what we are doing:

var link = document.createElement("link");
link.href="mycss.css";
link.type="text/css";
link.rel="stylesheet";
link.onload=function() { alert("GOT IT!"); }
document.getElementsByTagName("head")[0].appendChild(link);

So would like to attach an onload event handler like above.
I guess I agree with Jonas about the consistency thing.
However, if that is too hard just make the above code work.
Thanks.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 17 years ago

> However, if that is too hard just make the above code work.

Sorry, a half-implemented feature is usually worse than none, both in terms of the time investment and the author experience.

I'll probably need to make inline style fire a load event async, even if it's done parsing synchronously.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 17 years ago

Yeah, I looked at this some more, and doing it safely is not obviously trivial.  So I doubt it'll happen for Gecko 1.9 at this late stage and given the small amount of time I have to spend on Mozilla stuff.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 17 years ago

Firing async sounds ok to me FWIW.

	James Burke
	Comment 18 • 16 years ago

This would be helpful for the Dojo JavaScript toolkit if/when this feature lands. I'm trying to incorporate a feature where we can fire some code when a style sheet loaded via a dynamically added link tag finishes loading.

Right now I'm trying if(linkNode.sheet.cssRules) as an indicator that the stylesheet loaded, but:

1) Accessing sheet.cssRules throws a security exception when the link tag's href is on a different domain than the page.

2) It is not clear if the existence of cssRules means the sheet has loaded, including any @import calls.

If/when this feature lands, I'm hoping it works for dynamically added link nodes as described in Comment #14.

	Hixie (not reading bugmail)
	Comment 19 • 15 years ago

The HTML5 spec now requires this (async, waits for all subresources, ignores CSS parse errors but fires 'error' for any network error).

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 15 years ago

It's not obvious from the spec there whether these "load" and "error" events are allowed to fire after the "load" event for the window.  I would assume not, yes?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 15 years ago

Implementation note, in case someone else picks this up: the spec requires an error event on the DOMElement if any import from the sheet fails.  This means several things:

1)  We need to propagate child SheetLoadData load status up to parent
    SheetLoadData (probably in a new member).
2)  We need to track the load status on the sheet itself so that clones work
    correctly in terms of firing error instead of load (this is needed for
    toplevel sheets too, really).

It should be pretty easy to write mochitests for both, of course.

	James Burke
	Comment 22 • 15 years ago

(In reply to comment #20)
> It's not obvious from the spec there whether these "load" and "error" events
> are allowed to fire after the "load" event for the window.  I would assume not,
> yes?

If the tag was added after the page load dynamically through script, I would expect load and error on the stylesheet to fire. 

This is useful for Dojo since we want to delay loading some resources, but we want the style sheet loaded before we create some DOM nodes and ask for style info/dimensions/coordinates on the nodes. In that case we would wait for the style sheet load event, then proceed with the getting the style info for a node.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 15 years ago

> If the tag was added after the page load dynamically through script, I would
> expect load and error on the stylesheet to fire. 

Yes, yes.  That's not the issue I was talking about.  The spec defines these events to fire asynchronously, whereas the page load event fires synchronously at completion of network activity in Gecko.  Therefore, unless special precautions are taken otherwise, if a stylesheet is the last thing loading its load event will fire after the page's load event (the latter synchronously at load completion, the former asynchronously from load completion).  It's not clear to me from the spec whether said special precautions need to be taken.

	Paul Irish
	Comment 25 • 14 years ago

I've filed a related ticket over on Chrome's tracker: http://crbug.com/67522

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 26 • 13 years ago

I might do this at one point.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 27 • 13 years ago

Boris: Any way we could simply send the event synchronously? The advantage from a user point of view is that it gives the page the chance to react to any layout changes caused by applying the stylesheet, before things are actually rendered on screen. I.e. do things like update javascript driven layout elements.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 13 years ago

In this case "synchronously" would mean "under BindToTree" in many cases.  I really don't think you want synchronous.

I suppose we could do it off a script runner, but then you get issues like this.  If you create a <link> and put it in the DOM and then set up the load listener.. it's too late.  The event has already fired.  Unless we're still loading it, and then it will work.  This makes it easy to write code that works in some cases but not others depending on what we have cached where...

One possible option is to make the event fire off the refresh driver.  That would guarantee that it runs before the next time we reflow/etc, right?  The problem is speccing it.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 29 • 13 years ago

Yeah, script runner was my initial thought too.

One option would be to make sure that inserting a <link> never synchronously applies the stylesheet. I'm a little bit surprised that that can apply the stylesheet synchronously since even hitting the http cache normally generates asynchronous callbacks.

Under what conditions can a <link> "load" synchronously?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 30 • 13 years ago

> Under what conditions can a <link> "load" synchronously?

css::Loader has its own cache of already-loaded sheets (plus there's the xul proto cache for chrome sheets); if you insert a <link> pointing to such a sheet, then it will just be ready immediately.  See the eSheetComplete case in Loader::LoadStyleLink.

In this case the "restyle the document" event is posted synchronously before LoadStyleLink returns; the actual restyle will of course happen whenever it happens.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 31 • 13 years ago

How would you feel about making that stylesheet apply asynchronously even in that case (probably with an exception for XUL)?

It would make page behavior more consistent, but could possibly slow things down.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 32 • 13 years ago

That would take some significant surgery, actually.  Right now we insert a sheet into the document immediately when the load starts.  It starts applying once it's "ready", but the ready state is shared by all clones, so if you're cloning an already-ready sheet you end up with a ready sheet.

We could add another member to nsCSSStyleSheet to track when its "ready" state differs from its underlying data's readiness, I guess, and then make sure that we lie to things and claim to not be ready as needed...  And then have a separate codepath for the XUL case.  :(

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 33 • 13 years ago

Here is what I think we should do for now:

1. If a stylesheet is applied synchronously, we fire the event from a script runner ASAP.

2. If it's not applied synchronously, fire the event as soon as the stylesheet is added to the doc, if there is no-one listening to the event, or if the event doesn't read style/layout data we won't do any extra work compared to today.


Ideally I think we should get rid of 1, but that can be done separately from this bug.

	Spocke
	Comment 34 • 13 years ago

Any chance that this will be added to the trunk in the near future since it's in the HTML5 spec and both IE and Opera supports it?

I've seen a lot of hacks on the net for lazy loading stylesheets so it seems that it's something a lot of people want to do and I think this is a very important feature when it comes to web application development for example lazy loading a widget and doing custom layout using JS on the rendered contents.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 35 • 13 years ago

I'm not likely to get to this for at least two months.  So if it's going to happen before then, someone needs to step up and do it.

	Daniel Buchner [:dbuc]
	Comment 36 • 13 years ago

There was a question as to whether I was volunteering to fix this - I would, but I have never worked on the platform before, not sure it would be the best option.

This is still a bug in all webkit browsers - oddly IE has the best support, followed by Opera.

Here are the associated bug tickets for Chromium and Webkit:

http://code.google.com/p/chromium/issues/detail?id=67522
https://bugs.webkit.org/show_bug.cgi?id=38995

	Christopher Blizzard (:blizzard)
	Comment 37 • 13 years ago

For reference: http://www.phpied.com/when-is-a-stylesheet-really-loaded/

	Daniel Buchner [:dbuc]
	Comment 38 • 13 years ago

(In reply to Christopher Blizzard (:blizzard) from comment #37)
> For reference: http://www.phpied.com/when-is-a-stylesheet-really-loaded/

According to W3C spec, we should be firing a load event. The method that was linked in the previous comment offers, IMO, a terribly hackish fix for any event deficiency: use a timer to ping something a bunch.

It is worth noting that IE adheres to the standard and supports this event...since IE 5.5  X(

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 39 • 13 years ago

Summarizing irc discussion with sicking:

We want to fire the load off a thread observer, both for start and end of event.  That guarantees that we fire it before we paint with the new style data.

	Daniel Buchner [:dbuc]
	Comment 40 • 13 years ago

Does that mean the event would fire before the values are parsed from the sheet and applied to the DOM objects? I ask because it is crucial that the event fires *after* that occurs.

One of the most frustrating things about making a UI widget framework that is consumed on-demand is that you can dynamically load your JS/HTML and CSS but if you access the dimensions properties with the JS before the parse applies the new values to the objects, you will be returned incorrect values if the sheet has not yet taken effect. A good example is elements that derive their height from the in-bound sheet - if you check their height in JS prior to the sheet being applied, it returns a value of 0.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 41 • 13 years ago

It would fire after the stylesheet style rules have been applied, but before we paint.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 42 • 13 years ago

Daniel, give us some credit here for not being completely out of it.  ;)

Of course the event will fire after the sheet has actually been applied.  The only question was how _much_ after.

	Daniel Buchner [:dbuc]
	Comment 43 • 13 years ago

LOL, sorry guys, just wanted to check and make sure. The event sequence as described seems perfect.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 44 • 13 years ago

I have a patch for this; it's running on try server right now to see whether the leaks initial testing showed it causing are fixed.  If so (and I fully expect it to be so), I'll post it tomorrow morning.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 50 • 13 years ago

Peter, one review note.  Right now we fire an error event or load event for all sheets, including cross-site ones.  This may be a privacy leak; maybe we should only fire error events for cross-site sheets, or fire nothing at all...

	Shawn Wilsher :sdwilsh
	Comment 51 • 13 years ago

Comment on attachment 560086 [details] [diff] [review]
part 1.  Fix add-ons manager to ignore load events on stylesheet PIs.

Review of attachment 560086 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/extensions.js
@@ +116,5 @@
>  __defineGetter__("gIsInitializing", function() gPendingInitializations > 0);
>  
> +function initialize(event) {
> +  // XXXbz this listener gets _all_ load events for all nodes in the
> +  // document... but relies on not being called "too early".

Would it be worthwhile to change the event to DOMContentLoaded then?  Would that still dispatch the CSS load event then?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 52 • 13 years ago

> Would it be worthwhile to change the event to DOMContentLoaded then?

Possibly.  I really have no idea what that code was trying to do, so I'm trying to just change it as little as possible in this bug, and I asked about it in the bug that introduced the code.

Using DOMContentLoaded would certainly not trigger from the CSS load event, but might still fire "too early" for the addons manager...

	James Ide
	Comment 53 • 13 years ago

(In reply to Boris Zbarsky (:bz) from comment #50)
> Peter, one review note.  Right now we fire an error event or load event for
> all sheets, including cross-site ones.  This may be a privacy leak; maybe we
> should only fire error events for cross-site sheets, or fire nothing at
> all...

Since many websites serve their stylesheets from different domains or a CDN, does it make sense to relax the same-origin policy here? I did not see the spec mention the SOP here. I think a reasonable compromise would be to honor CORS headers if it came to that. (Related: Accessing CSSStyleSheet.cssRules throws a SECURITY_ERR exception for cross-origin stylesheets, even if the appropriate CORS header is set.)

A small test also shows that Firefox 6 does not fire the abort event when the user terminates the page load. I believe the spec includes stylesheet loading under the "fetch algorithm" for which the abort event should be fired: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#aborting-a-document-load.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 54 • 13 years ago

> I did not see the spec mention the SOP here.

Sure.  The spec has been known to be wrong.

We've had security issues with the fact that we fire load for cross-site images; people can use that right now to sniff information about users.  I'd rather not add to that attack surface, in some ways.

And yes, the fact that CDNs or resource domains are used is a concern.

> even if the appropriate CORS header is set.

The appropriate CORS header being set is no good unless we're doing the whole CORS setup on our end... and if we did that and the site did NOT send the header the load would need to be aborted.  So the only way to do CORS for stylesheets is to make it opt-in in the markup, like we did for images.

> A small test also shows that Firefox 6 does not fire the abort event

Sure.  That's completely unrelated to this bug.

	James Burke [:jrburke]
	Comment 55 • 13 years ago

(In reply to Boris Zbarsky (:bz) from comment #54)
> We've had security issues with the fact that we fire load for cross-site
> images; people can use that right now to sniff information about users.  I'd
> rather not add to that attack surface, in some ways.

Loading a CSS file seems to have the same sort of data leakage as loading a script: both have observable effects outside receiving a load event. So, I can see treating load notification on CSS files the same as script load notification.

For what it is worth, IE 6-9 (do not have access to 10) and Opera fire load for cross domain CSS. Webkit does not fire load for either same domain or cross domain scripts, so it is not really helpful in this case. Test here:

http://www.tagneto.org/blogcode/185236/loadtest.html

If supporting cross domain load notification is not possible as part of this ticket, it would be good to know what steps would be needed to get it in (different ticket, what criteria you need from a standards org). 

But without cross domain load notification this is not really useful for JS toolkit developers.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 56 • 13 years ago

James, I seriously suggest actually reading comment 50....

	Spocke
	Comment 57 • 13 years ago

I think it's super important to fire load events cross domain. Most people that uses the onload event on IE or odd workarounds for other browsers are widget or framework developers these tends to be hosted on a central CDN like Google CDN or similar. I think it should work in the same way as script elements and IE/Opera.

However it could work in the same way as today where you will get an security exception when you try to access the actual contents of the CSS the cssRules etc.

Most of the usage patterns I've seen for this is when a widget/framework want to do custom layout or measure elements after the CSS for it has been lazy loaded. For example in the TinyMCE case it would load the TinyMCE editor script cross domain than the CSS once both are loaded it would do UI layout and measure elements.

Since most Web 2.0 applications these days are Ajax based users would probably want to lazy load our component in when needed and without a page reload.

	Tim McCormack
	Comment 58 • 13 years ago

I don't think that firing a load event for cross-domain stylesheets is a security concern, because one could always just poll the styles of an element that is known to be affected by the stylesheet in question.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 59 • 13 years ago

Tim, that involves knowing something about the stylesheet; how did you get that information, exactly, for a cross-domain stylesheet whose contents depend on user cookies?  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 60 • 13 years ago

As far as comment 57 goes....  I know what the use cases are.  That's why the patch is written the way it is.  But since you mention script elements, please see https://grepular.com/Abusing_HTTP_Status_Codes_to_Expose_Private_Information

	Daniel Buchner [:dbuc]
	Comment 61 • 13 years ago

Having read over the last few comments, I would like to reiterate (as Boris and others have) that the events provided for in this patch should *absolutely* fire on all sheets regardless of origin (as the patch does now). Same origin here would basically confound the entire purpose of the patch and fail to serve the basic use case.

	Blair McBride [:Unfocused] (UNAVAILABLE)
	Comment 62 • 13 years ago

Comment on attachment 560086 [details] [diff] [review]
part 1.  Fix add-ons manager to ignore load events on stylesheet PIs.

Review of attachment 560086 [details] [diff] [review]:
-----------------------------------------------------------------

Eh, yea, it's not great, but fixing it properly seems way out of scope of this bug. So I'm happy to have a temporary bandaid fix here just so nothing breaks - filed bug 688948 for a more permanent (and sane) solution.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 63 • 13 years ago

Thanks!

	Peter Van der Beken [:peterv]
	Comment 64 • 13 years ago

Comment on attachment 560089 [details] [diff] [review]
part 4.  Pass along the owning element to all sheet load datas, assuming we have one at all.

Review of attachment 560089 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/Loader.cpp
@@ +2131,4 @@
>  {
>    LOG(("css::Loader::PostLoadEvent"));
>    NS_PRECONDITION(aSheet, "Must have sheet");
> +  NS_PRECONDITION(aObserver || aElement, "Must have observer or element");

This seems correct given the old code, but wouldn't this fire for the callers that check |aObserver || !mObservers.IsEmpty()|?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 65 • 13 years ago

> but wouldn't this fire for the callers that check |aObserver || !mObservers.IsEmpty()|?

Good catch.  Added a !mObservers.IsEmpty() to the precondition (as part of patch 3, actually).

	Peter Van der Beken [:peterv]
	Comment 66 • 13 years ago

Comment on attachment 560088 [details] [diff] [review]
part 3.  Add a way to differentiate SheetComplete calls for actual loads and for the fake SheetLoadData we use to trigger observer notifications for already-complete sheets.

r+ with the problem from comment 64 fixed.

	Peter Van der Beken [:peterv]
	Comment 67 • 13 years ago

Comment on attachment 560093 [details] [diff] [review]
part 5.  Fire load and error events on stylesheet linking elements.

Review of attachment 560093 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/Loader.cpp
@@ +462,5 @@
> +SheetLoadData::OnProcessNextEvent(nsIThreadInternal* aThread,
> +                                  PRBool aMayWait,
> +                                  PRUint32 aRecursionDepth)
> +{
> +  // We want to fire out load even before or after event processing,

Not sure I understand this, did you mean "our load event"?

::: layout/style/test/test_load_events_on_stylesheets.html
@@ +111,5 @@
> +}
> +document.body.appendChild(link);
> +
> +// Make sure we have that last stylesheet
> +is(document.styleSheets.length, 7, "Should have three stylesheets");

seven?

@@ +113,5 @@
> +
> +// Make sure we have that last stylesheet
> +is(document.styleSheets.length, 7, "Should have three stylesheets");
> +
> +// Make sure that the second stylesheet is all loaded

Sixth?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 68 • 13 years ago

> Not sure I understand this, did you mean "our load event"?

Yes, I do.  Fixed.

>seven?
>Sixth?

Er, yes.  This test expanded over time.  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 69 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb87ceae27a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/465554cdfbcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c05e937de5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/231da6eb8224
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e237c03adb

	Ed Morley [:emorley]
	Comment 70 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/bb87ceae27a5
https://hg.mozilla.org/mozilla-central/rev/465554cdfbcc
https://hg.mozilla.org/mozilla-central/rev/2c05e937de5d
https://hg.mozilla.org/mozilla-central/rev/231da6eb8224
https://hg.mozilla.org/mozilla-central/rev/c8e237c03adb

	Eric Shepherd [:sheppy]
	Comment 71 • 13 years ago

Documented:

https://developer.mozilla.org/en/HTML/Element/link#Stylesheet_load_events

Listed on Firefox 9 for developers.