Bug 1512956 - Ensure empty string is considered valid CSS authoredText; r=pbro
☠☠ backed out by d87cc2cc3641 ☠ ☠
authorRazvan Caliman <rcaliman@mozilla.com>
Sun, 24 Feb 2019 10:28:14 +0000
changeset 518669 070cb098260644cbd2cac29deee6ac56d0fd6fb9
parent 518668 acd0d09cec46fd3e9f2cabcbafcaee7f31e179ae
child 518670 d87cc2cc36411fe6e5b2b6eccc210ed19eb88814
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1512956
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1512956 - Ensure empty string is considered valid CSS authoredText; r=pbro When removing all declarations from a rule via the Rule view, the authoredText value ends up as an empty string. This patch ensures that the fallback cssText is not used in that case because that accidentally restores the whole declaration block when re-parsing the text of the rule. Differential Revision: https://phabricator.services.mozilla.com/D14753
devtools/client/inspector/rules/test/browser.ini
devtools/client/inspector/rules/test/browser_rules_edit-property-remove_04.js
devtools/server/actors/styles.js
devtools/shared/css/parsing-utils.js
devtools/shared/fronts/styles.js
--- a/devtools/client/inspector/rules/test/browser.ini
+++ b/devtools/client/inspector/rules/test/browser.ini
@@ -121,16 +121,17 @@ skip-if = (os == "linux") # Bug 1356214
 [browser_rules_edit-property-commit.js]
 [browser_rules_edit-property-computed.js]
 [browser_rules_edit-property-increments.js]
 [browser_rules_edit-property-order.js]
 [browser_rules_edit-property-remove_01.js]
 skip-if = (verify && debug && os == 'win')
 [browser_rules_edit-property-remove_02.js]
 [browser_rules_edit-property-remove_03.js]
+[browser_rules_edit-property-remove_04.js]
 [browser_rules_edit-property_01.js]
 [browser_rules_edit-property_02.js]
 [browser_rules_edit-property_03.js]
 [browser_rules_edit-property_04.js]
 [browser_rules_edit-property_05.js]
 [browser_rules_edit-property_06.js]
 [browser_rules_edit-property_07.js]
 [browser_rules_edit-property_08.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/rules/test/browser_rules_edit-property-remove_04.js
@@ -0,0 +1,46 @@
+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Tests that removing the only declaration from a rule and unselecting then re-selecting
+// the element will not restore the removed declaration. Bug 1512956
+
+const TEST_URI = `
+  <style type='text/css'>
+  #testid {
+    color: #00F;
+  }
+  </style>
+  <div id='testid'>Styled Node</div>
+  <div id='empty'></div>
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const {inspector, view} = await openRuleView();
+
+  info("Select original node");
+  await selectNode("#testid", inspector);
+
+  info("Get the first property in the #testid rule");
+  const rule = getRuleViewRuleEditor(view, 1).rule;
+  const prop = rule.textProps[0];
+
+  info("Delete the property name to remove the declaration");
+  const onRuleViewChanged = view.once("ruleview-changed");
+  await removeProperty(view, prop, false);
+  info("Wait for Rule view to update");
+  await onRuleViewChanged;
+
+  is(rule.textProps.length, 0, "No CSS properties left on the rule");
+
+  info("Select another node");
+  await selectNode("#empty", inspector);
+
+  info("Select original node again");
+  await selectNode("#testid", inspector);
+
+  is(rule.textProps.length, 0, "Still no CSS properties on the rule");
+});
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -1260,19 +1260,21 @@ var StyleRuleActor = protocol.ActorClass
         form.keyText = this.rawRule.keyText || "";
         break;
     }
 
     // Parse the text into a list of declarations so the client doesn't have to
     // and so that we can safely determine if a declaration is valid rather than
     // have the client guess it.
     if (form.authoredText || form.cssText) {
-      const declarations = parseNamedDeclarations(isCssPropertyKnown,
-                                                form.authoredText || form.cssText,
-                                                true);
+      // authoredText may be an empty string when deleting all properties; it's ok to use.
+      const cssText = (typeof form.authoredText === "string")
+        ? form.authoredText
+        : form.cssText;
+      const declarations = parseNamedDeclarations(isCssPropertyKnown, cssText, true);
 
       // We need to grab CSS from the window, since calling supports() on the
       // one from the current global will fail due to not being an HTML global.
       const CSS = this.pageStyle.inspector.targetActor.window.CSS;
       form.declarations = declarations.map(decl => {
         // Use the 1-arg CSS.supports() call so that we also accept !important
         // in the value.
         decl.isValid = CSS.supports(`${decl.name}:${decl.value}`);
--- a/devtools/shared/css/parsing-utils.js
+++ b/devtools/shared/css/parsing-utils.js
@@ -265,17 +265,17 @@ function cssTrim(str) {
  *        This only makes sense when inComment=true.
  *        When true, assume that the comment was generated by
  *        rewriteDeclarations, and skip the usual name-checking
  *        heuristic.
  */
 function parseDeclarationsInternal(isCssPropertyKnown, inputString,
                                    parseComments, inComment, commentOverride) {
   if (inputString === null || inputString === undefined) {
-    throw new Error("empty input string");
+    return [];
   }
 
   const lexer = getCSSLexer(inputString);
 
   let declarations = [getEmptyDeclaration()];
   let lastProp = declarations[0];
 
   // This tracks the "!important" parsing state.  The states are:
--- a/devtools/shared/fronts/styles.js
+++ b/devtools/shared/fronts/styles.js
@@ -128,17 +128,19 @@ class StyleRuleFront extends FrontClassW
   }
   get column() {
     return this._form.column || -1;
   }
   get cssText() {
     return this._form.cssText;
   }
   get authoredText() {
-    return this._form.authoredText || this._form.cssText;
+    return (typeof this._form.authoredText === "string")
+      ? this._form.authoredText
+      : this._form.cssText;
   }
   get declarations() {
     return this._form.declarations || [];
   }
   get keyText() {
     return this._form.keyText;
   }
   get name() {