From 72146a83dd22ca549def66911c2848573776c87b Mon Sep 17 00:00:00 2001 From: Mason Freed Date: Mon, 6 Apr 2020 18:42:52 -0700 Subject: [PATCH] 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] https://github.com/WICG/construct-stylesheets/issues/119#issuecomment-597733392 [2] https://github.com/WICG/construct-stylesheets/issues/119#issuecomment-600410089 [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 Reviewed-by: Rune Lillesveen Reviewed-by: Rakina Zata Amni Commit-Queue: Mason Freed Cr-Commit-Position: refs/heads/master@{#756894} --- css/cssom/CSSStyleSheet-constructable.html | 105 +++++++++++------- .../css-module/import-css-module-basic.html | 4 +- 2 files changed, 68 insertions(+), 41 deletions(-) diff --git a/css/cssom/CSSStyleSheet-constructable.html b/css/cssom/CSSStyleSheet-constructable.html index fbee4298c14119..e6293909c21789 100644 --- a/css/cssom/CSSStyleSheet-constructable.html +++ b/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/html/semantics/scripting-1/the-script-element/css-module/import-css-module-basic.html b/html/semantics/scripting-1/the-script-element/css-module/import-css-module-basic.html index 902430d0779e3b..1cb290de3feb58 100644 --- a/html/semantics/scripting-1/the-script-element/css-module/import-css-module-basic.html +++ b/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");