fire "load" event on stylesheet linking elements when the sheet load finishes or "error" if the load fails

RESOLVED FIXED in mozilla9

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla9
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 1 obsolete attachment)

This is just a mental note to do this....
Any way we can make this work with @import links too?
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?
Created attachment 112600 [details]
testcase
Created attachment 112601 [details] [diff] [review]
patch
(Assignee)

Updated

15 years ago
Priority: -- → P1
Summary: fire "load" event on stylesheet linking elements when the sheet load finishes → [FIX]fire "load" event on stylesheet linking elements when the sheet load finishes
Target Milestone: --- → mozilla1.3final

Updated

15 years ago
Attachment #112601 - Flags: review+
(Assignee)

Updated

15 years ago
Attachment #112601 - Flags: superreview?(peterv)
Ask for it, bz makes it.
Thanks a lot Boris. That will be VERY helpful.
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);
Attachment #112601 - Flags: superreview?(peterv) → superreview+
(Assignee)

Updated

15 years ago
Summary: [FIX]fire "load" event on stylesheet linking elements when the sheet load finishes → [FIXr]fire "load" event on stylesheet linking elements when the sheet load finishes
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....
Attachment #112601 - Attachment is obsolete: true
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...
Depends on: 188030
Target Milestone: mozilla1.3final → mozilla1.4alpha
(Assignee)

Updated

15 years ago
Blocks: 111943
(Assignee)

Updated

15 years ago
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
(Assignee)

Updated

15 years ago
Summary: [FIXr]fire "load" event on stylesheet linking elements when the sheet load finishes → fire "load" event on stylesheet linking elements when the sheet load finishes
(Assignee)

Updated

14 years ago
Depends on: 115770
I'll take another stab at this now that bug 188030 is fixed.
Target Milestone: mozilla1.5alpha → mozilla1.9alpha
QA Contact: ian → style-system

Comment 10

10 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?
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.
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?
It would make sense to me if we did, for the sake of consistency. And it looks like <script> elements do.

Comment 14

10 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.
> 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.
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.
Firing async sounds ok to me FWIW.

Comment 18

10 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.
Blocks: 460323
The HTML5 spec now requires this (async, waits for all subresources, ignores CSS parse errors but fires 'error' for any network error).
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?
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

9 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.
> 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.
Blocks: 495347

Updated

7 years ago
Blocks: 563176
Duplicate of this bug: 563176
Summary: fire "load" event on stylesheet linking elements when the sheet load finishes → fire "load" event on stylesheet linking elements when the sheet load finishes or "error" if the load fails
Priority: P1 → P2
Target Milestone: mozilla1.9alpha1 → ---
Priority: P2 → P3

Comment 25

7 years ago
I've filed a related ticket over on Chrome's tracker: http://crbug.com/67522
I might do this at one point.
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.
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.
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?
> 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.
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.
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.  :(
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

6 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.
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.
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
For reference: http://www.phpied.com/when-is-a-stylesheet-really-loaded/
(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(
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.
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.
It would fire after the stylesheet style rules have been applied, but before we paint.
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.
LOL, sorry guys, just wanted to check and make sure. The event sequence as described seems perfect.
Priority: P3 → P1
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.
Created attachment 560086 [details] [diff] [review]
part 1.  Fix add-ons manager to ignore load events on stylesheet PIs.
Attachment #560086 - Flags: review?(dtownsend)
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.
Attachment #560087 - Flags: review?(peterv)
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.
Attachment #560088 - Flags: review?(peterv)
Created attachment 560089 [details] [diff] [review]
part 4.  Pass along the owning element to all sheet load datas, assuming we have one at all.
Attachment #560089 - Flags: review?(peterv)
Created attachment 560093 [details] [diff] [review]
part 5.  Fire load and error events on stylesheet linking elements.
Attachment #560093 - Flags: review?(peterv)
Whiteboard: [need review]
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 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?
> 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

6 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.
> 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.
(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.
James, I seriously suggest actually reading comment 50....

Comment 57

6 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.

Comment 58

6 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.
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?  ;)
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
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.
Attachment #560086 - Flags: review?(dtownsend) → review?(bmcbride)
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.
Attachment #560086 - Flags: review?(bmcbride) → review+
Thanks!
Attachment #560087 - Flags: review?(peterv) → review+
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()|?
Attachment #560089 - Flags: review?(peterv) → review+
> 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 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.
Attachment #560088 - Flags: review?(peterv) → review+
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?
Attachment #560093 - Flags: review?(peterv) → review+
> Not sure I understand this, did you mean "our load event"?

Yes, I do.  Fixed.

>seven?
>Sixth?

Er, yes.  This test expanded over time.  ;)
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
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla9
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Depends on: 690990
Depends on: 693725

Updated

6 years ago
Depends on: 696103
Documented:

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

Listed on Firefox 9 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Depends on: 714560

Updated

6 years ago
Depends on: 713745
Depends on: 713014
You need to log in before you can comment on or make changes to this bug.