From 585c3dd0d5bec1a8ad2d7caefbab6d43199b08e8 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Thu, 25 Mar 2021 18:30:09 +0000 Subject: [PATCH] Bug 1699648 - [devtools] Use the proper principal to fetch the text of imported stylesheets r=nchevobbe Imported stylesheets don't have an ownerNode property, it needs to be retrieved on the parentStylesheet (potentially recursively). Add a helper to resolve the ownerNode and use it retrieve the correct principal to fetch the stylesheet. Add a test case simulating a stylesheet update. Differential Revision: https://phabricator.services.mozilla.com/D109629 --- .../inspector/rules/test/browser_part2.ini | 3 ++ .../browser_rules_imported_stylesheet_edit.js | 46 ++++++++++++++++ .../doc_rules_imported_stylesheet_edit.html | 4 ++ .../test/sjs_imported_stylesheet_edit.sjs | 53 +++++++++++++++++++ .../server/actors/resources/stylesheets.js | 21 +++----- devtools/server/actors/style-sheet.js | 35 ++++++++++-- 6 files changed, 146 insertions(+), 16 deletions(-) create mode 100644 devtools/client/inspector/rules/test/browser_rules_imported_stylesheet_edit.js create mode 100644 devtools/client/inspector/rules/test/doc_rules_imported_stylesheet_edit.html create mode 100644 devtools/client/inspector/rules/test/sjs_imported_stylesheet_edit.sjs diff --git a/devtools/client/inspector/rules/test/browser_part2.ini b/devtools/client/inspector/rules/test/browser_part2.ini index 982a0d52e58c..0f1638b5d467 100644 --- a/devtools/client/inspector/rules/test/browser_part2.ini +++ b/devtools/client/inspector/rules/test/browser_part2.ini @@ -21,6 +21,7 @@ support-files = doc_print_media_simulation.html doc_pseudoelement.html doc_ruleLineNumbers.html + doc_rules_imported_stylesheet_edit.html doc_sourcemaps.css doc_sourcemaps.css.map doc_sourcemaps.html @@ -39,6 +40,7 @@ support-files = doc_visited_in_media_query.html doc_visited_with_style_attribute.html head.js + sjs_imported_stylesheet_edit.sjs !/devtools/client/inspector/test/head.js !/devtools/client/inspector/test/shared-head.js !/devtools/client/shared/test/shared-head.js @@ -83,6 +85,7 @@ skip-if = os == 'linux' # focusEditableField times out consistently on linux. [browser_rules_highlight-element-rule.js] [browser_rules_highlight-property.js] [browser_rules_highlight-used-fonts.js] +[browser_rules_imported_stylesheet_edit.js] [browser_rules_inactive_css_display-justify.js] [browser_rules_inactive_css_flexbox.js] [browser_rules_inactive_css_grid.js] diff --git a/devtools/client/inspector/rules/test/browser_rules_imported_stylesheet_edit.js b/devtools/client/inspector/rules/test/browser_rules_imported_stylesheet_edit.js new file mode 100644 index 000000000000..7b185c07a9e7 --- /dev/null +++ b/devtools/client/inspector/rules/test/browser_rules_imported_stylesheet_edit.js @@ -0,0 +1,46 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ +"use strict"; + +const TEST_URI = URL_ROOT + "doc_rules_imported_stylesheet_edit.html"; +const SJS_URI = URL_ROOT + "sjs_imported_stylesheet_edit.sjs"; +/** + * Test that imported stylesheets are correctly handled by the inspector after + * being updated. + * The inspector used to retrieve an outdated version of the stylesheet text, + * which triggered many issues: outdated values, blank panels etc... + * + * This test involves an imported CSS which is generated by a sjs file. + * Using sjs here allows us to simulate an "update" of a stylesheet while still + * fetching the same URL, which closely matches what a developer would experience + * when manually editing a stylesheet in an IDE before reloading a page. + */ +add_task(async function() { + info("Call `?setup` on the test sjs"); + await fetch(SJS_URI + "?setup"); + + info("Add the test tab, open the rule-view and select the test node"); + await addTab(TEST_URI); + + const { inspector, view } = await openRuleView(); + + await selectNode("div", inspector); + const redColorProp = getTextProperty(view, 1, { color: "red" }); + ok(redColorProp, "RuleView displays a color:red property"); + + // The "?update-stylesheet" call will change the CSS returned by sjs_imported_stylesheet_edit.sjs: + // - some rules are added before the matching `div {}` rule + // - the value of the `color` property changes + info("Call `?update-stylesheet` on the test sjs"); + await fetch(SJS_URI + "?update-stylesheet"); + + info("Reload the page to restore the initial state"); + await navigateTo(TEST_URI); + + info("Wait until a rule is displayed at index 1"); + await waitFor(() => view.element.children[1]); + + info("Check that the displayed rule has been correctly updated."); + const goldColorProp = getTextProperty(view, 1, { color: "gold" }); + ok(goldColorProp, "RuleView displays a color:gold property"); +}); diff --git a/devtools/client/inspector/rules/test/doc_rules_imported_stylesheet_edit.html b/devtools/client/inspector/rules/test/doc_rules_imported_stylesheet_edit.html new file mode 100644 index 000000000000..f1c87fa0abaa --- /dev/null +++ b/devtools/client/inspector/rules/test/doc_rules_imported_stylesheet_edit.html @@ -0,0 +1,4 @@ + +
diff --git a/devtools/client/inspector/rules/test/sjs_imported_stylesheet_edit.sjs b/devtools/client/inspector/rules/test/sjs_imported_stylesheet_edit.sjs new file mode 100644 index 000000000000..5a68c5571c74 --- /dev/null +++ b/devtools/client/inspector/rules/test/sjs_imported_stylesheet_edit.sjs @@ -0,0 +1,53 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at . */ + +"use strict"; +const INITIAL_CONTENT = ` +div { + color: red +} +`; + +const UPDATED_CONTENT = ` +span { + color: green; +} + +a { + color: blue; +} + +div { + color: gold; +} +`; + +/** + * This sjs file supports three endpoint: + * - "sjs_imported_stylesheet_edit.sjs" -> will return a text/css content which + * will be either INITIAL_CONTENT or UPDATED_CONTENT. Initially will return + * INITIAL_CONTENT. + * - "sjs_imported_stylesheet_edit.sjs?update-stylesheet" -> will update an + * internal flag. After calling this URL, the regular endpoint will return + * UPDATED_CONTENT instead of INITIAL_CONTENT + * - "sjs_imported_stylesheet_edit.sjs?setup" -> set the internal flag to its + * default value. Should be called at the beginning of every test to avoid + * side effects. + */ +function handleRequest(request, response) { + const { queryString } = request; + if (queryString === "setup") { + setState("serve-updated-content", "false"); + response.setHeader("Content-Type", "text/html"); + response.write("OK"); + } else if (queryString === "update-stylesheet") { + setState("serve-updated-content", "true"); + response.setHeader("Content-Type", "text/html"); + response.write("OK"); + } else { + response.setHeader("Content-Type", "text/css"); + const shouldServeUpdatedCSS = getState("serve-updated-content") == "true"; + response.write(shouldServeUpdatedCSS ? UPDATED_CONTENT : INITIAL_CONTENT); + } +} diff --git a/devtools/server/actors/resources/stylesheets.js b/devtools/server/actors/resources/stylesheets.js index 894f5aa08a81..f12774c405b6 100644 --- a/devtools/server/actors/resources/stylesheets.js +++ b/devtools/server/actors/resources/stylesheets.js @@ -35,7 +35,7 @@ loader.lazyRequireGetter( ); loader.lazyRequireGetter( this, - ["UPDATE_GENERAL", "UPDATE_PRESERVING_RULES"], + ["getSheetOwnerNode", "UPDATE_GENERAL", "UPDATE_PRESERVING_RULES"], "devtools/server/actors/style-sheet", true ); @@ -342,10 +342,11 @@ class StyleSheetWatcher { const excludedProtocolsRe = /^(chrome|file|resource|moz-extension):\/\//; if (!excludedProtocolsRe.test(href)) { // Stylesheets using other protocols should use the content principal. - if (styleSheet.ownerNode) { + const ownerNode = getSheetOwnerNode(styleSheet); + if (ownerNode) { // eslint-disable-next-line mozilla/use-ownerGlobal - options.window = styleSheet.ownerNode.ownerDocument.defaultView; - options.principal = styleSheet.ownerNode.ownerDocument.nodePrincipal; + options.window = ownerNode.ownerDocument.defaultView; + options.principal = ownerNode.ownerDocument.nodePrincipal; } } @@ -499,18 +500,12 @@ class StyleSheetWatcher { } _getSourcemapBaseURL(styleSheet) { - // When the style is imported, `styleSheet.ownerNode` is null, - // so retrieve the topmost parent style sheet which has an ownerNode - let parentStyleSheet = styleSheet; - while (parentStyleSheet.parentStyleSheet) { - parentStyleSheet = parentStyleSheet.parentStyleSheet; - } - // When the style is injected via nsIDOMWindowUtils.loadSheet, even // the parent style sheet has no owner, so default back to target actor // document - const ownerDocument = parentStyleSheet.ownerNode - ? parentStyleSheet.ownerNode.ownerDocument + const ownerNode = getSheetOwnerNode(styleSheet); + const ownerDocument = ownerNode + ? ownerNode.ownerDocument : this._targetActor.window; return getSourcemapBaseURL( diff --git a/devtools/server/actors/style-sheet.js b/devtools/server/actors/style-sheet.js index 866d4a12946a..5af8828ba6aa 100644 --- a/devtools/server/actors/style-sheet.js +++ b/devtools/server/actors/style-sheet.js @@ -82,6 +82,34 @@ function getSheetText(sheet) { exports.getSheetText = getSheetText; +/** + * For imported stylesheets, `ownerNode` is null. + * To resolve the ownerNode for an imported stylesheet, loop on `parentStylesheet` + * until we reach the topmost stylesheet, which should have a valid ownerNode. + * + * @param {StyleSheet} + * The stylesheet for which we want to retrieve the ownerNode. + * @return {DOMNode} The ownerNode + */ +function getSheetOwnerNode(sheet) { + // If this is not an imported stylesheet and we have an ownerNode available + // bail out immediately. + if (sheet.ownerNode) { + return sheet.ownerNode; + } + + let parentStyleSheet = sheet; + while ( + parentStyleSheet.parentStyleSheet && + parentStyleSheet !== parentStyleSheet.parentStyleSheet + ) { + parentStyleSheet = parentStyleSheet.parentStyleSheet; + } + + return parentStyleSheet.ownerNode; +} +exports.getSheetOwnerNode = getSheetOwnerNode; + /** * Get the charset of the stylesheet. */ @@ -132,10 +160,11 @@ async function fetchStylesheet(sheet) { const excludedProtocolsRe = /^(chrome|file|resource|moz-extension):\/\//; if (!excludedProtocolsRe.test(href)) { // Stylesheets using other protocols should use the content principal. - if (sheet.ownerNode) { + const ownerNode = getSheetOwnerNode(sheet); + if (ownerNode) { // eslint-disable-next-line mozilla/use-ownerGlobal - options.window = sheet.ownerNode.ownerDocument.defaultView; - options.principal = sheet.ownerNode.ownerDocument.nodePrincipal; + options.window = ownerNode.ownerDocument.defaultView; + options.principal = ownerNode.ownerDocument.nodePrincipal; } }