[go: up one dir, main page]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Something needs to define the actual behavior of load/error events on <link> #3532

Open
bzbarsky opened this issue Mar 5, 2018 · 11 comments
Open

Something needs to define the actual behavior of load/error events on <link> #3532

bzbarsky opened this issue Mar 5, 2018 · 11 comments
Labels
interop Implementations are not interoperable with each other topic: link topic: style

Comments

@bzbarsky
Copy link
Contributor
bzbarsky commented Mar 5, 2018

Consider this testcase:

<!DOCTYPE html>
<link rel="stylesheet" href="http://example.com"
      onload="alert('success')" onerror="alert('fail')">

In this case, the sheet is never applied, since this is a cross-site load and the returned type is not text/css. Browser behavior is as follows (the "loaded over" refers to the original page; the sheet is always being loaded over http):

  • Chrome, loaded over http: alerts success
  • Chrome, loaded over https: alerts fail
  • Safari, loaded over http: alerts success
  • Safari, loaded over https: alerts success
  • Edge, loaded over http: alerts success
  • Edge, loaded over https: no alert at all
  • Firefox, loaded over http: alerts fail
  • Firefox, loaded over https: alerts fail

Now consider a similar testcase:

<!DOCTYPE html>
<link rel="stylesheet" href="data:text/css,@import url('http://example.com')"
      onload="alert('success')" onerror="alert('fail')">

Now the behavior is:

  • Chrome, loaded over http: alerts success
  • Chrome, loaded over https: alerts fail
  • Safari, loaded over http: alerts success
  • Safari, loaded over https: alerts success
  • Edge, loaded over http: alerts success
  • Edge, loaded over https: alerts success
  • Firefox, loaded over http: alerts fail
  • Firefox, loaded over https: alerts success

This is clearly broken... ;)
@bzbarsky
Copy link
Contributor Author
bzbarsky commented Mar 5, 2018

@zcorpan you might be interested in this too.

Note that sheets can get applied even if they report "error" in terms of the event, as far as I can tell.

@bzbarsky
Copy link
Contributor Author
bzbarsky commented Mar 5, 2018

Firefox, loaded over https: alerts success

I will probably fix that in https://bugzilla.mozilla.org/show_bug.cgi?id=1443344

@domenic
Copy link
Member
domenic commented Mar 6, 2018

https://html.spec.whatwg.org/multipage/semantics.html#obtaining-a-resource-from-a-link-element tries to do this for most links, and I guess https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet is supposed to reuse that, although we have #968 implying that's not quite right.

I guess the rather-vague part is

Once the attempts to obtain the resource and its critical subresources are complete, the user agent must, if the loads were successful, queue a task to fire an event named load at the link element, or, if the resource or one of its critical subresources failed to completely load for any reason (e.g. DNS error, HTTP 404 response, a connection being prematurely closed, unsupported Content-Type), queue a task to fire an event named error at the link element. Non-network errors in processing the resource or its subresources (e.g. CSS parse errors, PNG decoding errors) are not failures for the purposes of this paragraph.

which is coupled with some fun stuff about the Content-Type in https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet

This should ideally be defined in terms of network errors and non-ok statuses and more, and I guess we should find some more rigorous way to define "critical subresources".

But by my reading of the current spec, assuming @imported stylesheets are critical subresources, we have:

  • First example, loaded over http: alerts fail

  • First example, loaded over https: alerts fail

  • Second example, loaded over http: alerts fail

  • Second example, loaded over https: alerts fail

So here is my proposed short-term fix:

  • Change the rather-vague part above to talk about network errors, non-ok statuses, and "resource-specific fetch errors"
  • Change the link rel=stylesheet section to say that a mismatched Content-Type, in the non-quirk case, is a "resource specific fetch error"
  • Define the critical subresources for CSS as @imported files. (Maybe this should eventually move to some CSS spec.)
  • You write tests as part of the Firefox bug you're fixing

How does this sound?

@bzbarsky
Copy link
Contributor Author
bzbarsky commented Mar 6, 2018

That sounds lovely, and I agree that all the alerts should be "fail".

I will probably write tests separately from those bugs I'm fixing, because I need to get those fixed ASAP and it's simpler to modify exisiting non-wpt tests we have for them. But I will make sure the tests get written.

@bzbarsky
Copy link
Contributor Author
bzbarsky commented Mar 6, 2018

So writing tests now... Chrome will fire "error" for a sheet that imports a failing load, but fire "load" for a sheet that imports a sheet that imports a failing load....

@bzbarsky
Copy link
Contributor Author
bzbarsky commented Mar 6, 2018

@domenic https://bug1443652.bmoattachments.org/attachment.cgi?id=8956628 has some tests if you wanted to take a look and see whether they cover the cases you can think of.

I didn't write anything for <style> yet, by the way... Still need to do that, I guess.

@annevk
Copy link
Member
annevk commented Mar 7, 2018

@TakayoshiKochi and @ericwilligers probably want to study/help with this as it will affect https://github.com/WICG/construct-stylesheets.

@annevk annevk added interop Implementations are not interoperable with each other topic: link labels Mar 7, 2018
domenic added a commit that referenced this issue Mar 7, 2018
@domenic
Improve clarity in link and style sheet resource fetching
a4ccf1c 
* Defines the critical subresources for CSS as @imported style sheets, including
  indirectly imported ones. This was previously defined nowhere; probably it
  should live in CSS, but for now it lives here.
* Makes it clearer that the steps to obtain the resource are the default steps,
  which some link types override.
* Adds explicit steps for fetching critical subresources, and ties those to the
  load/error events.
* Uses fetch primitives like network error and ok status instead of vague
  language.
* Makes it clearer how incorrect Content-Types for CSS generate error events, by
  defining the new "resource-specific fetch error" concept.
* Makes it clearer that resources apply if and only if their main fetch
  succeeds. (Critical subresource fetches generate error events, but do not
  impact the "obtained successfully" or "apply" concepts.)

Fixes #3532.
@domenic domenic mentioned this issue Mar 7, 2018
Closed
@domenic
Copy link
Member
domenic commented Mar 7, 2018

@bzbarsky tests look good; I'm glad you got the nested import case. One thing that you might consider orthogonal, but I found a bit surprising and made sure to note when writing #3544, was that style sheets do apply even if they import something that fails. See http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5812.

@domenic domenic mentioned this issue Apr 4, 2018
Open
domenic added a commit that referenced this issue Apr 4, 2018
@domenic
Improve clarity in link and style sheet resource fetching
a913b6e 
* Defines the critical subresources for CSS as @imported style sheets, including
  indirectly imported ones. This was previously defined nowhere; probably it
  should live in CSS, but for now it lives here.
* Makes it clearer that the steps to obtain the resource are the default steps,
  which some link types override.
* Adds explicit steps for fetching critical subresources, and ties those to the
  load/error events.
* Uses fetch primitives like network error and ok status instead of vague
  language.
* Makes it clearer how incorrect Content-Types for CSS generate error events, by
  defining the new "resource-specific fetch error" concept.
* Makes it clearer that resources apply if and only if their main fetch
  succeeds. (Critical subresource fetches generate error events, but do not
  impact the "obtained successfully" or "apply" concepts.)

Fixes #3532.
@domfarolino domfarolino mentioned this issue Sep 13, 2018
Merged
@domenic
Copy link
Member
domenic commented Jun 1, 2020

@domfarolino, I think the OP in this issue is now covered by some nice rigorous spec text. Do you agree? However, I'm not as sure about web platform tests...

@domfarolino
Copy link
Member

Yes I agree. In terms of tests, my work added tests about the ordering between style sheet load/error event & scripting and removing / loading style sheets. I think there are still two kinds of tests needed:

Also regarding the first comment's test case, I think we can write tests for the HTTP case, since Fetch returns a network error for requests blocked by mixed content. For the HTTPs case, I guess that'd be covered by the tests in the second bullet above?

@domenic
Copy link
Member
domenic commented Jul 6, 2020

I agree with your assessment of the missing tests for previous work. Any help pushing them over the finish line would be much appreciated, e.g. reviewing web-platform-tests/wpt#10350.

For the first comment test cases, can't we just basically transcribe those tests into web platform test files?

@andreubotella andreubotella mentioned this issue Nov 4, 2020
Closed
@domenic domenic mentioned this issue Jan 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interop Implementations are not interoperable with each other topic: link topic: style
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve clarity in link and style sheet resource fetching whatwg/html
4 participants
@domenic @bzbarsky @annevk @domfarolino