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

Need to define what fetch groups various loads from the sheet go in #15

Closed
bzbarsky opened this issue Feb 6, 2018 · 21 comments · Fixed by #69
Closed

Need to define what fetch groups various loads from the sheet go in #15

bzbarsky opened this issue Feb 6, 2018 · 21 comments · Fixed by #69

Comments

@bzbarsky
Copy link

bzbarsky commented Feb 6, 2018

This applies to @import, backgrounds, fonts, etc.

This is a bit more complicated than the <link> case, because in that case there is only one plausible fetch group: the one for the document. With constructible sheets, there are at least two fetch groups involved: the one for the window the constructor came from (esp. for sheets not attached to anything) and the one for wherever the sheet is actually being used.

I guess a sheet can in fact be used in multiple places, so there could be any number of fetch groups involved....

As a concrete question, if I create a sheet from documen A's window, and am using it in documents B and C, and B matches a rule with one background image while C matches a rule with another, which load events, if any, are blocked by the resulting image loads.

@TakayoshiKochi
Copy link
Member

What if we constrain the usage of the constructed stylesheet which is created in document A, it shall only be used within document A? (related issue: #10 and #23)

I don't think this restriction will mess up any (useful) use cases, and just in case if you really need to use the same stylesheet to another document, we can come up with document.adoptStyleSheet().

@TakayoshiKochi
Copy link
Member

As #23 concludes a stylesheet can be only used for one document, closing this.

If the conclusion was reverted, I'll reopen this.

@bzbarsky
Copy link
Author

@TakayoshiKochi Even if you can only use them in one document, there are open questions in this issue because that document might not match the window the constructor came from. Please actually address those questions.

Unfortunately it looks like I cannot reopen this issue...

@TakayoshiKochi
Copy link
Member

@bzbarsky could you elaborate how can we create the situation to make your script run in a document which is different from the current global? (sorry for my ignorance...)

@bzbarsky
Copy link
Author

Maybe I misunderstood what the proposal was? If I do this:

<iframe></iframe>
<script> var sheet = new frames[0].CSSStyleSheet(text);</script>

Can I use sheet in document? Or so I have to use it in frames[0].document? In the latter case, there is no ambiguity, apart from the fact that the document in that Window can change...

@rakina
Copy link
Member

rakina commented Jul 6, 2018

@bzbarsky it should only be usable in frames[0].document I think, and if the document changes it's not usable on the changed document.

@bzbarsky
Copy link
Author

bzbarsky commented Jul 6, 2018

In that case, if the sheet is actually tied to a Document not a Window, why is it not created via a factory method on that Document?

@TakayoshiKochi
Copy link
Member

@bzbarsky I share the same question... in terms of the CSSOM spec CSSStyleSheet doesn't have the notion of ownerDocument, but has .ownerNode, and even when its inner slot location is null (e.g. for <style>) url() in the sheet is actually resolved against the document's base URL.

So for the constructor, it has an indirect association with its global (window)'s document, but it would be clearer if Document has another (other than one returns a Promise) factory method to return a CSSStyleSheet object. (this may sound contradict my own #2 (comment), but...)

@tabatkins do we still want a CSSStyleSheet() constructor independent of document?

aarongable pushed a commit to chromium/chromium that referenced this issue Aug 7, 2018
Currently, empty CSSStyleSheets can only be constructed using a constructor method.
This CL adds Document.createEmptyCSSStyleSheet so that it can be tied to document in the future.
Tying it to document restricts the use of a CSSStyleSheet in different documents, which means CSSStyleSheets can only be used in the documents where they are constructed. This will reduce security risk.

Note:
The constructed CSSStyleSheet is not currently tied to the Document yet
Constructor method will be deleted in another CL
createEmptyCSSStyleSheet(CSSStyleSheetInit) produces an empty CSSStyleSheet, while createCSSStyleSheet(text, CSSStyleSheetInit) creates a CSSStyleSheet with text

Link to related comments in discussion:
WICG/construct-stylesheets#23 (comment)
WICG/construct-stylesheets#15 (comment)

Bug: 807560

Change-Id: I94ea6f795deaf0dee67fba5c2705c8749ac72da8
Reviewed-on: https://chromium-review.googlesource.com/1160422
Commit-Queue: Momoko Sumida <[email protected]>
Reviewed-by: Hayato Ito <[email protected]>
Reviewed-by: Rakina Zata Amni <[email protected]>
Cr-Commit-Position: refs/heads/master@{#581169}
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 9, 2018
Currently, empty CSSStyleSheets can be constructed either with a constructor or with Document.createEmptyCSSStyleSheet.
This CL deletes the constructor so that they can only be produced by Document.createEmptyCSSStyleSheet.

Document.createEmptyCSSStyleSheet is considered to be more desirable, as CSSStyleSheets produced by Document.createEmptyCSSStyleSheet can be tied to documents in the future. This means that their use can be limited in the documents where they were produced, resulting in higher security.

Note:
The constructed CSSStyleSheet is not currently tied to the Document yet

Link to related comments in discussion:
WICG/construct-stylesheets#23 (comment)
WICG/construct-stylesheets#15 (comment)

Bug: 807560
Change-Id: I767e15e83e1f31eb278bc81233c8b579d0f511c7
Reviewed-on: https://chromium-review.googlesource.com/1164876
Reviewed-by: Rakina Zata Amni <[email protected]>
Reviewed-by: Hayato Ito <[email protected]>
Commit-Queue: Momoko Sumida <[email protected]>
Cr-Commit-Position: refs/heads/master@{#581836}
aarongable pushed a commit to chromium/chromium that referenced this issue Aug 10, 2018
This reverts commit af00b29.

Reason for revert: Suspect for https://crbug.com/873015

I have low confidence that this is the culprit but it's my best guess.

Original change's description:
> Delete constructor for creating empty CSSStyleSheets
> 
> Currently, empty CSSStyleSheets can be constructed either with a constructor or with Document.createEmptyCSSStyleSheet.
> This CL deletes the constructor so that they can only be produced by Document.createEmptyCSSStyleSheet.
> 
> Document.createEmptyCSSStyleSheet is considered to be more desirable, as CSSStyleSheets produced by Document.createEmptyCSSStyleSheet can be tied to documents in the future. This means that their use can be limited in the documents where they were produced, resulting in higher security.
> 
> Note:
> The constructed CSSStyleSheet is not currently tied to the Document yet
> 
> Link to related comments in discussion:
> WICG/construct-stylesheets#23 (comment)
> WICG/construct-stylesheets#15 (comment)
> 
> Bug: 807560
> Change-Id: I767e15e83e1f31eb278bc81233c8b579d0f511c7
> Reviewed-on: https://chromium-review.googlesource.com/1164876
> Reviewed-by: Rakina Zata Amni <[email protected]>
> Reviewed-by: Hayato Ito <[email protected]>
> Commit-Queue: Momoko Sumida <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#581836}

[email protected],[email protected],[email protected]

Change-Id: Iea70ff4dcc3a5ecf3a806b417572fd8f88e2e58b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 807560, 873015
Reviewed-on: https://chromium-review.googlesource.com/1170462
Reviewed-by: Matt Giuca <[email protected]>
Commit-Queue: Matt Giuca <[email protected]>
Cr-Commit-Position: refs/heads/master@{#582054}
@annevk
Copy link

annevk commented Sep 20, 2018

How does ownerNode work when the style sheet is reused across shadow roots?

@rakina
Copy link
Member

rakina commented Sep 20, 2018

How does ownerNode work when the style sheet is reused across shadow roots?

Constructed stylesheets' ownerNode is null. Can you elaborate on what might be the problem you mentioned on the other thread?

@annevk
Copy link

annevk commented Sep 20, 2018

Sorry, I didn't mean to imply there was a problem, just that you have to do something. Always returning null might be viable. Perhaps ownerNode was just a badly designed API that should not have been there.

@tabatkins
Copy link
Contributor

ownerNode isn't necessarily bad, but it's an element-ish API, making the assumption that a stylesheet lives in one and only one place in a document. This wasn't an unreasonable assumption before, as duplicating a stylesheet was worthless.

So yeah, just having it be always null for constructed sheets is the way to go.

@yutakahirano
Copy link

yutakahirano commented Oct 4, 2018

@hiroshige-g Hi Hiroshige, I think the situation is similar to the case where a stylesheet is shared by multiple documents via MemoryCache. Do you know what we are doing in Blink right now? Do you have any suggestion what should be specced here?

@hiroshige-g
Copy link

hiroshige-g commented Oct 4, 2018

Yeah, the same question applies to a stylesheet loaded from network and shared via MemoryCache across multiple Documents.
Currently, Blink loads background images etc. in stylesheets using ResourceLoader that belongs to the Document that needed the background image for the first time.

For example:
Q. if a stylesheet is loaded from network from document A, shared with documents B and C via MemoryCache, and B matches a rule with one background image R while C matches a rule with another background image S, which load events, if any, are blocked by the resulting image loads?
A. R blocks B's load event, S blocks C's load event.

@hiroshige-g
Copy link

Clarification: the "load event" discussed here is Document/Window's load events, right?

In the first place, I don't think the delay-the-Document-load-event behavior for background images are specced or interoperable.

@hiroshige-g
Copy link

Anyway, I agree with @TakayoshiKochi's comment:
#15 (comment)

In general, I think enforcing same-Document relationship is good for code health and security.

@bzbarsky
Copy link
Author

Per discussion today, static @import can use the fetch group of the document the sheet is being created for, we can disallow dynamic @import (see #56), and for other loads we may want to do separate fetches per document (esp. because for fonts the Origin headers might need to be different if document.domain is used). Gecko apparently already does this last for user and UA stylesheets, which are shared across documents.

@tabatkins
Copy link
Contributor

Oooh, good final point.

So that suggests we should be fine to share a stylesheet across documents? That makes some things much simpler.

(Is it already specified how adopting a shadow host into another document re-resolves any url()s in its stylesheets?)

@bzbarsky
Copy link
Author

So that suggests we should be fine to share a stylesheet across documents?

That's what @rakina and @domenic and @emilio and I were discussing during break. The conclusion so far was that it might be feasible. There's some double-checking to be done.

Is it already specified how adopting a shadow host into another document re-resolves any url()s in its stylesheets?

If we're talking about sheets that come from the shadow DOM, for sheets loaded via <link> there would be nothing to re-resolve. For sheets loaded via <style>, I'm not sure what happens.

One thing we could do for constructible stylesheets is to just capture a base URL at construction time and not do any re-resolving on adopt.

@tabatkins
Copy link
Contributor

One thing we could do for constructible stylesheets is to just capture a base URL at construction time and not do any re-resolving on adopt.

While I'm sad that this doesn't match the behavior of other stylesheets with relative urls, I am happy that this is way simpler.

@bzbarsky
Copy link
Author

While I'm sad that this doesn't match the behavior of other stylesheets with relative urls

Does it not? I'm not aware of other cases where re-resoving happens, offhand, but it's been a while since I looked at the relevant bits.

Definitely in Gecko a base URL is captured by a CSSStyleSheet at the point when it's created.

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

Successfully merging a pull request may close this issue.

7 participants