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.

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

Add a few tests for whether CSS subresources are critical #5525

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gsnedders
Copy link
Member

@gsnedders gsnedders commented Apr 10, 2017

tests for w3c/csswg-drafts#1088

these should be moved somewhere better once we know what directory to put them in

@ghost

This comment was marked as outdated.

@ghost

This comment was marked as outdated.

@gsnedders

This comment was marked as outdated.

@gsnedders gsnedders closed this Apr 10, 2017
@gsnedders gsnedders reopened this Apr 10, 2017
@gsnedders

This comment was marked as outdated.

@ghost

This comment was marked as outdated.

@wpt-pr-bot

This comment was marked as outdated.

@gsnedders

This comment was marked as outdated.

@gsnedders
Copy link
Member Author

While the CSS WG has still not resolved the above issue, these tests do at this point show identical behaviour across all three major browser engines, hence on the whole I'd be in favour of landing these as tentative tests if someone wants to review them.

@domfarolino
Copy link
Member

For some reason this thread was buried in my inbox and it doesn't look there's been much activity in a while. Are you still seeking review @gsnedders? Has there been any more consensus or spec updates here?

@gsnedders
Copy link
Member Author

Has there been any more consensus or spec updates here?

Not that I'm aware of; that said, I'm still in favour of review (and landing) the tests which pass in all three major browser engines, as we may as well have tentative tests that assert the current intersection behaviour (and ensure we don't move away from interoperability there).

Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM % a couple of comments. Also, maybe we can change instances of var to let or const appropriately?

<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
</script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this <script></script> needed?

font: 25px/1 Revalia;
}
</style>
<div id=foo>XXX</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 2/4 font-using tests in this PR, the id=foo element comes before the <style> that includes the font, and in the other 2/4 it is the opposite. Can we make this consistent? (Not that it really matters much...)

@domfarolino
Copy link
Member

Another variation for these tests that could be useful to include in this PR is: moving all of the <style> block contents to a <link rel=stylesheet> and seeing if the link element's load event is indeed blocked when the Document's load event is blocked.

In w3c/csswg-drafts#1088 (comment), Hiroshige points out that apparently Chrome blocks the Document's load event in some cases where the <link> element's load event is not blocked, which is pretty interesting. It would be good to know what browsers do here more generally.

@gsnedders
Copy link
Member Author

Another variation for these tests that could be useful to include in this PR is: moving all of the <style> block contents to a <link rel=stylesheet> and seeing if the link element's load event is indeed blocked when the Document's load event is blocked.

I'll fix the nits you mentioned above, but I probably won't expand the coverage of this (I don't really immediately have the time to actually work on this right now).

@domfarolino
Copy link
Member

Let me know whenever you do and I'm happy to re-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants