construct-stylesheets

construct-stylesheets copied to clipboard

I get the reasoning to forbid @import from replaceSync, but it's not clear to me why forbidding it in insertRule is desired, or necessary. We allow insertRule with @import with other stylesheets.

Looking a bit at the history this comes from https://github.com/WICG/construct-stylesheets/issues/56 and such.

Do we still have hopes to eventually be able to share these stylesheets across documents? If so it makes sense to add a note to the spec saying that this is why this restriction is there.

If not, this restriction should probably be removed?

cc @bzbarsky @rakina @tabatkins

Given replace() allows @import now (right?) there's no reason to not allow insertRule with the same semantics, unless I'm missing something.

I don't really remember the details but I think one of the reasons we wanted to disallow @import in insertRule was because it was because of fetch groups (see https://github.com/WICG/construct-stylesheets/issues/15#issuecomment-432238521). I'm not sure why the static vs dynamic differs here, probably @bzbarsky can explain more?

That was back when the idea was that the same sheet could be shared across multiple documents. Static vs dynamic was relevant because for the static case there was an obvious document to use that actually made sense, whereas for dynamic there wasn't.

But in today's spec https://wicg.github.io/construct-stylesheets/#dom-documentorshadowroot-adoptedstylesheets setter step 2 prevents use across documents, so there is always a well-defined document and its fetch group can just be used. Unless we still plan to relax that restriction in the future, I don't see a reason to prevent dynamic @import.

Yeah, looks like we should be good to remove the restriction now.

The CSS Working Group just discussed Add a note about the reasoning to forbid `insertRule(@import)`, or remove the condition?, and agreed to the following:

• RESOLVED: Disallow @import in all constructable stylesheet apis with a note that we're doing it to match current state of modules and this might relax in future

The full IRC log of that discussion

<dael> Topic: Add a note about the reasoning to forbid `insertRule(@import)`, or remove the condition?
<dael> github: https://github.com/WICG/construct-stylesheets/issues/119
<dael> TabAtkins: Spec has a restriction you cannot use insert to insert @import on constructable
<dael> TabAtkins: Other reasons to not allow @import like if you're doing a sync bc imposes some async and stylesheet won't be ready until finish import.
<dael> TabAtkins: General case have blanket disallow of @import. Blanket reason is we intended Const SS usable across documents and there's technical details with fetch group effecting how you would import and you don't want to leak resources
<dael> TabAtkins: So we said kill entirely and avoid issue
<dael> TabAtkins: We no longer do that. We have explicit check that it must be same document. No longer a good reason to disallow
<dael> TabAtkins: Suggestion is we remove that, the sync call we keep but all other ways of putting @import should be valid now
<dael> astearns: Concerns?
<dael> TabAtkins: Prop: Remove the general disallow of @import rules in Const SS. Replace sync method call will still fail with @import rule but all other cases will allow @import
<dael> chrishtr: And didn't do it before for consistent?
<dael> TabAtkins: No, because if share cross doc fetch group depends on doc and if on different doc fetch group is unclear. Now prevent cross doc use so it's fine.
<dael> chrishtr: And that was after resolve on import when shipping in first place
<dael> TabAtkins: Yes
<dael> chrishtr: Okay
<dael> astearns: Prop: No longer forbid @imports
<dael> astearns: Objections?
<dael> chrishtr: What happesn to css module?
<dael> chrishtr: People working on css modules where you import stylesheet as module that doens't have imports. I guess restriction flows directly through.
<dael> TabAtkins: Haven't looked exactly. Generic restriction doesn't apply since JS attached to a document. Not certain we want sync/async to matter in JS imports. If we do should apply at time you import. Good poin to raise and I'll make sure we handle that property
<dael> dbaron: My memory of css imports TAG review is they didn't want to make design decision on if @import should build module graph or function as normal so they punted on that and disallowed
<dael> chrishtr: Right, it complicates JS module graph. If you have a stylesheet and add @import when does browser fetch?
<dael> TabAtkins: Immediately after insert rule
<dael> chrishtr: Before you put in adopted array?
<dael> TabAtkins: Think so
<dael> chrishtr: When do we load images?
<dael> TabAtkins: Illdefined but not until applied to an element.
<dael> TabAtkins: Replace call is async so delay wait for imports isn't huge. Insert is a sync call and no way to tell except checking if an imported stylesheet has loaded
<dael> TabAtkins: If you take a normal stylesheet and an @import rule it immediately loads. async in bg
<dbaron> The TAG review I mentioned was https://github.com/w3ctag/design-reviews/issues/405
<dael> chrishtr: After instered a Const SS you can add rules and so similar behavior should occur
<dael> TabAtkins: Yeah. Least difference between UA and Const is good
<dael> chrishtr: So shouldn't loa duntil you stick it in b/c doens't load until it's in the DOM
<dael> TabAtkins: True. Warrents further timing discussion
<dael> TabAtkins: Regardless of timing, do you have objections to generally allowing import?
<dael> chrishtr: Obj b/c it's inconsistent and if other cases are best practice they can't use best if authors doing @import
<dael> chrishtr: Stylesheet in a JS module is a best practice and they can't have @import so devs would avoid it and use another mech
<dael> emilio: You can add imports to regular stylesheets so I don't know objections
<dael> chrishtr: Procomplie SS, stick in JS module, and then they put it into adopted stylesheet by importing module but that doesn't allow adding @import unless have special code where after adding JS they manuuall add import
<dael> emilio: You can import multi module right?
<dael> chrishtr: Right, multiple modules to rep imports rather then it implicit based on module dependency
<dael> emilio: Still not sure...
<dael> chrishtr: Inconsistent
<dael> emilio: I think it's inconsistent to allow rules expect @import. Current spec wording is wrong if we're disallowing @import
<dael> chrishtr: I prefer to think more and talk to other engineers about implications
<dael> TabAtkins: Sympathetic to that it's good for css mdoules and Const SS work the same. Since punting on modules we should punt on Const SS
<dael> TabAtkins: So I'm okay keeping restrinction until decide on CSS modules
<dael> emilio: Wouldn't that mea have to handle them in cssstyle.docReplace? Const have @import if you use async
<dael> TabAtkins: Yes, idea is be consistent and disallow @import in all Const stylesheets with poss to change later
<dael> emilio: Means you need to change replace and such
<dael> TabAtkins: Yeah
<dael> emilio: I would rather either both or none for allowing
<dael> TabAtkins: Agree
<dael> astearns: Are we at poitnt o resolve on disallow or do we need research ong css modules?
<dael> TabAtkins: I think. Prop: Const stylesheets should be same as modules and disallow @import in all cases
<dael> TabAtkins: To be consistent with modules and to deal with emilio issue on handling constructable imports
<dael> dbaron: Thing with modules felt it was disallowing b/c not wanting decision on how it should work, not because either option was bad
<dael> TabAtkins: Yes, I believe taht is correct
<dael> chrishtr: True that async replace allows @import in chromium?
<dael> emilio: Yes
<dbaron> s/Thing with modules/I'd hope to see this resolved sooner. Thing with modules/
<dael> astearns: Prop: Disallow @import in all constructable stylesheet apis with a note that we're doing it to match current stte of modules and this might relax in future
<dael> emilio: Error handling? Reject, fail to load?
<dael> TabAtkins: Invalid or error?
<dael> emilio: Right
<dael> TabAtkins: What are we doing for modules?
<dael> TabAtkins: I think answer is match css modules. If not clear need and answer for both
<dael> emilio: Modules should match inserting invalid @import rule. But that's another discussion
<dael> TabAtkins: Yeah. I think calling insert rule with unknown rule silently fails. I suspect best to match with inserting import into import disallowed sheet.
<dael> emilio: Maybe. Don't need to descide now
<dael> astearns: Obj to Disallow @import in all constructable stylesheet apis with a note that we're doing it to match current stte of modules and this might relax in future
<dael> RESOLVED: Disallow @import in all constructable stylesheet apis with a note that we're doing it to match current state of modules and this might relax in future
<dael> astearns: If error handling is not immediately clear please raise an issue
<dbaron> Was the resolution intended to have "for now" in it?
<dbaron> oh, I guess it does at the end

Distilling the discussion:

• Chris raised the problem the JS importing a CSS Module currently disallows @import rules (as a way of punting on whether the imported sheets should be part of the module graph (and thus shared between distinct imported sheets that @import the same URL) or not (distinct stylesheet objects)). If authors aren't going to be able to use @import in CSS Modules, it'll be weird to allow it here; it's unclear that toolchains will usefully manage that distinction.

• Emilio's hidden point in his OP is that currently, .replace() on a constructed stylesheet does allow @import; it was weird that .insertRule() disallowed it. People on the call agreed this should be consistent.

• Overall resolution is that we should keep the restriction, and make .replace() consistently restrict it as well.

• We should also push for solving the CSS Module @import discussion one way or the other, and then be consistent with that in constructed stylesheets.

Given the discussion on the blink-dev mailing-list we should discuss what the error handling mechanism here should look like.

I've asked @astearns to put it on the agenda for the next week telecon, does that seem fine with everyone?

cc @mfreed7 @chirshtr

Oh, for reference the blink-dev thread is: https://groups.google.com/a/chromium.org/d/msg/blink-dev/RKG8oxp22RY/fdFnG1rGCgAJ

I think the following behavior for Constructed StyleSheets would make the most sense:

• replace() ignores @import rules and continues to parse the rest of the sheet.
• replaceSync() ignores @import rules and continues to parse the rest of the sheet.
• insertRule() throws a SyntaxError if the rule is an @import rule.

It is more consistent with existing behaviors to continue to parse the rest of the sheet, rather than to throw a NotAllowedError and reject the entire parse if there is an @import rule.

Does insertRule() throw SyntaxError in other cases?

Yes:

document.styleSheets[0].insertRule("@foobar") // SyntaxError: An invalid or illegal string was specified

While I think I argued for "silently drop" in the call a bit ago, I think I've swung the other way, and believe we should reject the sheet entirely. That's more consistent with JS imports, which fail the module load entirely. They do that for a good reason, too - it makes it safer to upgrade into supporting @import later, because it's very likely that authors aren't depending on a sheet being completely ignored. (While a sheet working slightly wonky because some of its @imports don't load is more plausibly ignorable by an author.)

The CSS Working Group just discussed Add a note about the reasoning to forbid `insertRule(@import)`, or remove the condition?, and agreed to the following:

• RESOLVED: @import doesn't parse in constructable stylesheets and as a result throw syntax errors

The full IRC log of that discussion

<dael> Topic: Add a note about the reasoning to forbid `insertRule(@import)`, or remove the condition?
<dael> github: https://github.com/WICG/construct-stylesheets/issues/119#issuecomment-594873154
<dael> TabAtkins: emilio is best to take it. I'm fine with it but I have an answer I want and I'm not sure what others feel
<dael> chrishtr: WE want to discuss if it throws an exception with an illegal rule. Not about allowing imports
<dael> TabAtkins: Yeah. It's if we silently ignore or if we throw an error when you try and create a sheet, replace text, or insert an import rule directly
<dael> Rossen_: Is there a proposed part forward with exception or silent?
<dael> TabAtkins: Not a decision yet. I'm for throwing b/c more consistent with CSS modules. This makes it easier to fix later. If we end up allowing imports it's less likely authors depend on it if the module is completely broken. For consistency and friendly to futire decisions I think we should throw entirely for now if you try and include an import
<dael> emilio: I disagree. I don't think it's different then syntax that doesn't work and eventually works. We should be consistent about syntax we want to eventually support. If we add @import supports and silently ignore but if we have syntactically valid we throw
<dael> Rossen_: Sounds like we have consensus around throwing
<dael> emilio: I'm arguing to not throw
<dael> chrishtr: I agree with emilio not throwing is better. Can't we change css modules to not throw?
<dael> TabAtkins: We can. It would mean we're slightly more likely to pick up a dependency if we don't decide soon
<dael> TabAtkins: I agree silent ignore is better overall. Consistency and reason css modules does it is compelling to me.
<dael> Rossen_: There were lengthy discussions on css modules and how they should work. Part of the recent tag review about event loop. Chasing down the thread between those that were there...there were a lot of discussions on this and pointers between groups saying css should define if we throw and us not. Lots of what's on WICG assumes we're throwing. It would be good to reconcile these discussions
<dael> Rossen_: If we have strong proposal to not throw that's fine but I think we will need to reopen the larger discussion
<dael> Rossen_: I don't think this will be end of conversations if we change. But do we have consensus on not throwing
<dael> TabAtkins: If we can take it back to css modules I'm fine with not throwing
<dael> Rossen_: So update this issue to say this will be consistent with css modules?
<dael> chrishtr: [missed] not to throw
<dael> Rossen_: If modules define not it's fine
<dael> emilio: Modules defines just replacing. If modules wants to throw they might need to do a general thing to throw. I still would argue that's not great
<dael> chrishtr: Suggest we resolve not to throw and someone takes and action to check this is okay with modules. If it's not okay we come back tot he group
<dael> emilio: Sounds great
<dael> chrishtr: We should also resolve insert rule throws syntax error
<dael> emilio: Can say @import does not parse in constr/ stylesheets and that gives implicit syntax error
<dael> Rossen_: Prop: Do not parse @import which will cause a syntax error and also not throw
<dael> Rossen_: Objections?
<dbaron> Not supporting @import seems like it's becoming progressively more and more complicated...
<dael> emilio: @import doesn't parse in constructable stylesheets and as a result throw syntax errors
<dael> TabAtkins: Agree that's right wording for it
<dael> RESOLVED: @import doesn't parse in constructable stylesheets and as a result throw syntax errors
<dael> Rossen_: dbaron do you want to add to conversation?
<dael> dbaron: Worried we're piling on more stuff to not support @import. Seems it's getting progressively more complex
<dael> emilio: I don't know how throwing or not makes this more complicated.
<dael> dbaron: More complicated may not be right, but there's more decisions about it because it's different
<dael> emilio: Our const. style sheets we plan to show a warning if you stash an @import
<dael> emilio: Doesn't seem situation is worse by not throwing. In fact I think it's better
<dael> dbaron: I would be pessimistic about ever being able to change this. If we don't support @import we'll be stuck with that
<dael> emilio: That's a different discssion though? At least in replacing you have to define a way to [missed]
<TabAtkins> Phew, I simply can't hear Emilio due to the background conversation.
<dael> emilio: I was saying we need to define an error handling for replacing. Doesn't seem to me...we need to define error handling either way. I see dbaron point about not supporting @import but it's a different convo. For replacing you need error handling and I think this is most consistent
<dael> Rossen_: dbaron Do you want to reconsider the resolution? Back to the issue and discuss there?
<dael> TabAtkins: Alternative is we go resolve the issue for css modules about if imports are shared or independent in module graph. We're trying to avoid b/c we couldn't decide on that question and didn't want to lock in api
<dael> emilio: And about sharing cross document which we're not doing
<dbaron> I think we'd be much better off if we just resolved the CSS modules thing one way or the other.
<dael> Rossen_: Given this is in WICG still by no means is this final final. If you need more time to work with module folks that would be good to do so and see if we can help close the issue for the design and the @import decisions will be taken care of here by time modules are complete
<dbaron> I suspect that supporting @import either way would be less bad than not supporting @import.
<dael> chrishtr: Need decision on throwing because it impacts something being implemented. If we can preserve resolution of exception that would be good
<dael> Rossen_: We did record a resolution
<dael> chrishtr: Checking it still holds
<dael> Rossen_: Yes. Trying to see if dbaron wants to object and keep working on this or keep as is as a stop gap until modules is in better shape. dbaron can you comment on your preference to move forward?
<dael> dbaron: If you give me enough opportunities to object at some point I will
<dael> Rossen_: You've got an opportunity to object right now
<dael> dbaron: Having all this depend on one decision in css modules is weird. But I won't object
<dael> Rossen_: Then previous resolution holds. I'd encourage everyone in this to re-engage with modules and see if we can make progress

• RESOLVED: @import doesn't parse in constructable stylesheets and as a result throw syntax errors

This resolution text is confusing. What we actually resolved is to throw syntax errors in insertRule, but otherwise ignore @import silently.

• RESOLVED: @import doesn't parse in constructable stylesheets and as a result throw syntax errors

This resolution text is confusing. What we actually resolved is to throw syntax errors in insertRule, but otherwise ignore @import silently.

This was discussed in the blink-dev intent to ship thread, but for those arriving here from the Chromium deprecation message, here is the more detailed description of the changes, at least as they are implemented starting in Chromium 84:

• When replaceSync() encounters an @import rule, it will now issue a warning and will ignore the rule. However, no exception will be thrown.
• When replace() encounters an @import rule, it will issue a warning, and will ignore the rule.
• In both cases above, sheet parsing will continue after the @import rule.
• On document.styleSheets, insertRule() will allow insertion of @import rules. No change here.
• On constructed stylesheets, insertRule() inserting an @import rule will throw a SyntaxError instead of a NotAllowedError.