Clarify loading stuff and disallow multiple document usage for now #69
Conversation
|
Could you see more about what motivates the single document restriction? |
My recollection of TPAC was that nobody had any use cases for multiple documents, and thought it would be weird anyway since style sheets are tied (at least through base URL in specs, and maybe more in implementations) to one document. So at first we were all thinking it would be a good idea to disallow multiple document support. Then, we thought about how you'd actually disallow multiple document support. At the time the best we could come up with required modifying the adopt a node algorithm to peek into any shadow roots and error out if there were cross-document adopted stylesheets. That's gross (and a big layering violation), so we wanted to avoid that. So, we thought harder, and realized that theoretically we could make multiple documents work. It took more machinery, i.e. all the tracking of list-of-adopted-documents and so on. But that's what we wrote up. Discussion around #15 (comment) . Then @rakina went to implement in Blink, and our loading team (@nyaxt in particular) noted that in Blink we tie stylesheets much more strongly to documents than the spec does. So, the specced multi-document support was not feasible for us to implement in the near-/medium-term timeframe; we'd instead have to start with single-document functionality (probably with modifications to adopt-a-node? Or, just no re-fetching with the new document?). With this new info, we wanted to think harder if we could go back to (our recollection of) the original TPAC desire for single-document-only. We came up with the idea here: don't bother throwing on adoption or anything, just ignore stylesheets from another document. This is kind of nice in that it matches how we treat scripts. That's the story and motiviation. Our hope is that we can start simple, and more-easily-implementable, with single document only. Eventually we could loosen that up (probably with opt-in to avoid compatibility problems), if it actually turns out to be important. But our suspicion is that multi-document is not actually an important use case. It was just the less-bad option compared with modifying adopt a node. Hope this helps! |
|
I don't see how they are tied through the base URL. A style sheet has its own base URL. They are tied through the "fetch group", which is poorly defined, but basically comes down to needing a document to being responsible for fetching subresources. I can see how that's hard to deal with, in particular with the state of specifications around those concepts. It seems we couldn't loosen things if the style sheets are present but ignored. As at that point they'd become present and used, which would likely regress things. |
|
@rakina you want to go in parallel once and then run a set of steps there. The first step is to parse, then it's to load @import recursively. Then it's to wait for that to finish (or one to fail). I'd probably say something like "terminate fetching", since eventually all of this needs to be on top of https://fetch.spec.whatwg.org/. |
|
Right, that's why I mentioned opt-in, e.g. a { crossDocument: true } argument to the CSSStyleSheet constructor. |
|
I realized a problem with the new "in parallel" version, which is that you modify the sheet's CSS rules (i.e. modify a main thread object) from the "in parallel" section (i.e. from the background thread). See https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-for-spec-authors for more. The same is true for the various flags you set before resolving the promise. So, the more correct version (sorry for all the back and forth!) would be something like.
Happy to explain any of this in more detail in person today; I just wanted to note it before I forgot. |
|
I just noticed the explainer probably needs a bit of updating too, and it should be in the same PR if possible so that things stay consistent. I think this is as simple as changing |
| // Fine, returns Promise that resolves when 'some.css' finished loading. | ||
| sheet.replace("@import('some.css');"); | ||
| // Fails | ||
| sheet.replace("@import('some.css');"); | ||
| sheet.insertRule("@import('some.css');"); | ||
| ``` | ||
|
|
||
| * Each constructed `CSSStyleSheet` is "tied" to the `Document` it is constructed on, meaning that it can only be used in that document tree (whether in a top-level document or shadow trees). | ||
|
|
||
| * Each constructed `CSSStyleSheet` is "tied" to the `Document` it is constructed on, meaning that it can only be used in that document tree (whether in a top-level document or shadow trees). It can be adopted into a different document tree, but it will be ignored for style calculation purposes. |
There was a problem hiding this comment.
So this means that the proposed behavior is to keep the StyleSheet objects in the ShadowRoot, but ignoring them during style recalc? That sounds like a huge pain to me.
There was a problem hiding this comment.
Hmm, interesting, can you explain more about why it's hard in Gecko? For blink the change is quite simple (see this CL). Also, since this PR is merged already, it might be better to continue this discussion in a new issue on this repo. I can make one if you want, or you can start one with your perspective on this.
There was a problem hiding this comment.
(oops, linked the wrong CL, I edited it but in case you're reading from email, the CL is https://chromium-review.googlesource.com/c/chromium/src/+/1377486)
There was a problem hiding this comment.
I don't think it's hard to implement or such, but feels really weird that you can poke at the cssRules of an object and change it without having any effect whatsoever on the page.
There was a problem hiding this comment.
The spec already says we should throw if any of the sheets in the array are not constructed. Why not also throw if any of the sheets have a different constructor document?
There was a problem hiding this comment.
@lilles So the difference is for non-constructed stylesheets the entry point is only when adding to the adoptedStyleSheets, while for different-document one there's also the case of adopting a subtree that contains adoptedStyleSheets with stylesheets from another document tree. That means we need to change the "adopt a node" spec to to go through every shadow root and check each entry in their adoptedStyleSheets, which might be annoying.
There was a problem hiding this comment.
BTW I reopened #23, might want to continue there.
Adds some notes about
@importloading, and restricts usage of stylesheets to just one document.Fixes #68, fixes #66, fixes #65, fixes #64, fixes #63, fixes #62, fixes #54, fixes #26, fixes #15.
cc @bzbarsky, we think we don't want to allow usage in multiple documents in the spec for now. Blink is looking into it and realized it's hard to allow that and we might want to ship with single-document usage at first, so we think maybe it's a good idea for the spec to be more restrictive, at least for now?
Also cc @domenic @dbaron @annevk
Preview | Diff