Specification text for replace() does not match Chromium implementation

[nordzilla]

Description

I am currently implementing the replace() functionality for Constructable StyleSheets in Firefox. I noticed that the specification steps for replace() do not match what is implemented in Chromium.

To be clear, I think that what is implemented in Chromium is more desirable than what is described in the spec, and I think that the spec should be updated to reflect this.

Differences

The sheet keeps the new rules, even if an @import rule fails:

Spec

• In step 5) when an @import rule fails to load, there is no text that says to set the sheet's rules to the new rules, suggesting that the sheet should retain its previous rules.

Actual

• https://jsfiddle.net/enordin/vnzo3809/17/
• The color of the square is black, which is specified in the newest set of rules after the failed @import rule.
• If the previous rules were kept, the color of the square would be yellow.

---

Other @import rules are loaded, even if one of them fails.

Spec

• In step 5) the spec says, "If any of them [the @import rules] failed to load, terminate fetching of the remaining @import rules..."

Actual

• https://jsfiddle.net/enordin/vnzo3809/24/
• The color of the square is gray, which is specified in the second @import rule.
• If the other @import rule were cancelled, or if the previous rules were kept, the color of the square would be yellow.

Proposed Resolution

I think that the spec text should be modified to convey the following:

• Assign the new rules to the sheet, even if one or more @import rules fail to load.
• Continue with loading other @import rules, even if at least one @import rule fail to load.

This is also consistent with how loading @import rules works in contexts other than Constructable StyleSheets.

[chrishtr]

Please note that we look likely to remove support for @import from style sheets loaded via replace(), in order to be consistent with all the other ways of creating such style sheets, and therefore improve API interop between them.

See #119 .

Are there use cases where you think it would be unreasonable to work around the removal of @import?

[nordzilla]

@chrishtr Thanks for the clarification. I had started work on this feature prior to the decision about #119. As it says in #119, "this [restriction] might relax in future."

I wouldn't expect Chromium to fully undo its existing implementation details. It will probably just restrict the behavior as early as possible in the method call. This way it's still ready to go if/when the restriction relaxes.

Even if the current plan is to disallow @import rules for now, I think it would still be worthwhile to gain clarity on this functionality that already exists (yet differs from the current spec).

[nordzilla]

@chrishtr I stand corrected. I had assumed that since the decision from #119 said:

"this [restriction] might relax in the future"

that the underlying implementation would be kept around, and that the call to replace() would just disallow @imports at the top level.

It appears that the plan is, indeed, is to deprecate replace() entirely.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/RKG8oxp22RY/fdFnG1rGCgAJ

[mfreed7]

We’re not deprecating replace() entirely, just the case where the replacement sheet contains an @import rule. Calls to replace() without @import will continue working as is.

[nordzilla]

@mfreed7 Thanks for the update. In that case, I do think that this issue is still relevant.

[mfreed7]

I just wanted to add support to this issue. It would seem to be most-useful to ignore @import rules (with a warning) but keep parsing the remaining rules. This is completely consistent with regular <link rel="stylesheet"> parsing.

Note that only "part 1" of the suggestion is relevant here, I think:

• Assign the new rules to the sheet, even if one or more @import rules fail to load.

Part 2, I think, is moot, given 119 and I2D:

• Continue with loading other @import rules, even if at least one @import rule fail to load.

[dandclark]

cc @tabatkins @rakina @mfreed7
If I wrote a PR to update this spec to match the current Chromium implementation, would it be accepted? Or was the plan to wait until CSS modules had reached a conclusion on what to do about @import, and only update the spec after that?

Quoting myself from here, after the recent TPAC breakout session I think it would be useful to move forward in this manner with an @import-blocking version of CSS modules:

1. Update https://wicg.github.io/construct-stylesheets so that it reflects the behavior for replace and replaceSync resolved in the CSSWG (ignore @import but don't stop parsing or throw an error).
2. In the CSS Modules implementation, make the create a CSS module script steps use the replace steps in https://wicg.github.io/construct-stylesheets/#dom-cssstylesheet-replace to parse the CSS text and generate the stylesheet object. This includes failing the module load if replace throws an error. Referencing constructable stylesheets in this way codifies the matching behavior of these features right into the spec.
3. Optionally circle back to the CSSWG to reconsider holistically whether replace, replaceSync, and CSS modules should all throw an error on @import instead of ignoring. Any change here would be applied to the constructible stylesheets spec, and CSS modules would get the new behavior automatically.

And then eventually:

4. Update the constructable stylesheet spec to allow @import in the replace() steps after deciding whether @import should have module syntax or not, which would also allow use of @import in CSS modules.

Shipping and standardizing an @import-less version of CSS modules will build developer excitement around the feature and give us insights into how it is used in production, and gathering feedback about this could help ensure we arrive at the correct final design choice for handling @import without risking backwards compatibility problems. But if we wait until there is consensus on the final behavior of @import in CSS modules before standardizing something simpler, then the feature and thus the behavior of @import in replace() might languish indefinitely.

[mfreed7]

If I wrote a PR to update this spec to match the current Chromium implementation, would it be accepted? Or was the plan to wait until CSS modules had reached a conclusion on what to do about @import, and only update the spec after that?

I agree with just about everything in your post - I think this is the right way forward. Let's limit the use of @import now, and see how this feature is used. And let's definitely spec these two features (CSS Modules and Constructable Stylesheets replace) to point to the same spec algorithms, so they behave the same.

The only real question I (still) have is whether throwing exceptions (and stopping parsing) is the more "future proof" way to go here. I know we went the opposite direction for Constructable Stylesheets, and I fully support making that behavior the same as the CSS Modules behavior. But I'm wondering if we shouldn't revisit that, since CSS Modules are likely to get significant usage. I just want to make sure we're leaving ourselves open to later allow @import without compat problems. The two options are:

1. Silently ignore @import: if @imports are later allowed, stylesheets that were previously silently ignored will now get fetched and applied.
2. Throw an exception on @import: if @imports are later allowed, exceptions will stop being thrown, and stylesheets will be fetched and applied.

Of these, #2 seems less likely to get baked in somewhere. If exceptions get thrown, I'd expect developers to fix/remove the offending @import, and reduce the occurrence of this situation. For #1, it seems too easy for developers to ignore this situation because it doesn't cause any issues, other than perhaps a console warning. The downside of #2, as @emilio pointed out, is that it doesn't match the "standard" more-permissive parsing of CSS.

[dandclark]

I'm finally getting to this with #137 and #138.

One change from the plan I discussed here is that for now I think CSS modules should still reference the steps from replaceSync(), not replace(). The HTML spec assumes that the steps to create a module script will finish synchronously, so we'd need some additional refactoring to make it work with the replace() steps since replace() runs some of its steps in parallel. If we end up making @import in a CSS module create a CSS module, then we couldn't use replace() anyway; we'd need some new algorithm that still parses the CSS text synchronously but makes each @import rule create a new CSS module script.

@mfreed7 I am sympathetic to the concerns that ignoring @import without an exception is less future proof. I guess it's a tradeoff between more robust future proofing and having the behavior be more consistent with existing CSS parsing rules. I'm fine with either outcome as long as CSS modules and constructable stylesheets do the same thing. Should we raise this as CSSWG agenda item again? I'm open to this but I'm not sure that we'd get consensus on a different outcome. @emilio has made good points on keeping the parsing behavior consistent with CSS precedent, and at the TPAC session https://www.w3.org/2020/10/29-components-minutes.html my recollection is that the room was mostly leaning towards ignoring rather than throwing for @import, although the notes don't reflect a conclusion there.