Bug 38995

Summary: style element and link element for CSS stylesheet should emit load/error event when sheet loads/fails to load
Product: WebKit Reporter: Leon Clarke <leonclarke@google.com>
Component: HTML DOMAssignee: Daniel Bates <dbates@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, alexis@webkit.org, android-webkit-unforking@google.com, ap@webkit.org, commit-queue@webkit.org, darin@apple.com, dbates@webkit.org, dglazkov@chromium.org, dominicc@chromium.org, eric@webkit.org, gavinp@chromium.org, gns@gnome.org, gustavo.noronha@collabora.co.uk, japhet@chromium.org, jchaffraix@webkit.org, jonnew@gmail.com, koivisto@iki.fi, macpherson@chromium.org, mathias@qiwi.be, ml@graougraou.com, niall.smart@gmail.com, paulirish@chromium.org, pnormand@igalia.com, priyajeet.hora@gmail.com, rik@webkit.org, simon_kaegi@ca.ibm.com, spocke@moxiecode.com, tonyg@chromium.org, webkit.review.bot@gmail.com, xan.lopez@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Bug Depends on: 78840    
Bug Blocks: 76198    
Attachments:
Description Flags
Proposed patch
none
Re-done layout test in right place
ap: review-, ap: commit‑queue-
Get rid of the timer
darin: review-, darin: commit‑queue-
Patch and layout tests
none
Patch and layout tests
none
Patch and layout tests abarth: review+, webkit.review.bot: commit‑queue-

Description From 2010-05-12 08:32:09 PST
According to the HTML5 spec, link elements should support onload events. I'm not aware of anyone in the real world who wants these, but I wanted them for rel=prefetch links, just so that prefetch links had some side-effect that could be tested in a layout test. I may as well get them working for css links at the same time. Since this is arguably separable from rel=prefetch, I'm putting it in a separate bug.

Here's the prefetch bug https://bugs.webkit.org/show_bug.cgi?id=3652
------- Comment #1 From 2010-05-12 08:36:16 PST -------
Created an attachment (id=55851) [details]
Proposed patch
------- Comment #2 From 2010-05-12 11:22:29 PST -------
(From update of attachment 55851 [details])
The DOM test suite is something we imported from the W3C.

Is this new test from that source? If not, then the test should be written in the usual WebKit test idiom and put in the directory fast/dom/HTMLLinkElement instead of the directory dom/html/level2/html.

Does this feature work in other browsers? Do they pass this test? Is it from a particular specification?
------- Comment #3 From 2010-05-13 02:47:07 PST -------
OK. I'll move the layout test.

This is taken from the HTML5 draft. I've tested on Android and Safari so far; the feature works and the test passes. Since it's nowhere near anything platform dependent and it works on those 2 platforms, I see no reason to think it won't work on all other platforms.
------- Comment #4 From 2010-05-13 06:18:15 PST -------
Created an attachment (id=55967) [details]
Re-done layout test in right place
------- Comment #5 From 2010-05-14 20:04:45 PST -------
> Does this feature work in other browsers? Do they pass this test?

I think the question was - does this work in IE, Firefox or Opera?
------- Comment #6 From 2010-05-14 20:15:29 PST -------
(From update of attachment 55967 [details])
+void HTMLLinkElement::notifyFinished(CachedResource*)
+{
+    if (!m_timer.isActive())
+        m_timer.startOneShot(0);
+}

I doubt that a timer is needed here. HTML5 spec probably says to queue a task to dispatch an event, but that usually translates as "dispatch the event immediately" in browser code.

+    if (!m_loading) {
         c->setCSSStyleSheet(m_url, m_response.url(), m_decoder->encoding().name(), this);
+        c->notifyFinished(this);
+    }

I'm not quite sure about the design here, but it seems that setCSSStyleSheet should go into notifyFinished() now. Please compare to what other CachedResource subclasses do.

r-, because I'm fairly sure that adding a timer is wrong.
------- Comment #7 From 2010-05-17 03:47:23 PST -------
(In reply to comment #5)
> > Does this feature work in other browsers? Do they pass this test?
> 
> I think the question was - does this work in IE, Firefox or Opera?

Sorry. I misunderstood.

It works in Opera but not in Firefox. I don't have access to a Windows machine right now to try IE.
------- Comment #8 From 2010-05-17 08:32:40 PST -------
Do you know if there is a reason why Firefox doesn't support this? Is there a bug at bugzilla.mozilla.org?
------- Comment #9 From 2010-05-20 04:03:00 PST -------
I had a look and couldn't see any bugs in firefox. I rather suspect that not many people have found a use for this, so firefox haven't bothered. The reason I'm implementing it on WebKit is a side-effect of something else, and I haven't seen any other bugs asking for this in WebKit.

In response to your review comments, not having a timer is a good idea; I was copying some inappropriate code that resulted in the timer. 

I kept changing my mind about whether it would be better to put the setCSSStlyeSheet call into notifyFinished. Obviously, it would to be done for the other subclasses of CachedResource that implement setCSSStyleSheet, in which case it would probably be best to introduce an intermediate class whose notifyFinished calls setCSSStyleSheet (since otherwise I'm adding identical functions to 2 separate subclasses of the same class). Then presumably we should consider whether to do the same thing for setXSLStyleSheet and other functions in CachedResourceClient. At the end of all that, I would have added a couple of extra classes and refactored a load of code I wasn't intending to touch, and I'm not certain it would have made anything any clearer. Therefore I'm going to argue that it's best if this change keeps the calls to setCSSStyleSheet separate, and someone could consider the refactoring at a later date. It might also help if that someone had a deeper understanding of how all these classes are intended to fit together than I do currently.

So I'll upload another patch that gets rid of the timer. (and slightly tidies up the CachedCSSStyleSheet changes)
------- Comment #10 From 2010-05-20 04:04:15 PST -------
Created an attachment (id=56582) [details]
Get rid of the timer
------- Comment #11 From 2010-05-20 09:36:26 PST -------
See <https://bugzilla.mozilla.org/show_bug.cgi?id=185236>. Skimming over the comments, it seems that some of them may be interesting to us, as well.
------- Comment #12 From 2010-05-20 10:30:09 PST -------
Yes, that is interesting. The comments about the need to wait until included subresources are loaded certainly seem to be valid, and thus my patch is not good enough.

I'm also not sure what the correct thing to do is on the question of whether the event should come before or after the main document onload.
------- Comment #13 From 2010-05-24 06:59:00 PST -------
Given the issue with subresources, it's no longer the case that this issue can essentially be resolved with code that I wanted to submit anyway while fixing bug 3652. As a result, I've removed the link between this and 3652. Having started this, I'll try and finish it but I might not get round to it immediately.
------- Comment #14 From 2010-06-12 19:05:22 PST -------
(From update of attachment 56582 [details])
Given the current state of this code, there is no need to add a new notifyFinished function. The code to dispatch the load event can go in HTMLLinkElement::setCSSStyleSheet.

review- because of the subresource issue mentioned in the bug discussion
------- Comment #15 From 2011-06-20 14:19:25 PST -------
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 #16 From 2011-10-03 10:40:33 PST -------
This is now fixed in Firefox <https://bugzilla.mozilla.org/show_bug.cgi?id=185236> 

Would be great to get a fix for WebKit too.
------- Comment #17 From 2012-01-11 17:41:19 PST -------
We might be able to piggie-back on the newer LinkLoader class to do this for us.  Nate wrote it for pre-fetch.

This bug it tickled by the IE Test Center samples:
http://samples.msdn.microsoft.com/ietestcenter/domevents/domevents_harness.htm?url=UIEvent.load.stylesheet.html
------- Comment #18 From 2012-02-16 22:55:18 PST -------
Created an attachment (id=127527) [details]
Patch and layout tests

I ran into the same issue as Gavin Peters, <https://bugs.webkit.org/show_bug.cgi?id=51941#c45>, with regards to the need for a one-shot timer to reliably dispatch load events. As Gavin remarked, such an approach is used in ImageLoader. I spent some time trying to debug this issue, but I was unable to diagnose the cause. If I have additional time tomorrow, I'll look to further debug this issue.

For completeness, error events seemed to be reliably dispatched regardless of whether they were dispatched from a one-shot timer. I chose to dispatch them using a one-shot timer (managed by EventSender) to be consistent with the approach used in LinkLoader.cpp. Subsequent patches will look to better integrate class LinkLoader with class LoadEventSender and a define a default constructor for class EventSender so that we don't have to explicitly specify an event type.

I'm open to suggestions on this patch.
------- Comment #19 From 2012-02-16 23:04:33 PST -------
Created an attachment (id=127530) [details]
Patch and layout tests

Remove extraneous whitespace, and change the declaration order of HTMLStyleElement::m_firedLoad and HTMLStyleElement::m_loadedSheet.
------- Comment #20 From 2012-02-21 12:38:58 PST -------
(From update of attachment 127530 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127530&action=review

> Source/WebCore/html/HTMLLinkElement.cpp:60
> +typedef EventSender<HTMLLinkElement> LoadEventSender;
> +static LoadEventSender& loadEventSender()
> +{
> +    DEFINE_STATIC_LOCAL(LoadEventSender, sharedLoadEventSender, (eventNames().loadEvent));
> +    return sharedLoadEventSender;
> +}

Woah, shared across all of WebKit.  Crazy.

> Source/WebCore/html/HTMLLinkElement.h:66
> +    void dispatchPendingEvent(EventSender<HTMLLinkElement>*);

Should we create a typedef for EventSender<HTMLLinkElement> like we did in the image case?

> Source/WebCore/html/HTMLStyleElement.cpp:40
> +typedef EventSender<HTMLStyleElement> LoadEventSender;

Ah, I see we do here.  It might be worth moving this to the header.
------- Comment #21 From 2012-02-21 12:39:26 PST -------
This looks reasonable to me.  @japhet: Would you like to take a look as well?
------- Comment #22 From 2012-02-21 12:42:10 PST -------
Any luck ditching the timer?
------- Comment #23 From 2012-02-21 12:43:58 PST -------
(In reply to comment #22)
> Any luck ditching the timer?

I haven't had a chance to look into the timer issue. I'll take a look later today.
------- Comment #24 From 2012-02-21 12:49:20 PST -------
(From update of attachment 127530 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127530&action=review

> Source/WebCore/css/CSSStyleSheet.cpp:257
> +    m_didLoadErrorOccur |= sheet->errorOccurred() || sheet->response().httpStatusCode() > 400;

I think a > 400 response should be a strict subset of errorOccurred() for style sheets (see shouldIgnoreHTTPStatusCodeErrors() in CachedResource and subclasses).

> Source/WebCore/dom/Node.h:301
>      virtual bool sheetLoaded() { return true; }
> +    virtual void notifyLoadedSheetAndAllCriticalSubresources(bool /* error loading subresource */) { }
>      virtual void startLoadingDynamicSheet() { ASSERT_NOT_REACHED(); }

It's a shame we have to declare these functions all the way down in Node :(
------- Comment #25 From 2012-02-21 13:30:42 PST -------
(In reply to comment #24)
> (From update of attachment 127530 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127530&action=review
> 
> > Source/WebCore/css/CSSStyleSheet.cpp:257
> > +    m_didLoadErrorOccur |= sheet->errorOccurred() || sheet->response().httpStatusCode() > 400;
> 
> I think a > 400 response should be a strict subset of errorOccurred() for style sheets (see shouldIgnoreHTTPStatusCodeErrors() in CachedResource and subclasses).

As discussed on IRC with Nate, it's sufficient to check CachedResource::errorOccurred() for style sheets by <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubresourceLoader.cpp?rev=105226#L232>, <http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedResource.h?rev=107672#L103>, and by definition of CachedCSSStyleSheet (which extends CachedResource and doesn't override shouldIgnoreHTTPStatusCodeErrors()).

Therefore I'll change this line to read:

m_didLoadErrorOccur |= sheet->errorOccurred();

before landing this patch.
------- Comment #26 From 2012-02-22 11:30:47 PST -------
(In reply to comment #22)
> Any luck ditching the timer?

Calling dispatchEvent() for a Load/Error event without the queue/timer approach does fire the Load/Error event. The issue is that we may still be constructing the DOM tree and DOM elements referenced (say, using document.getElementById()) within a <link>/<style>'s onload/onerror handler may not be in the document at the time the event is fired. By using the queue/timer approach we can ensure (see remark (*)) that Load/Error events for non-programmatically inserted link/style elements are dispatched once the document has been built.

Additional remarks:

In the patch, we queue load/error events and dispatch these queued events in Document::implicitClose(). (See LoadEventSender in HTML{Link, Style}Element.cpp and the patch for bug #78840 for more details on the queue.) Notice the timer is in EventSender and is used to empty the queue. Disregarding programmatically inserted link/style elements (I look at this case in the next paragraph), the emptying of the queue in Document::implicitClose() ensures that pending Load/Error events are dispatched after the document has been built and hence a <link>/<style>'s onload handler may reference an arbitrary DOM element.

When a link/style element is programmatically inserted into a document, the timer in EventSender ensures that any queued load/error is eventually dispatched.

The queue/timer approach seems reasonable. Notice this approach is used by ImageLoader. We may want to look to implement an approach similar to the one described in (*) for Load/BeforeLoad/Error events. I suggest we do this in a follow up patch.

(*) It's actually the use of a queue that ensures this property. That is, the timer isn't necessary. For programmatically inserted link/style elements we could dispatch the Load/Error event immediately. Otherwise, we queue Load/Error events until Document::implicitClose() is called to empty the queue. A side effect of this queue approach is that we ensure that Load/Error events for link/style elements are dispatched before the window Load event is dispatched as required by sections "The link element" and "The style element" in the HTML5 spec. (draft 05/25/2011).
------- Comment #27 From 2012-02-22 13:24:17 PST -------
Created an attachment (id=128275) [details]
Patch and layout tests

Updated patch based on Adam Barth's and Nate Chapin's remarks.
------- Comment #28 From 2012-02-22 13:44:53 PST -------
(From update of attachment 128275 [details])
Attachment 128275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11560603
------- Comment #29 From 2012-02-22 13:47:16 PST -------
(In reply to comment #28)
> (From update of attachment 128275 [details] [details])
> Attachment 128275 [details] [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11560603

The patch doesn't build because it depends on the patch for #78840, which I haven't landed yet.
------- Comment #30 From 2012-02-22 13:50:33 PST -------
(From update of attachment 128275 [details])
Attachment 128275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11559603
------- Comment #31 From 2012-02-22 13:58:45 PST -------
(From update of attachment 128275 [details])
Attachment 128275 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11446027
------- Comment #32 From 2012-02-22 14:30:58 PST -------
(From update of attachment 128275 [details])
Attachment 128275 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11560617
------- Comment #33 From 2012-02-22 14:50:24 PST -------
(From update of attachment 128275 [details])
Attachment 128275 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11556631
------- Comment #34 From 2012-02-23 13:08:34 PST -------
(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 128275 [details] [details] [details])
> > Attachment 128275 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11560603
> 
> The patch doesn't build because it depends on the patch for #78840, which I haven't landed yet.

LGTM, though I'd prefer to wait for a second opinion before r+.
------- Comment #35 From 2012-02-23 13:45:43 PST -------
(From update of attachment 128275 [details])
Looks good to me too.
------- Comment #36 From 2012-02-24 08:52:26 PST -------
Committed r108809: <http://trac.webkit.org/changeset/108809>