Bug 1624068 [wpt PR 22374] - Disable @import in replace() and replace…

…Sync(), a=testonly Automatic update from web-platform-tests 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} -- wpt-commits: e797def7c972c5a7e7f6d37d909931d1f34bb6dc wpt-pr: 22374
xeonchen · Apr 13, 2020 · 48cbe9c · 48cbe9c
1 parent 8c9a050
commit 48cbe9c
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 41 deletions.
diff --git a/testing/web-platform/tests/css/cssom/CSSStyleSheet-constructable.html b/testing/web-platform/tests/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");

diff --git a/...sts/html/semantics/scripting-1/the-script-element/css-module/import-css-module-basic.html b/...sts/html/semantics/scripting-1/the-script-element/css-module/import-css-module-basic.html
@@ -22,13 +22,13 @@
             const iframe = document.createElement("iframe");
             iframe.src = "resources/css-module-at-import-iframe.html";
             iframe.onload = test.step_func_done(function () {
-                assert_equals(iframe.contentDocument.load_error, "NotAllowedError");
+                assert_equals(iframe.contentDocument.load_error, undefined);
                 assert_not_equals(getComputedStyle(iframe.contentDocument.querySelector('#test'))
                     .backgroundColor, "rgb(255, 0, 0)",
                     "CSS module @import should not succeed");
             });
             document.body.appendChild(iframe);
-        }, "An @import CSS Module should not load");
+        }, "An @import CSS Module should not load, but should not throw an exception");
 
         async_test(function (test) {
             const iframe = document.createElement("iframe");