style element and link element for CSS stylesheet should emit load/error event when sheet loads/fails to load

[Leon Clarke]

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

[Leon Clarke]

Created attachment 55851 [details]
Proposed patch

[Darin Adler]

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?

[Leon Clarke]

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.

[Leon Clarke]

Created attachment 55967 [details]
Re-done layout test in right place

[Alexey Proskuryakov]

> 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?

[Alexey Proskuryakov]

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.

[Leon Clarke]

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

[Alexey Proskuryakov]

Do you know if there is a reason why Firefox doesn't support this? Is there a bug at bugzilla.mozilla.org?

[Leon Clarke]

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)

[Leon Clarke]

Created attachment 56582 [details]
Get rid of the timer

[Alexey Proskuryakov]

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.

[Leon Clarke]

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.

[Leon Clarke]

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.

[Darin Adler]

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

[Johan "Spocke" Sörlin]

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.

[Simon Kaegi]

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.

[Eric Seidel (no email)]

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

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 127530 [details]
Patch and layout tests

Remove extraneous whitespace, and change the declaration order of HTMLStyleElement::m_firedLoad and HTMLStyleElement::m_loadedSheet.

[Adam Barth]

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.

[Adam Barth]

This looks reasonable to me.  @japhet: Would you like to take a look as well?

[Gavin Peters]

Any luck ditching the timer?

[Daniel Bates]

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

[Nate Chapin]

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 :(

[Daniel Bates]

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

[Daniel Bates]

(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).

[Daniel Bates]

Created attachment 128275 [details]
Patch and layout tests

Updated patch based on Adam Barth's and Nate Chapin's remarks.

[WebKit Review Bot]

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

[Daniel Bates]

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

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Collabora GTK+ EWS bot]

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

[Philippe Normand]

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

[Nate Chapin]

(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+.

[Adam Barth]

Comment on attachment 128275 [details]
Patch and layout tests

Looks good to me too.

[Daniel Bates]

Committed r108809: <http://trac.webkit.org/changeset/108809>