Intent to Remove: @import rules in CSSStyleSheet.replace()

[Mason Freed]

Contact emails mason...@chromium.org Explainer Design docs/spec Specification: https://wicg.github.io/construct-stylesheets/ https://github.com/WICG/construct-stylesheets/blob/gh-pages/explainer.md TAG review Summary The original spec for constructable stylesheets allowed for calls to sheet.replace("@import('some.css');"). This use case is being removed - calls to replace() will throw an exception if @import rules are found in the replaced content.

The plan is to deprecate this feature in M82, and remove it in M83.

The CSS Working Group resolved to remove this feature on Feb 19, 2020.

Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)? Yes Is this feature fully tested by web-platform-tests? Yes There are existing WPT tests for constructable stylesheets (https://wpt.fyi/results/css/cssom/CSSStyleSheet-constructable.html) which will be modified to match this removal. Link to entry on the Chrome Platform Status https://chromestatus.com/feature/4735925877735424

[Philip Jägenstedt]

LGTM1, the WG resolution looks sensible and it's a good idea to act fast on issues for https://wicg.github.io/construct-stylesheets/ raised by other vendors.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAM%3DNeDiG%3DTmbkfL0u%3Du6smGPhzt2n%2B9RtadQ04ouLw54cWoLsA%40mail.gmail.com.

[Mounir Lamouri]

What's the current usage of the feature? Is it implemented in other browsers? Are they also dropping it?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYft2NjUnWLQxwt3Rb8QTz5j5cgSf9MxuRLTKCm4dny3nw%40mail.gmail.com.

[Mason Freed]

On Tue, Feb 25, 2020 at 4:07 PM Mounir Lamouri <mlam...@google.com> wrote:

What's the current usage of the feature? Is it implemented in other browsers? Are they also dropping it?

Good questions. I thought I had included the usage in the I2R template but I guess not. The current usage for all calls to CSSStyleSheet.replace() is just over 0.03%. Note that this includes calls that do not load rules containing @import, so it overcounts this deprecation. I'm landing a counter to measure the actual usage of replace() with @import, and I'll check back here when I have that data. Other browsers have not yet implemented constructable stylesheets in general. Mozilla is in the process of implementing it, which is what triggered this discussion and deprecation.

Thanks,

Mason

[Manuel Rego Casasnovas]

On 25/02/2020 23:53, Mason Freed wrote:
> The original spec for constructable stylesheets allowed for calls to
> sheet.replace("@import('some.css');"). This use case is being removed -
> calls to replace() will throw an exception if @import rules are found in
> the replaced content.

It seems error handling was not discussed/agreed on the CSSWG
resolution. Dunno if this was agreed somewhere else, but otherwise I
guess it'd be nice to have a proposal and agree there with other parties.

Bye,
Rego

[Philip Jägenstedt]

Per testing https://software.hixie.ch/utilities/js/live-dom-viewer/saved/7780 in Firefox and Safari, they don't have CSSStyleSheet.prototype.replace at all. Per the same test, it looks like it was shipped in Chrome 73 (intent).

Thanks Mason for adding the extra use counter. If you'd like to, I think it'd make sense to land that counter now, and the removal right after the next branch point (week of March 12). That would give enough time to have some numbers in time to revert the removal if necessary.

There's a certain cost to delaying too, so if you think @import in replace() calls is likely a very small proportion of total replace() calls, then adding the exception and counter in the same release and comparing relative usage in the beta channel would also work.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/6cb07226-6e0c-a794-dc3c-39444419fa27%40igalia.com.

[Emilio Cobos Álvarez]

I think the main reason on why we didn't have consensus on error
handling is because we didn't know off-hand how CSS imports were
handling it. They just rely on replaceSync turns out.

I think the "throw/reject if there are any import rules" is a bit
inconsistent with how the rest of stylesheet parsing works everywhere
else. This is a problem that replaceSync() also has, fwiw.

* Syntactically valid @import rules just disappear from a stylesheet.
* Other kind of invalid @import rules (like, failed or blocked loads)
just have an empty CSSStyleSheet.

It's slightly weird that constructable stylesheets introduce this third
failure mode where if you have an @import your whole stylesheet is
invalid... But anyhow not really opposed to it, just odd, and causes our
implementation to have a somewhat ugly special-case just for this.

But yeah, we should resolve on something here, even if it's just
rejecting / throwing.

-- Emilio

> Bye,
> Rego
>

[Chris Harrelson]

Hi Emilio,

On Wed, Feb 26, 2020 at 2:11 AM Emilio Cobos Álvarez <emi...@mozilla.com> wrote:

On 2/26/20 10:36 AM, Manuel Rego Casasnovas wrote:
>
> On 25/02/2020 23:53, Mason Freed wrote:
>> The original spec for constructable stylesheets allowed for calls to
>> sheet.replace("@import('some.css');"). This use case is being removed -
>> calls to replace() will throw an exception if @import rules are found in
>> the replaced content.
>
> It seems error handling was not discussed/agreed on the CSSWG
> resolution. Dunno if this was agreed somewhere else, but otherwise I
> guess it'd be nice to have a proposal and agree there with other parties.

I think the main reason on why we didn't have consensus on error
handling is because we didn't know off-hand how CSS imports were
handling it. They just rely on replaceSync turns out.

I think the "throw/reject if there are any import rules" is a bit
inconsistent with how the rest of stylesheet parsing works everywhere
else. This is a problem that replaceSync() also has, fwiw.

  * Syntactically valid @import rules just disappear from a stylesheet.
  * Other kind of invalid @import rules (like, failed or blocked loads)
just have an empty CSSStyleSheet.

In what cases are you saying @import rules just disappear? In Chromium's implementation of replaceSync?

 

It's slightly weird that constructable stylesheets introduce this third
failure mode where if you have an @import your whole stylesheet is
invalid... But anyhow not really opposed to it, just odd, and causes our
implementation to have a somewhat ugly special-case just for this.

I think it's important for @import to throw for these cases, so that it's possible to add @import later if we need to.

[Emilio Cobos Álvarez]

No no, I mean when something doesn't parse in a stylesheet like:

@import garbage;

Or anything more sensible, like the yet-to-be-implemented-in-browsers:

@import "my-fancy-grid.css" supports(display: grid);

They just don't appear in the stylesheet. They don't cause anything else
below them not to apply.

That's the mechanism CSS usually has to deal with unsupported stuff,
which may or may not be the best.

> It's slightly weird that constructable stylesheets introduce this third
> failure mode where if you have an @import your whole stylesheet is
> invalid... But anyhow not really opposed to it, just odd, and causes
> our
> implementation to have a somewhat ugly special-case just for this.
>
>
> I think it's important for @import to throw for these cases, so that
> it's possible to add @import later if we need to.

I don't think not-throwing prevents you from adding @import in the
future, in the same way it doesn't prevent you from adding any other CSS
feature like @import supports(..); above, right?

I was just pointing out that this failure mode is unheard of, as far as
I can tell, and that saying "@import rules don't parse for Constructable
StyleSheets / CSS Modules" is a much more natural way to handle it (in
my opinion). You can still detect whether support is present by looking
at cssRules.length, or calling functions that throw on invalid rule
syntax like insertRule.

-- Emilio

[Mason Freed]

On Wed, Feb 26, 2020 at 2:00 AM Philip Jägenstedt <foo...@chromium.org> wrote:

Per testing https://software.hixie.ch/utilities/js/live-dom-viewer/saved/7780 in Firefox and Safari, they don't have CSSStyleSheet.prototype.replace at all. Per the same test, it looks like it was shipped in Chrome 73 (intent).

Thanks Mason for adding the extra use counter. If you'd like to, I think it'd make sense to land that counter now, and the removal right after the next branch point (week of March 12). That would give enough time to have some numbers in time to revert the removal if necessary.

There's a certain cost to delaying too, so if you think @import in replace() calls is likely a very small proportion of total replace() calls, then adding the exception and counter in the same release and comparing relative usage in the beta channel would also work.

Thanks - I landed the counter and the deprecation message a few days ago. Pending resolution of the "throw or don't throw" question below, I can also land the removal as soon as Canary 83 branches. That's a good suggestion, thanks.

> I was just pointing out that this failure mode is unheard of, as far as I can tell, and that saying "@import rules don't parse for Constructable StyleSheets / CSS Modules" is a much more natural way to handle it (in my opinion). You can still detect whether support is present by looking at cssRules.length, or calling functions that throw on invalid rulesyntax like insertRule.

I think I generally agree with you that throwing on @import is a bit odd, as compared to the other ways to load stylesheets. The only problem I see is that from a developer's point of view, it doesn't give much guidance that your @import didn't load, or why. It feels a bit too "silent". Especially in the case that you were previously using this functionality, and then M83 rolls around and it just stops working. I suppose we could add a console warning to make that easier to see?

We do need a resolution on what to do when replace() is called with @import rules.

 

On Wed, Feb 26, 2020 at 10:37 AM Manuel Rego Casasnovas <re...@igalia.com> wrote:

On 25/02/2020 23:53, Mason Freed wrote:
> The original spec for constructable stylesheets allowed for calls to
> sheet.replace("@import('some.css');"). This use case is being removed -
> calls to replace() will throw an exception if @import rules are found in
> the replaced content.

It seems error handling was not discussed/agreed on the CSSWG
resolution. Dunno if this was agreed somewhere else, but otherwise I
guess it'd be nice to have a proposal and agree there with other parties.

Bye,
  Rego

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/6cb07226-6e0c-a794-dc3c-39444419fa27%40igalia.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYc4FPjpUXiWeYudFBy%3DSQSivwib3Uwaki8FAoYjFDqpSw%40mail.gmail.com.

[eno...@mozilla.com]

I am also in favor of not throwing. I think it would be cleaner and more consistent with existing practices to skip over @import rules and let the rest of the parse still apply.

I understand your point about it feeling too silent, and I think that adding a console warning is a good idea.

I think the API would end up better for it, overall.

On Thursday, February 27, 2020 at 1:59:55 PM UTC-8, Mason Freed wrote:

On Wed, Feb 26, 2020 at 2:00 AM Philip Jägenstedt <foo...@chromium.org> wrote:

Per testing https://software.hixie.ch/utilities/js/live-dom-viewer/saved/7780 in Firefox and Safari, they don't have CSSStyleSheet.prototype.replace at all. Per the same test, it looks like it was shipped in Chrome 73 (intent).

Thanks Mason for adding the extra use counter. If you'd like to, I think it'd make sense to land that counter now, and the removal right after the next branch point (week of March 12). That would give enough time to have some numbers in time to revert the removal if necessary.

There's a certain cost to delaying too, so if you think @import in replace() calls is likely a very small proportion of total replace() calls, then adding the exception and counter in the same release and comparing relative usage in the beta channel would also work.

Thanks - I landed the counter and the deprecation message a few days ago. Pending resolution of the "throw or don't throw" question below, I can also land the removal as soon as Canary 83 branches. That's a good suggestion, thanks.

> I was just pointing out that this failure mode is unheard of, as far as I can tell, and that saying "@import rules don't parse for Constructable StyleSheets / CSS Modules" is a much more natural way to handle it (in my opinion). You can still detect whether support is present by looking at cssRules.length, or calling functions that throw on invalid rulesyntax like insertRule.

I think I generally agree with you that throwing on @import is a bit odd, as compared to the other ways to load stylesheets. The only problem I see is that from a developer's point of view, it doesn't give much guidance that your @import didn't load, or why. It feels a bit too "silent". Especially in the case that you were previously using this functionality, and then M83 rolls around and it just stops working. I suppose we could add a console warning to make that easier to see?

We do need a resolution on what to do when replace() is called with @import rules.

 

On Wed, Feb 26, 2020 at 10:37 AM Manuel Rego Casasnovas <re...@igalia.com> wrote:

On 25/02/2020 23:53, Mason Freed wrote:
> The original spec for constructable stylesheets allowed for calls to
> sheet.replace("@import('some.css');"). This use case is being removed -
> calls to replace() will throw an exception if @import rules are found in
> the replaced content.

It seems error handling was not discussed/agreed on the CSSWG
resolution. Dunno if this was agreed somewhere else, but otherwise I
guess it'd be nice to have a proposal and agree there with other parties.

Bye,
  Rego

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blin...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/6cb07226-6e0c-a794-dc3c-39444419fa27%40igalia.com.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blin...@chromium.org.

[Mason Freed]

Two quick updates on this thread:

1. The CSSWG just resolved that replace() and replaceSync() should both ignore @import rules, and not throw exceptions. But insertRule() with an @import rule will throw a SyntaxError. This is what I'll implement in M83.
2. The first data point from the use counter has been reported: the usage of replace() with an @import rule is 0.00005%. This is early data, but encouraging.

Thanks,

Mason

[Mason Freed]

...also, this thread has only one LGTM at the moment. Could a few more api owners please take a look?

Thanks,

Mason

[Yoav Weiss]

LGTM2

@import is bad news for performance. No need to propagate it to new contexts.

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/e405b269-7e3d-4c94-a988-c99261277074%40chromium.org.

[Daniel Bratell]

LGTM3

/Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CACj%3DBEgV4b6ieeDtKi8wOJ%3D_FvkTfBBGHPEyrcNtJ6bT%2BFm0uw%40mail.gmail.com.

[Mason Freed]

Thanks everyone!

[Mason Freed]

So as I previously mentioned, the CSSWG resolved that "@import doesn't parse in constructable stylesheets and as a result throw syntax errors". As this sentence isn't completely clear, and neither is the full transcript of the discussion leading up to the resolution, we have a comment requesting clarification. But as I mentioned below, my working assumption is:
- calling replace() with text that includes @import will silently ignore the @import. No exception or warning. This, in particular, is a larger change than the original I2R indicated.
- calling replaceSync() with text that includes @import will silently ignore the @import. No exception or warning.
- calling insertRule() with an @import rule will throw a SyntaxError.

Prior to these changes, the behavior of Chromium was:
- calling replace() with text that includes @import would work. The returned Promise would resolve once all @imports were loaded. Any invalid @import rules would cause a NetworkError to be thrown.
- calling replaceSync() with text that includes @import would throw a "NotAllowed" exception.
- calling insertRule() with an @import rule would throw a SyntaxError for constructed stylesheets only. No exception would be thrown for document.styleSheets.

I just wanted to call out these differences very explicitly, to make sure everyone is on the same page. LMK if there are disagreements about the above. I plan to make this change soon, for M83, but to be sure, I'd like to ask for another set of LGTMs on this I2R.

Thanks,

Mason

[Yoav Weiss]

My LGTM still stands, but...

On Thu, Mar 19, 2020 at 9:00 PM Mason Freed <mason...@chromium.org> wrote:

So as I previously mentioned, the CSSWG resolved that "@import doesn't parse in constructable stylesheets and as a result throw syntax errors". As this sentence isn't completely clear, and neither is the full transcript of the discussion leading up to the resolution, we have a comment requesting clarification. But as I mentioned below, my working assumption is:
- calling replace() with text that includes @import will silently ignore the @import. No exception or warning. This, in particular, is a larger change than the original I2R indicated.
- calling replaceSync() with text that includes @import will silently ignore the @import. No exception or warning.

Why no warning? How are developers supposed to know that their @imports are not working.

[Mason Freed]

On Thu, Mar 19, 2020 at 11:24 PM Yoav Weiss <yo...@yoav.ws> wrote:

Why no warning? How are developers supposed to know that their @imports are not working.

You make a very good point - it would probably be a good idea to issue warnings, particularly for the new replace() behavior. Unless I hear objections, I’ll plan to do that.

Thanks for the re-LGTM.

Thanks,

Mason

[Rune Lillesveen]

On Thu, Mar 19, 2020 at 9:01 PM Mason Freed <mason...@chromium.org> wrote:

So as I previously mentioned, the CSSWG resolved that "@import doesn't parse in constructable stylesheets and as a result throw syntax errors". As this sentence isn't completely clear, and neither is the full transcript of the discussion leading up to the resolution, we have a comment requesting clarification. But as I mentioned below, my working assumption is:

- calling insertRule() with an @import rule will throw a SyntaxError.

Prior to these changes, the behavior of Chromium was:

- calling insertRule() with an @import rule would throw a SyntaxError for constructed stylesheets only. No exception would be thrown for document.styleSheets.

We haven't use-counted this case right? We assume this pattern does not have a high use count and that this change is largely web compatible? 

[Emilio Cobos Álvarez]

On 3/23/20 10:51 AM, Rune Lillesveen wrote:
>
>
> On Thu, Mar 19, 2020 at 9:01 PM Mason Freed <mason...@chromium.org
> <mailto:mason...@chromium.org>> wrote:
>
> So as I previously mentioned, the CSSWG resolved

> <https://github.com/WICG/construct-stylesheets/issues/119#issuecomment-597733392> that

> "@import doesn't parse in constructable stylesheets and as a result
> throw syntax errors". As this sentence isn't completely clear, and
> neither is the full transcript of the discussion leading up to the
> resolution, we have a comment

> <https://github.com/WICG/construct-stylesheets/issues/119#issuecomment-600410089>

> requesting clarification. But as I mentioned below, my working
> assumption is:
> - calling insertRule() with an @import rule will throw a SyntaxError.
>

> *Prior* to these changes, the behavior of Chromium was:

> - calling insertRule() with an @import rule would throw a

> SyntaxError *for constructed stylesheets only*. No exception would

> be thrown for document.styleSheets.
>
>
> We haven't use-counted this case right? We assume this pattern does not
> have a high use count and that this change is largely web compatible?

My reading of the resolution (looking at the CL) is that this should
only throw for constructed stylesheets, not all stylesheets over-all. I
agree the later would need more measuring.

I think the idea (at least when I proposed) was "@import doesn't parse
in constructed stylesheets", which implies throwing a SyntaxError due to
invalid syntax from insertRule as a side effect, but _only_ on those
stylesheets.

-- Emilio

> --
> You received this message because you are subscribed to the Google
> Groups "blink-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to blink-dev+...@chromium.org

> <mailto:blink-dev+...@chromium.org>.

> To view this discussion on the web visit

> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CACuPfeT8gRVjwQjTZkW_aVWwMB8UiN2C-oshr_YGsVM3qztO%3Dw%40mail.gmail.com
> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CACuPfeT8gRVjwQjTZkW_aVWwMB8UiN2C-oshr_YGsVM3qztO%3Dw%40mail.gmail.com?utm_medium=email&utm_source=footer>.

[Rune Lillesveen]

That doesn't match the CL[1], or Mason's clarification above, which is why I asked.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2106996

[Mason Freed]

Another quick update on this thread:

- per the discussion below, insertRule() will go back to allowing @import rules on document.styleSheets. As before, insertRule() on constructed stylesheets will throw an error, but the error itself will change from NotAllowedError to SyntaxError.

- warnings will now be issued when replace() and replaceSync() encounter an @import rule. Such rules will be ignored, but sheet parsing will continue.

- Due to the release schedule changes for Chromium amid COVID-19, I'm going to delay landing this feature removal until M84. The deprecation message will be updated accordingly.

Thanks,

Mason

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CACuPfeQ6DZ8JhREjiOZdYp1Yp%2Bk65UT5Vnod2HVG5ZYad%3Dy-uQ%40mail.gmail.com.

[Chris Kruining]

I am new here, so I hope reply-all is the right choice :S

I have a question regarding this topic. Currently I am creating a system with adopted stylesheets to inject css into my webcomponents without modifying the DOM.
This is for the CMS I am making, and I want to prevent pollution of my HTML.

This system also is able to load stylesheets via url, this is to load themes. themes have dependencies to other sheets and therefor use @import.

Now that I can fetch and replace into a contructed stylesheet anymore.
What is the 'new' recommended way to add a stylesheet of which I only have an url into the `adoptedstyleheets` property of my shadowDOM?

Thanks in advance!

Sincerly,

Chris Kruining

[Mason Freed]

This system also is able to load stylesheets via url, this is to load themes. themes have dependencies to other sheets and therefor use @import.

Now that I can fetch and replace into a contructed stylesheet anymore.
What is the 'new' recommended way to add a stylesheet of which I only have an url into the `adoptedstyleheets` property of my shadowDOM?

Thanks for the comment/question. Now, with @imports being disallowed in calls to replace(), the "new" recommended way to achieve your goals would be to load each stylesheet URL into its own constructed stylesheet, and add them all to adoptedStyleSheets separately. This does require some refactoring, assuming your existing stylesheets are all linked from a main CSS file that @imports the rest. Now these would all have to be directly linked. Sorry about the inconvenience! The motivation for this change is to be consistent with the eventual spec for Cascading Style Sheet Modules.

Thanks,

Mason

[Chris Kruining]

Hi Mason,

Thanks for your response.

I have solved it with a nasty workaround hack until the css modules are available in browsers.

(the hack is to "simply" recursively replace all `@import` statements with the content of it's url, it's a nasty nasty hack, but functional)

Cheers,

Chris

[Chris Kruining]

I truly hope you guys don't mind me asking, but I'm grasping at straws at this point

Like I mentioned in june I am working on a CMS based on webcomponents. Currently I am encountering an issue (since chrome 85-86, so I suspect it is a bug) that when I use the replace method on a constructed stylesheet which is adopted by a document (iframe with srcdoc and sandbox to be precise) the other constructed sheets seem to either lose their content or to lose their adoption by the document. I do not which of the 2 it is as I can only observe the style getting lost both visually as well as in the inspector.

Since my setup is so complex I wouldn't be surprised if this issue originated from a bug in my code, but the thing worked flawlessly before.

Can anyone here help me? I ask because you guys seem to be on top of  constructed stylesheets.

I assume this to be way off topic, so could I get a pointer to someone who would be willing/able to help me out?

Many many thanks in advance!!!

Cheers,

Chris Kruining

[mason...@google.com]

Without digging into it much further, or with a simplified repro to show the problem, it'll be hard to offer really concrete advice here. One thing you said, however, might be worth looking into. The spec for adoptedStyleSheets says that moving a node to a different document will clear any adoptedStyleSheets on that node. Perhaps that's your issue?

Thanks,

Mason

[Chris Kruining]

I can't seem to be able to reproduce this issue outside of my project. So for now I'll just give up on this.