Disable @import in replace() and replaceSync()

The CSSWG resolved [1] that "@import doesn't parse in constructable
stylesheets and as a result throw syntax errors". We have a comment [2]
requesting clarification, but the working assumption is:

- calling replace() with text that includes @import will ignore the
  @import, and issue a console warning.
- calling replaceSync() with text that includes @import will ignore
  the @import, and issue a console warning.
- calling insertRule() on a constructed stylesheet with an @import
  rule will throw a SyntaxError. (no change here)

*Prior* to this CL, 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() on a constructed stylesheet with an @import
  rule will throw a SyntaxError.

The Intent to Remove for this change is at [3].

[1] WICG/construct-stylesheets#119 (comment)
[2] WICG/construct-stylesheets#119 (comment)
[3] https://groups.google.com/a/chromium.org/g/blink-dev/c/RKG8oxp22RY/m/fdFnG1rGCgAJ

Bug: 1055943
Change-Id: I0a8444289b137b4c2880be5231696bb526331107
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106996
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756894}

third_party/blink/renderer/core/css/css_style_sheet.cc

@@ -40,6 +40,7 @@
 #include "third_party/blink/renderer/core/html/html_link_element.h"
 #include "third_party/blink/renderer/core/html/html_style_element.h"
 #include "third_party/blink/renderer/core/html_names.h"
+#include "third_party/blink/renderer/core/inspector/console_message.h"
 #include "third_party/blink/renderer/core/probe/core_probes.h"
 #include "third_party/blink/renderer/core/svg/svg_style_element.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
@@ -358,8 +359,8 @@ unsigned CSSStyleSheet::insertRule(const String& rule_string,
   RuleMutationScope mutation_scope(this);
   if (rule->IsImportRule() && is_constructed_) {
     exception_state.ThrowDOMException(
-        DOMExceptionCode::kNotAllowedError,
-        "Can't insert @import rules to a constructed stylesheet.");
+        DOMExceptionCode::kSyntaxError,
+        "Can't insert @import rules into a constructed stylesheet.");
     return 0;
   }
   bool success = contents_->WrapperInsertRule(rule, index);
@@ -460,15 +461,11 @@ ScriptPromise CSSStyleSheet::replace(ScriptState* script_state,
             DOMExceptionCode::kNotAllowedError,
             "Can't call replace on non-constructed CSSStyleSheets."));
   }
-  // Parses the text synchronously, loads import rules asynchronously.
-  SetText(text, true /* allow_import_rules */, nullptr);
-  if (!IsLoading())
-    return ScriptPromise::Cast(script_state, ToV8(this, script_state));
-  // We're loading a stylesheet that contains @import rules. This is deprecated.
-  Deprecation::CountDeprecation(OwnerDocument(),
-                                WebFeature::kCssStyleSheetReplaceWithImport);
-  resolver_ = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
-  return resolver_->Promise();
+  SetText(text, CSSImportRules::kIgnoreWithWarning);
+  // We currently parse synchronously, and since @import support was removed,
+  // nothing else happens asynchronously. This API is left as-is, so that future
+  // async parsing can still be supported here.
+  return ScriptPromise::Cast(script_state, ToV8(this, script_state));
 }
 
 void CSSStyleSheet::replaceSync(const String& text,
@@ -478,7 +475,7 @@ void CSSStyleSheet::replaceSync(const String& text,
         DOMExceptionCode::kNotAllowedError,
         "Can't call replaceSync on non-constructed CSSStyleSheets.");
   }
-  SetText(text, false /* allow_import_rules */, &exception_state);
+  SetText(text, CSSImportRules::kIgnoreWithWarning);
 }
 
 void CSSStyleSheet::ResolveReplacePromiseIfNeeded(bool load_error_occured) {
@@ -569,19 +566,21 @@ void CSSStyleSheet::SetLoadCompleted(bool completed) {
     contents_->ClientLoadStarted(this);
 }
 
-void CSSStyleSheet::SetText(const String& text,
-                            bool allow_import_rules,
-                            ExceptionState* exception_state) {
+void CSSStyleSheet::SetText(const String& text, CSSImportRules import_rules) {
   child_rule_cssom_wrappers_.clear();
 
   CSSStyleSheet::RuleMutationScope mutation_scope(this);
   contents_->ClearRules();
-  if (contents_->ParseString(text, allow_import_rules) ==
-      ParseSheetResult::kHasUnallowedImportRule) {
-    DCHECK(exception_state);
-    exception_state->ThrowDOMException(DOMExceptionCode::kNotAllowedError,
-                                       "@import rules are not allowed when "
-                                       "creating stylesheet synchronously.");
+  bool allow_imports = import_rules == CSSImportRules::kAllow;
+  if (contents_->ParseString(text, allow_imports) ==
+          ParseSheetResult::kHasUnallowedImportRule &&
+      import_rules == CSSImportRules::kIgnoreWithWarning) {
+    OwnerDocument()->AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
+        mojom::blink::ConsoleMessageSource::kJavaScript,
+        mojom::blink::ConsoleMessageLevel::kWarning,
+        "@import rules are not allowed here. See "
+        "https://github.com/WICG/construct-stylesheets/issues/"
+        "119#issuecomment-588352418."));
   }
 }
 

third_party/blink/renderer/core/css/css_style_sheet.h

@@ -48,6 +48,11 @@ class ScriptPromiseResolver;
 class ScriptState;
 class StyleSheetContents;
 
+enum class CSSImportRules {
+  kAllow,
+  kIgnoreWithWarning,
+};
+
 class CORE_EXPORT CSSStyleSheet final : public StyleSheet {
   DEFINE_WRAPPERTYPEINFO();
 
@@ -189,9 +194,7 @@ class CORE_EXPORT CSSStyleSheet final : public StyleSheet {
   bool SheetLoaded();
   bool LoadCompleted() const { return load_completed_; }
   void StartLoadingDynamicSheet();
-  // If `allow_import_rules` is false, an ExceptionState pointer must be
-  // provided, otherwise it can be null.
-  void SetText(const String&, bool allow_import_rules, ExceptionState*);
+  void SetText(const String&, CSSImportRules);
   void SetMedia(MediaList*);
   void SetAlternateFromConstructor(bool);
   bool CanBeActivated(const String& current_preferrable_name) const;

third_party/blink/renderer/core/inspector/inspector_style_sheet.cc

@@ -997,7 +997,7 @@ String InspectorStyleSheet::FinalURL() {
 bool InspectorStyleSheet::SetText(const String& text,
                                   ExceptionState& exception_state) {
   InnerSetText(text, true);
-  page_style_sheet_->SetText(text, true /* allow_import_rules */, nullptr);
+  page_style_sheet_->SetText(text, CSSImportRules::kAllow);
   OnStyleSheetTextChanged();
   return true;
 }

third_party/blink/web_tests/external/wpt/css/cssom/CSSStyleSheet-constructable-expected.txt

@@ -1,8 +1,8 @@
 This is a testharness.js-based test.
 PASS document.adoptedStyleSheets should initially have length 0.
-FAIL new CSSStyleSheet produces empty CSSStyleSheet assert_equals: expected (object) null but got (string) ""
-FAIL title cannot be set in the CSSStyleSheet constructor assert_equals: expected (object) null but got (string) "something"
-FAIL CSSStyleSheet.replace produces Promise<CSSStyleSheet> assert_equals: expected (object) null but got (string) ""
+FAIL new CSSStyleSheet produces empty CSSStyleSheet assert_equals: The title attribute must return the title or null if title is the empty string expected (object) null but got (string) ""
+FAIL title can be set in the CSSStyleSheet constructor assert_equals: title and alternate are not supported by the constructor. https://github.com/WICG/construct-stylesheets/issues/105 expected (object) null but got (string) "something"
+FAIL CSSStyleSheet.replace produces Promise<CSSStyleSheet> assert_equals: The title attribute must return the title or null if title is the empty string expected (object) null but got (string) ""
 PASS Constructed style sheets can be applied on document
 PASS Constructed style sheets can be applied on shadow root
 PASS Re-attaching shadow host with adopted stylesheets work
@@ -16,13 +16,14 @@ PASS Adding non-constructed stylesheet to AdoptedStyleSheets is not allowed when
 PASS Adding non-constructed stylesheet to AdoptedStyleSheets is not allowed when the owner document of the stylesheet and the AdoptedStyleSheets are in different document trees
 PASS CSSStyleSheet.replaceSync replaces stylesheet text synchronously
 PASS CSSStyleSheet.replaceSync correctly updates the style of its adopters synchronously
-FAIL Adopted sheets are ordered after non-adopted sheets in the shadow root assert_equals: expected "rgb(255, 255, 255)" but got "rgb(255, 0, 0)"
-FAIL Adopted sheets are ordered after non-adopted sheets in the document assert_equals: expected "rgb(255, 255, 255)" but got "rgb(255, 0, 0)"
-PASS CSSStyleSheet.replaceSync throws exception when there is import rule inside
+FAIL Adopted sheets are ordered after non-adopted sheets in the shadow root assert_equals: with the adopted sheet disabled, the non-adopted style should take effect expected "rgb(255, 255, 255)" but got "rgb(255, 0, 0)"
+FAIL Adopted sheets are ordered after non-adopted sheets in the document assert_equals: with the adopted sheet disabled, the non-adopted style should take effect expected "rgb(255, 255, 255)" but got "rgb(255, 0, 0)"
 PASS Inserting an @import rule through insertRule on a constructed stylesheet throws an exception
 PASS CSSStyleSheet.replaceSync should not trigger any loads from @import rules
-PASS CSSStyleSheet.replace allows import rule inside
-FAIL CSSStyleSheet.replace returns rejected promise on failed imports assert_equals: expected "NetworkError" but got "NotAllowedError"
+PASS CSSStyleSheet.replace allows, but ignores, import rule inside
+PASS CSSStyleSheet.replace ignores @import rule but still loads other rules
+PASS CSSStyleSheet.replaceSync allows, but ignores, import rule inside
+PASS CSSStyleSheet.replace does not reject on failed imports
 PASS Cloning a shadow host will not clone shadow root, and also adoptedStyleSheets
 PASS Importing a shadow host will not copy shadow root, and also adoptedStyleSheets
 PASS Adopting a shadow host will empty adoptedStyleSheets if adopting to a different document

third_party/blink/web_tests/external/wpt/css/cssom/CSSStyleSheet-constructable.html

@@ -48,7 +48,7 @@
 
 test(() => {
   const sheet = new CSSStyleSheet({disabled: true, media: "screen, print"});
-  assert_equals(sheet.title, null);
+  assert_equals(sheet.title, null, "The title attribute must return the title or null if title is the empty string");
   assert_equals(sheet.ownerNode, null);
   assert_equals(sheet.ownerRule, null);
   assert_equals(sheet.media.length, 2);
@@ -66,7 +66,7 @@
   assert_equals(sheet.cssRules[0].cssText, redStyleTexts[1]);
 
   const sheet2 = new CSSStyleSheet({});
-  assert_equals(sheet2.title, null);
+  assert_equals(sheet2.title, null, "The title attribute must return the title or null if title is the empty string");
   assert_equals(sheet2.ownerNode, null);
   assert_equals(sheet2.ownerRule, null);
   assert_equals(sheet2.media.length, 0);
@@ -81,7 +81,7 @@
   assert_equals(sheet2.cssRules.length, 0);
 
   const sheet3 = new CSSStyleSheet();
-  assert_equals(sheet3.title, null);
+  assert_equals(sheet3.title, null, "The title attribute must return the title or null if title is the empty string");
   assert_equals(sheet3.ownerNode, null);
   assert_equals(sheet3.ownerRule, null);
   assert_equals(sheet3.media.length, 0);
@@ -98,14 +98,14 @@
 
 test(() => {
   const sheet = new CSSStyleSheet({title: "something"});
-  assert_equals(sheet.title, null);
-}, "title cannot be set in the CSSStyleSheet constructor");
+  assert_equals(sheet.title, null, "title and alternate are not supported by the constructor. https://github.com/WICG/construct-stylesheets/issues/105");
+}, "title can be set in the CSSStyleSheet constructor");
 
 promise_test(() => {
   const sheet = new CSSStyleSheet({disabled: true, media: "screen, print"});
   const promise_sheet = sheet.replace(redStyleTexts[0]);
   return promise_sheet.then(function(sheet) {
-    assert_equals(sheet.title, null);
+    assert_equals(sheet.title, null, "The title attribute must return the title or null if title is the empty string");
     assert_equals(sheet.ownerNode, null);
     assert_equals(sheet.ownerRule, null);
     assert_equals(sheet.media.length, 2);
@@ -535,24 +535,19 @@
   const style = document.createElement("style");
   style.textContent = ".target { color: white; }";
   span.shadowRoot.appendChild(style)
-  // non-adopted styles should be ordered before adopted styles
-  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 0, 0)");
+  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 0, 0)", "non-adopted styles should be ordered before adopted styles");
 
   span.shadowRoot.adoptedStyleSheets = [];
-  // with no adopted styles in conflict, the non-adopted style should take effect
-  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 255, 255)");
+  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 255, 255)", "with no adopted styles in conflict, the non-adopted style should take effect");
 
   span.shadowRoot.adoptedStyleSheets = [sheet];
-  // the adopted style should be ordered after the non-adopted style
-  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 0, 0)");
+  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 0, 0)", "the adopted style should be ordered after the non-adopted style");
 
   sheet.disabled = true;
-  // with the adopted sheet disabled, the non-adopted style should take effect
-  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 255, 255)");
+  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 255, 255)", "with the adopted sheet disabled, the non-adopted style should take effect");
 
   sheet.disabled = false;
-  // the adopted sheet re-enabled, it should take effect again.
-  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 0, 0)");
+  assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 0, 0)", "the adopted sheet re-enabled, it should take effect again");
 }, 'Adopted sheets are ordered after non-adopted sheets in the shadow root')
 
 test(() => {
@@ -574,41 +569,32 @@
   const style = document.createElement("style");
   style.textContent = ".target { color: white; }";
   span.appendChild(style)
-  // non-adopted styles should be ordered before adopted styles
-  assert_equals(getComputedStyle(span).color, "rgb(255, 0, 0)");
+  assert_equals(getComputedStyle(span).color, "rgb(255, 0, 0)", "non-adopted styles should be ordered before adopted styles");
 
   document.adoptedStyleSheets = [];
-  // with no adopted styles in conflict, the non-adopted style should take effect
-  assert_equals(getComputedStyle(span).color, "rgb(255, 255, 255)");
+  assert_equals(getComputedStyle(span).color, "rgb(255, 255, 255)", "with no adopted styles in conflict, the non-adopted style should take effect");
 
   document.adoptedStyleSheets = [sheet];
-  // the adopted style should be ordered after the non-adopted style
-  assert_equals(getComputedStyle(span).color, "rgb(255, 0, 0)");
+  assert_equals(getComputedStyle(span).color, "rgb(255, 0, 0)", "the adopted style should be ordered after the non-adopted style");
 
   sheet.disabled = true;
-  // with the adopted sheet disabled, the non-adopted style should take effect
-  assert_equals(getComputedStyle(span).color, "rgb(255, 255, 255)");
+  assert_equals(getComputedStyle(span).color, "rgb(255, 255, 255)", "with the adopted sheet disabled, the non-adopted style should take effect");
 
   sheet.disabled = false;
-  // the adopted sheet re-enabled, it should take effect again.
-  assert_equals(getComputedStyle(span).color, "rgb(255, 0, 0)")
+  assert_equals(getComputedStyle(span).color, "rgb(255, 0, 0)", "the adopted sheet re-enabled, it should take effect again")
 }, 'Adopted sheets are ordered after non-adopted sheets in the document')
 
 const import_text = '@import url("support/constructable-import.css");';
 
 test(() => {
-  assert_throws_dom("NotAllowedError", () => { (new CSSStyleSheet).replaceSync(import_text) });
-}, 'CSSStyleSheet.replaceSync throws exception when there is import rule inside');
-
-test(() => {
-  assert_throws_dom("NotAllowedError", () => { (new CSSStyleSheet).insertRule(import_text) });
+  assert_throws_dom("SyntaxError", () => { (new CSSStyleSheet).insertRule(import_text) });
 }, 'Inserting an @import rule through insertRule on a constructed stylesheet throws an exception');
 
 async_test(t => {
     const importUrl = "support/constructable-import.css";
     const sheet = new CSSStyleSheet();
 
-    assert_throws_dom("NotAllowedError", () => { sheet.replaceSync(`@import url("${importUrl}");`) });
+    sheet.replaceSync(`@import url("${importUrl}");`);
 
     const timeAfterReplaceSync = performance.now();
     let link = document.createElement("link");
@@ -632,27 +618,68 @@
   const sheet = new CSSStyleSheet();
   span.shadowRoot.adoptedStyleSheets = [sheet];
   assert_equals(getComputedStyle(shadowDiv).color, "rgb(0, 0, 0)");
-  // Replace and assert that the imported rule is applied.
+  // Replace and assert that the imported rule is NOT applied.
   const sheet_promise = sheet.replace(import_text);
+  return sheet_promise.then((sheet) => {
+    // replace() ignores @import rules:
+    assert_equals(sheet.cssRules.length, 0);
+    assert_equals(getComputedStyle(shadowDiv).color, "rgb(0, 0, 0)");
+  }).catch((reason) => {
+    assert_unreached(`Promise was rejected (${reason}) when it should have been resolved`);
+  });
+}, 'CSSStyleSheet.replace allows, but ignores, import rule inside');
+
+promise_test(() => {
+  const span = document.createElement("span");
+  thirdSection.appendChild(span);
+  const shadowDiv = attachShadowDiv(span);
+  const targetSpan = document.createElement("span");
+  targetSpan.classList.add("target");
+  shadowDiv.appendChild(targetSpan);
+  const sheet = new CSSStyleSheet();
+  span.shadowRoot.adoptedStyleSheets = [sheet];
+  assert_equals(getComputedStyle(shadowDiv).color, "rgb(0, 0, 0)");
+  // Replace and assert that the imported rule is NOT applied, but regular rule does apply.
+  const sheet_promise = sheet.replace(import_text + ".target { color: blue; }");
   return sheet_promise.then((sheet) => {
     assert_equals(sheet.cssRules.length, 1);
-    assert_equals(sheet.cssRules[0].cssText, import_text);
-    assert_equals(getComputedStyle(shadowDiv).color, "rgb(255, 0, 0)");
+    // @import not applied:
+    assert_equals(getComputedStyle(shadowDiv).color, "rgb(0, 0, 0)");
+    // regular rule applied:
+    assert_equals(getComputedStyle(targetSpan).color, "rgb(0, 0, 255)");
   }).catch((reason) => {
     assert_unreached(`Promise was rejected (${reason}) when it should have been resolved`);
   });
-}, 'CSSStyleSheet.replace allows import rule inside');
+}, 'CSSStyleSheet.replace ignores @import rule but still loads other rules');
+
+test(() => {
+  const span = document.createElement("span");
+  thirdSection.appendChild(span);
+  const shadowDiv = attachShadowDiv(span);
+  const sheet = new CSSStyleSheet();
+  span.shadowRoot.adoptedStyleSheets = [sheet];
+  assert_equals(getComputedStyle(shadowDiv).color, "rgb(0, 0, 0)");
+  // Replace and assert that the imported rule is NOT applied.
+  try {
+    sheet.replaceSync(import_text);
+    // replaceSync() ignores @import rules:
+    assert_equals(sheet.cssRules.length, 0);
+    assert_equals(getComputedStyle(shadowDiv).color, "rgb(0, 0, 0)");
+  } catch(reason) {
+    assert_unreached(`replaceSync threw an exception (${reason}) when it shouldn't have`);
+  }
+}, 'CSSStyleSheet.replaceSync allows, but ignores, import rule inside');
 
 promise_test(() => {
   const sheet = new CSSStyleSheet();
   const sheet_promise = sheet.replace("@import url('not-there.css');");
 
   return sheet_promise.then((sheet) => {
-    assert_unreached("Promise was resolved when it should have been rejected");
+    // No exception here
   }).catch((reason) => {
-    assert_equals(reason.name, "NetworkError");
+    assert_unreached("Promise was rejected");
   });
-}, 'CSSStyleSheet.replace returns rejected promise on failed imports');
+}, 'CSSStyleSheet.replace does not reject on failed imports');
 
 test(() => {
   const span = document.createElement("span");