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>
Component: DOMAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, android-webkit-unforking, ap, commit-queue, darin, dbates, dglazkov, dominicc, eric, gavinp, gustavo.noronha, gustavo, japhet, jchaffraix, jonnew, koivisto, macpherson, mathias, menard, ml, niall.smart, paulirish, pnormand, priyajeet.hora, rik, simon_kaegi, spocke, tonyg, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: 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 Leon Clarke 2010-05-12 08:32:09 PDT
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 Leon Clarke 2010-05-12 08:36:16 PDT
Created attachment 55851 [details]
Proposed patch
Comment 2 Darin Adler 2010-05-12 11:22:29 PDT
Comment on attachment 55851 [details]
Proposed patch

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 Leon Clarke 2010-05-13 02:47:07 PDT
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 Leon Clarke 2010-05-13 06:18:15 PDT
Created attachment 55967 [details]
Re-done layout test in right place
Comment 5 Alexey Proskuryakov 2010-05-14 20:04:45 PDT
> 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 Alexey Proskuryakov 2010-05-14 20:15:29 PDT
Comment on attachment 55967 [details]
Re-done layout test in right place

+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 Leon Clarke 2010-05-17 03:47:23 PDT
(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 Alexey Proskuryakov 2010-05-17 08:32:40 PDT
Do you know if there is a reason why Firefox doesn't support this? Is there a bug at bugzilla.mozilla.org?
Comment 9 Leon Clarke 2010-05-20 04:03:00 PDT
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 Leon Clarke 2010-05-20 04:04:15 PDT
Created attachment 56582 [details]
Get rid of the timer
Comment 11 Alexey Proskuryakov 2010-05-20 09:36:26 PDT
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 Leon Clarke 2010-05-20 10:30:09 PDT
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 Leon Clarke 2010-05-24 06:59:00 PDT
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 Darin Adler 2010-06-12 19:05:22 PDT
Comment on attachment 56582 [details]
Get rid of the timer

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 Johan "Spocke" Sörlin 2011-06-20 14:19:25 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 16 Simon Kaegi 2011-10-03 10:40:33 PDT
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 Eric Seidel (no email) 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 Daniel Bates 2012-02-16 22:55:18 PST
Created attachment 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 Daniel Bates 2012-02-16 23:04:33 PST
Created attachment 127530 [details]
Patch and layout tests

Remove extraneous whitespace, and change the declaration order of HTMLStyleElement::m_firedLoad and HTMLStyleElement::m_loadedSheet.
Comment 20 Adam Barth 2012-02-21 12:38:58 PST
Comment on attachment 127530 [details]
Patch and layout tests

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 Adam Barth 2012-02-21 12:39:26 PST
This looks reasonable to me.  @japhet: Would you like to take a look as well?
Comment 22 Gavin Peters 2012-02-21 12:42:10 PST
Any luck ditching the timer?
Comment 23 Daniel Bates 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 Nate Chapin 2012-02-21 12:49:20 PST
Comment on attachment 127530 [details]
Patch and layout tests

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 Daniel Bates 2012-02-21 13:30:42 PST
(In reply to comment #24)
> (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).

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 Daniel Bates 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 Daniel Bates 2012-02-22 13:24:17 PST
Created attachment 128275 [details]
Patch and layout tests

Updated patch based on Adam Barth's and Nate Chapin's remarks.
Comment 28 WebKit Review Bot 2012-02-22 13:44:53 PST
Comment on attachment 128275 [details]
Patch and layout tests

Attachment 128275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11560603
Comment 29 Daniel Bates 2012-02-22 13:47:16 PST
(In reply to comment #28)
> (From update of attachment 128275 [details])
> Attachment 128275 [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 Early Warning System Bot 2012-02-22 13:50:33 PST
Comment on attachment 128275 [details]
Patch and layout tests

Attachment 128275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11559603
Comment 31 Gyuyoung Kim 2012-02-22 13:58:45 PST
Comment on attachment 128275 [details]
Patch and layout tests

Attachment 128275 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11446027
Comment 32 Collabora GTK+ EWS bot 2012-02-22 14:30:58 PST
Comment on attachment 128275 [details]
Patch and layout tests

Attachment 128275 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11560617
Comment 33 Philippe Normand 2012-02-22 14:50:24 PST
Comment on attachment 128275 [details]
Patch and layout tests

Attachment 128275 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11556631
Comment 34 Nate Chapin 2012-02-23 13:08:34 PST
(In reply to comment #29)
> (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.

LGTM, though I'd prefer to wait for a second opinion before r+.
Comment 35 Adam Barth 2012-02-23 13:45:43 PST
Comment on attachment 128275 [details]
Patch and layout tests

Looks good to me too.
Comment 36 Daniel Bates 2012-02-24 08:52:26 PST
Committed r108809: <http://trac.webkit.org/changeset/108809>