Last Comment Bug 185236 - fire "load" event on stylesheet linking elements when the sheet load finishes or "error" if the load fails
: fire "load" event on stylesheet linking elements when the sheet load finishes...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal with 17 votes (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://ms2ger.freehostia.com/tests/ht...
: 563176 (view as bug list)
Depends on: 115770 693725 188030 690990 696103 713014 713745 714560
Blocks: 495347 111943 460323 563176
  Show dependency treegraph
 
Reported: 2002-12-13 10:21 PST by Boris Zbarsky [:bz]
Modified: 2014-04-26 03:08 PDT (History)
33 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.02 KB, text/html)
2003-01-24 21:49 PST, Boris Zbarsky [:bz]
no flags Details
patch (3.26 KB, patch)
2003-01-24 21:52 PST, Boris Zbarsky [:bz]
john: review+
peterv: superreview+
Details | Diff | Splinter Review
part 1. Fix add-ons manager to ignore load events on stylesheet PIs. (1.33 KB, patch)
2011-09-13 18:00 PDT, Boris Zbarsky [:bz]
bmcbride: review+
Details | Diff | Splinter Review
part 2. Make sure to remove our SheetLoadData from mPostedEvents before calling SheetComplete on it, so that checking for pending loads from inside SheetComplete will work correctly. (2.24 KB, patch)
2011-09-13 18:01 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter 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. (7.73 KB, patch)
2011-09-13 18:02 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter Review
part 4. Pass along the owning element to all sheet load datas, assuming we have one at all. (6.79 KB, patch)
2011-09-13 18:02 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter Review
part 5. Fire load and error events on stylesheet linking elements. (16.49 KB, patch)
2011-09-13 18:22 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2002-12-13 10:21:38 PST
This is just a mental note to do this....
Comment 1 Hixie (not reading bugmail) 2002-12-13 10:44:27 PST
Any way we can make this work with @import links too?
Comment 2 Boris Zbarsky [:bz] 2002-12-13 10:50:28 PST
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?
Comment 3 Boris Zbarsky [:bz] 2003-01-24 21:49:53 PST
Created attachment 112600 [details]
testcase
Comment 4 Boris Zbarsky [:bz] 2003-01-24 21:52:42 PST
Created attachment 112601 [details] [diff] [review]
patch
Comment 5 Daniel Glazman (:glazou) 2003-01-25 13:24:30 PST
Ask for it, bz makes it.
Thanks a lot Boris. That will be VERY helpful.
Comment 6 Peter Van der Beken [:peterv] - away till Aug 1st 2003-01-27 01:42:56 PST
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);
Comment 7 Boris Zbarsky [:bz] 2003-02-11 12:46:53 PST
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....
Comment 8 Boris Zbarsky [:bz] 2003-02-11 12:47:47 PST
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...
Comment 9 Boris Zbarsky [:bz] 2005-09-12 18:58:56 PDT
I'll take another stab at this now that bug 188030 is fixed.
Comment 10 Mike Moening 2008-01-28 15:32:24 PST
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?
Comment 11 Boris Zbarsky [:bz] 2008-01-28 15:37:37 PST
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.
Comment 12 Boris Zbarsky [:bz] 2008-01-28 16:18:00 PST
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?
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2008-01-28 16:24:25 PST
It would make sense to me if we did, for the sake of consistency. And it looks like <script> elements do.
Comment 14 Mike Moening 2008-01-28 16:26:32 PST
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.
Comment 15 Boris Zbarsky [:bz] 2008-01-28 18:59:34 PST
> 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.
Comment 16 Boris Zbarsky [:bz] 2008-01-28 19:34:39 PST
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.
Comment 17 Jonas Sicking (:sicking) PTO Until July 5th 2008-01-28 21:48:32 PST
Firing async sounds ok to me FWIW.
Comment 18 James Burke 2008-02-26 21:22:20 PST
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.
Comment 19 Hixie (not reading bugmail) 2009-03-24 19:25:05 PDT
The HTML5 spec now requires this (async, waits for all subresources, ignores CSS parse errors but fires 'error' for any network error).
Comment 20 Boris Zbarsky [:bz] 2009-03-24 19:38:06 PDT
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?
Comment 21 Boris Zbarsky [:bz] 2009-03-24 20:12:25 PDT
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.
Comment 22 James Burke 2009-03-24 20:48:15 PDT
(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.
Comment 23 Boris Zbarsky [:bz] 2009-03-24 20:56:01 PDT
> 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.
Comment 24 Boris Zbarsky [:bz] 2010-05-02 19:44:21 PDT
*** Bug 563176 has been marked as a duplicate of this bug. ***
Comment 25 Paul Irish 2010-12-19 20:55:03 PST
I've filed a related ticket over on Chrome's tracker: http://crbug.com/67522
Comment 26 :Ms2ger (⌚ UTC+1/+2) 2011-01-14 10:17:51 PST
I might do this at one point.
Comment 27 Jonas Sicking (:sicking) PTO Until July 5th 2011-03-18 13:39:16 PDT
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.
Comment 28 Boris Zbarsky [:bz] 2011-03-18 14:11:48 PDT
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.
Comment 29 Jonas Sicking (:sicking) PTO Until July 5th 2011-03-18 14:25:42 PDT
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?
Comment 30 Boris Zbarsky [:bz] 2011-03-18 14:31:41 PDT
> 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.
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2011-03-18 14:35:06 PDT
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.
Comment 32 Boris Zbarsky [:bz] 2011-03-18 16:25:10 PDT
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.  :(
Comment 33 Jonas Sicking (:sicking) PTO Until July 5th 2011-03-30 15:27:27 PDT
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.
Comment 34 Spocke 2011-06-20 14:22:22 PDT
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.
Comment 35 Boris Zbarsky [:bz] 2011-06-21 06:44:47 PDT
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.
Comment 36 Daniel Buchner [:dbuc] 2011-06-22 16:31:23 PDT
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
Comment 37 Christopher Blizzard (:blizzard) 2011-08-16 13:31:37 PDT
For reference: http://www.phpied.com/when-is-a-stylesheet-really-loaded/
Comment 38 Daniel Buchner [:dbuc] 2011-08-16 13:59:05 PDT
(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(
Comment 39 Boris Zbarsky [:bz] 2011-09-02 11:37:03 PDT
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.
Comment 40 Daniel Buchner [:dbuc] 2011-09-02 11:45:00 PDT
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.
Comment 41 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-02 11:56:19 PDT
It would fire after the stylesheet style rules have been applied, but before we paint.
Comment 42 Boris Zbarsky [:bz] 2011-09-02 12:07:20 PDT
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.
Comment 43 Daniel Buchner [:dbuc] 2011-09-02 12:13:18 PDT
LOL, sorry guys, just wanted to check and make sure. The event sequence as described seems perfect.
Comment 44 Boris Zbarsky [:bz] 2011-09-13 01:25:36 PDT
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.
Comment 45 Boris Zbarsky [:bz] 2011-09-13 18:00:45 PDT
Created attachment 560086 [details] [diff] [review]
part 1.  Fix add-ons manager to ignore load events on stylesheet PIs.
Comment 46 Boris Zbarsky [:bz] 2011-09-13 18:01:33 PDT
Created attachment 560087 [details] [diff] [review]
part 2.  Make sure to remove our SheetLoadData from mPostedEvents before calling SheetComplete on it, so that checking for pending loads from inside SheetComplete will work correctly.
Comment 47 Boris Zbarsky [:bz] 2011-09-13 18:02:08 PDT
Created 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.
Comment 48 Boris Zbarsky [:bz] 2011-09-13 18:02:39 PDT
Created attachment 560089 [details] [diff] [review]
part 4.  Pass along the owning element to all sheet load datas, assuming we have one at all.
Comment 49 Boris Zbarsky [:bz] 2011-09-13 18:22:54 PDT
Created attachment 560093 [details] [diff] [review]
part 5.  Fire load and error events on stylesheet linking elements.
Comment 50 Boris Zbarsky [:bz] 2011-09-14 03:31:17 PDT
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...
Comment 51 Shawn Wilsher :sdwilsh 2011-09-14 15:52:31 PDT
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?
Comment 52 Boris Zbarsky [:bz] 2011-09-14 17:20:19 PDT
> 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...
Comment 53 James Ide 2011-09-14 21:19:59 PDT
(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.
Comment 54 Boris Zbarsky [:bz] 2011-09-14 22:23:41 PDT
> 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.
Comment 55 James Burke [:jrburke] 2011-09-14 23:46:53 PDT
(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.
Comment 56 Boris Zbarsky [:bz] 2011-09-15 00:23:55 PDT
James, I seriously suggest actually reading comment 50....
Comment 57 Spocke 2011-09-15 02:40:33 PDT
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.
Comment 58 Tim McCormack 2011-09-15 05:57:45 PDT
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.
Comment 59 Boris Zbarsky [:bz] 2011-09-15 10:09:45 PDT
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?  ;)
Comment 60 Boris Zbarsky [:bz] 2011-09-15 10:21:29 PDT
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
Comment 61 Daniel Buchner [:dbuc] 2011-09-15 11:05:15 PDT
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.
Comment 62 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-09-23 23:10:29 PDT
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.
Comment 63 Boris Zbarsky [:bz] 2011-09-23 23:11:56 PDT
Thanks!
Comment 64 Peter Van der Beken [:peterv] - away till Aug 1st 2011-09-26 12:15:34 PDT
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()|?
Comment 65 Boris Zbarsky [:bz] 2011-09-26 12:34:34 PDT
> 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).
Comment 66 Peter Van der Beken [:peterv] - away till Aug 1st 2011-09-26 13:33:40 PDT
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.
Comment 67 Peter Van der Beken [:peterv] - away till Aug 1st 2011-09-26 13:46:36 PDT
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?
Comment 68 Boris Zbarsky [:bz] 2011-09-26 14:13:39 PDT
> Not sure I understand this, did you mean "our load event"?

Yes, I do.  Fixed.

>seven?
>Sixth?

Er, yes.  This test expanded over time.  ;)
Comment 71 Eric Shepherd [:sheppy] 2011-11-08 07:59:52 PST
Documented:

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

Listed on Firefox 9 for developers.

Note You need to log in before you can comment on or make changes to this bug.