From 30f5d120ffd6bbcaafa589d5f7581b0c5b52972e Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Wed, 7 Sep 2016 11:08:12 -0600 Subject: [PATCH] Bug 1301078 - correctly disable pasted commented-out properties; r=pbro MozReview-Commit-ID: 97ueaqvYVVN --HG-- extra : rebase_source : 253428be10e9664693d0818deffbc983ef4c8af0 --- .../client/inspector/rules/models/rule.js | 2 +- ...browser_rules_add-property-and-reselect.js | 2 +- .../browser_rules_add-rule-and-property.js | 2 +- .../test/browser_rules_add-rule-iframes.js | 2 +- ...es_add-rule-then-property-edit-selector.js | 2 +- .../test/browser_rules_keyframes-rule_02.js | 2 +- .../test/browser_rules_pseudo-element_01.js | 6 +- ...er_rules_refresh-on-attribute-change_02.js | 2 +- .../test/unit/test_rewriteDeclarations.js | 64 ++++++++++++------- devtools/client/styleeditor/test/browser.ini | 1 + .../browser_styleeditor_syncAddProperty.js | 45 +++++++++++++ devtools/shared/css-parsing-utils.js | 27 +++++--- devtools/shared/fronts/styles.js | 4 +- 13 files changed, 117 insertions(+), 44 deletions(-) create mode 100644 devtools/client/styleeditor/test/browser_styleeditor_syncAddProperty.js diff --git a/devtools/client/inspector/rules/models/rule.js b/devtools/client/inspector/rules/models/rule.js index 98b08010c5f1..6bcb23f4a99f 100644 --- a/devtools/client/inspector/rules/models/rule.js +++ b/devtools/client/inspector/rules/models/rule.js @@ -189,7 +189,7 @@ Rule.prototype = { } this.applyProperties((modifications) => { - modifications.createProperty(ind, name, value, priority); + modifications.createProperty(ind, name, value, priority, enabled); // Now that the rule has been updated, the server might have given us data // that changes the state of the property. Update it now. prop.updateEditor(); diff --git a/devtools/client/inspector/rules/test/browser_rules_add-property-and-reselect.js b/devtools/client/inspector/rules/test/browser_rules_add-property-and-reselect.js index db1f218feb29..492739abe4b5 100644 --- a/devtools/client/inspector/rules/test/browser_rules_add-property-and-reselect.js +++ b/devtools/client/inspector/rules/test/browser_rules_add-property-and-reselect.js @@ -29,7 +29,7 @@ function* setPropertyOnAllRules(view) { // view to be updated. let onRefreshed = view.once("ruleview-refreshed"); for (let rule of view._elementStyle.rules) { - rule.editor.addProperty("font-weight", "bold", ""); + rule.editor.addProperty("font-weight", "bold", "", true); } yield onRefreshed; } diff --git a/devtools/client/inspector/rules/test/browser_rules_add-rule-and-property.js b/devtools/client/inspector/rules/test/browser_rules_add-rule-and-property.js index 6eec376d9a67..b73fc76d464c 100644 --- a/devtools/client/inspector/rules/test/browser_rules_add-rule-and-property.js +++ b/devtools/client/inspector/rules/test/browser_rules_add-rule-and-property.js @@ -20,7 +20,7 @@ add_task(function* () { let ruleEditor = getRuleViewRuleEditor(view, 1); let onRuleViewChanged = view.once("ruleview-changed"); - ruleEditor.addProperty("font-weight", "bold", ""); + ruleEditor.addProperty("font-weight", "bold", "", true); yield onRuleViewChanged; let textProps = ruleEditor.rule.textProps; diff --git a/devtools/client/inspector/rules/test/browser_rules_add-rule-iframes.js b/devtools/client/inspector/rules/test/browser_rules_add-rule-iframes.js index 2ca22daee6d7..7b0ba7812829 100644 --- a/devtools/client/inspector/rules/test/browser_rules_add-rule-iframes.js +++ b/devtools/client/inspector/rules/test/browser_rules_add-rule-iframes.js @@ -47,7 +47,7 @@ function* addNewProperty(view, index, name, value) { info(`Adding new property "${name}: ${value};"`); let onRuleViewChanged = view.once("ruleview-changed"); - idRuleEditor.addProperty(name, value, ""); + idRuleEditor.addProperty(name, value, "", true); yield onRuleViewChanged; let textProps = idRuleEditor.rule.textProps; diff --git a/devtools/client/inspector/rules/test/browser_rules_add-rule-then-property-edit-selector.js b/devtools/client/inspector/rules/test/browser_rules_add-rule-then-property-edit-selector.js index 4375320318f9..294eb67e43f9 100644 --- a/devtools/client/inspector/rules/test/browser_rules_add-rule-then-property-edit-selector.js +++ b/devtools/client/inspector/rules/test/browser_rules_add-rule-then-property-edit-selector.js @@ -39,7 +39,7 @@ add_task(function* () { function* testAddingProperty(view, index) { let ruleEditor = getRuleViewRuleEditor(view, index); - ruleEditor.addProperty("font-weight", "bold", ""); + ruleEditor.addProperty("font-weight", "bold", "", true); let textProps = ruleEditor.rule.textProps; let lastRule = textProps[textProps.length - 1]; is(lastRule.name, "font-weight", "Last rule name is font-weight"); diff --git a/devtools/client/inspector/rules/test/browser_rules_keyframes-rule_02.js b/devtools/client/inspector/rules/test/browser_rules_keyframes-rule_02.js index cb5bb0a6a6cd..b7652ecaabc4 100644 --- a/devtools/client/inspector/rules/test/browser_rules_keyframes-rule_02.js +++ b/devtools/client/inspector/rules/test/browser_rules_keyframes-rule_02.js @@ -36,7 +36,7 @@ function* testPacman(inspector, view) { // let defaultView = element.ownerDocument.defaultView; // let ruleEditor = view.element.children[5].childNodes[0]._ruleEditor; - // ruleEditor.addProperty("opacity", "0"); + // ruleEditor.addProperty("opacity", "0", true); // yield ruleEditor._applyingModifications; // yield once(element, "animationend"); diff --git a/devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js b/devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js index 28f4dd4e895d..e98b5437c7cd 100644 --- a/devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js +++ b/devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js @@ -70,12 +70,12 @@ function* testTopLeft(inspector, view) { let onAdded = view.once("ruleview-changed"); let firstProp = elementFirstLineRuleView.addProperty("background-color", - "rgb(0, 255, 0)", ""); + "rgb(0, 255, 0)", "", true); yield onAdded; onAdded = view.once("ruleview-changed"); let secondProp = elementFirstLineRuleView.addProperty("font-style", - "italic", ""); + "italic", "", true); yield onAdded; is(firstProp, @@ -108,7 +108,7 @@ function* testTopLeft(inspector, view) { onAdded = view.once("ruleview-changed"); firstProp = elementRuleView.addProperty("background-color", - "rgb(0, 0, 255)", ""); + "rgb(0, 0, 255)", "", true); yield onAdded; is((yield getComputedStyleProperty(id, null, "background-color")), diff --git a/devtools/client/inspector/rules/test/browser_rules_refresh-on-attribute-change_02.js b/devtools/client/inspector/rules/test/browser_rules_refresh-on-attribute-change_02.js index ec48d39bf6a5..6ee385faad25 100644 --- a/devtools/client/inspector/rules/test/browser_rules_refresh-on-attribute-change_02.js +++ b/devtools/client/inspector/rules/test/browser_rules_refresh-on-attribute-change_02.js @@ -32,7 +32,7 @@ function* testPropertyChanges(inspector, ruleView) { info("Adding a second margin-top value in the element selector"); let ruleEditor = ruleView._elementStyle.rules[0].editor; let onRefreshed = inspector.once("rule-view-refreshed"); - ruleEditor.addProperty("margin-top", "5px", ""); + ruleEditor.addProperty("margin-top", "5px", "", true); yield onRefreshed; let rule = ruleView._elementStyle.rules[0]; diff --git a/devtools/client/shared/test/unit/test_rewriteDeclarations.js b/devtools/client/shared/test/unit/test_rewriteDeclarations.js index 3248d666a36c..616402a90393 100644 --- a/devtools/client/shared/test/unit/test_rewriteDeclarations.js +++ b/devtools/client/shared/test/unit/test_rewriteDeclarations.js @@ -64,14 +64,14 @@ const TEST_DATA = [ desc: "simple create", input: "", instruction: {type: "create", name: "p", value: "v", priority: "important", - index: 0}, + index: 0, enabled: true}, expected: "p: v !important;" }, { desc: "create between two properties", input: "a:b; e: f;", instruction: {type: "create", name: "c", value: "d", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "a:b; c: d;e: f;" }, // "create" is passed the name that the user entered, and must do @@ -80,7 +80,7 @@ const TEST_DATA = [ desc: "create requiring escape", input: "", instruction: {type: "create", name: "a b", value: "d", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "a\\ b: d;" }, { @@ -137,7 +137,7 @@ const TEST_DATA = [ // Note the lack of a trailing semicolon. input: "color: red", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "color: red;a: b;" }, @@ -146,7 +146,7 @@ const TEST_DATA = [ desc: "simple newline insertion", input: "\ncolor: red;\n", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "\ncolor: red;\na: b;\n" }, // Newline insertion. @@ -155,7 +155,7 @@ const TEST_DATA = [ // Note the lack of a trailing semicolon. input: "\ncolor: red\n", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "\ncolor: red;\na: b;\n" }, // Newline insertion. @@ -164,7 +164,7 @@ const TEST_DATA = [ // Note the lack of a trailing semicolon and newline. input: "\ncolor: red", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "\ncolor: red;\na: b;\n" }, @@ -173,7 +173,7 @@ const TEST_DATA = [ desc: "indentation with create", input: "\n color: red;\n", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "\n color: red;\n a: b;\n" }, // Newline insertion and indentation. @@ -182,7 +182,7 @@ const TEST_DATA = [ // Note the lack of a trailing semicolon. input: "\n color: red\n", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "\n color: red;\n a: b;\n" }, { @@ -198,7 +198,7 @@ const TEST_DATA = [ // the indentation of the "}". input: "\n color: red;\n ", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "\n color: red;\n a: b;\n " }, // Newline insertion and indentation. @@ -207,7 +207,7 @@ const TEST_DATA = [ // Note how the comment comes before the declaration. input: "\n /* comment */ color: red\n", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "\n /* comment */ color: red;\n a: b;\n" }, // Default indentation. @@ -215,7 +215,7 @@ const TEST_DATA = [ desc: "use of default indentation", input: "\n", instruction: {type: "create", name: "a", value: "b", priority: "", - index: 0}, + index: 0, enabled: true}, expected: "\n\ta: b;\n" }, @@ -275,7 +275,7 @@ const TEST_DATA = [ desc: "create single quote termination", input: "content: 'hi", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "content: 'hi';color: red;", changed: {0: "'hi'"} }, @@ -293,7 +293,7 @@ const TEST_DATA = [ desc: "create double quote termination", input: "content: \"hi", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "content: \"hi\";color: red;", changed: {0: "\"hi\""} }, @@ -312,7 +312,7 @@ const TEST_DATA = [ desc: "create url termination", input: "background-image: url(something.jpg", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "background-image: url(something.jpg);color: red;", changed: {0: "url(something.jpg)"} }, @@ -331,7 +331,7 @@ const TEST_DATA = [ desc: "create url single quote termination", input: "background-image: url('something.jpg", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "background-image: url('something.jpg');color: red;", changed: {0: "url('something.jpg')"} }, @@ -350,7 +350,7 @@ const TEST_DATA = [ desc: "enable url double quote termination", input: "background-image: url(\"something.jpg", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "background-image: url(\"something.jpg\");color: red;", changed: {0: "url(\"something.jpg\")"} }, @@ -360,7 +360,7 @@ const TEST_DATA = [ desc: "create backslash termination", input: "something: \\", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "something: \\\\;color: red;", // The lexer rewrites the token before we see it. However this is // so obscure as to be inconsequential. @@ -372,7 +372,7 @@ const TEST_DATA = [ desc: "enable backslash single quote termination", input: "something: '\\", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "something: '\\\\';color: red;", changed: {0: "'\\\\'"} }, @@ -380,7 +380,7 @@ const TEST_DATA = [ desc: "enable backslash double quote termination", input: "something: \"\\", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "something: \"\\\\\";color: red;", changed: {0: "\"\\\\\""} }, @@ -390,7 +390,7 @@ const TEST_DATA = [ desc: "enable comment termination", input: "something: blah /* comment ", instruction: {type: "create", name: "color", value: "red", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "something: blah /* comment*/; color: red;" }, @@ -407,7 +407,7 @@ const TEST_DATA = [ desc: "create sanitize unpaired brace", input: "", instruction: {type: "create", name: "p", value: "}", priority: "", - index: 0}, + index: 0, enabled: true}, expected: "p: \\};", changed: {0: "\\}"} }, @@ -435,10 +435,25 @@ const TEST_DATA = [ desc: "disabled declaration does not need semicolon insertion", input: "/*! no: semicolon */\n", instruction: {type: "create", name: "walrus", value: "zebra", priority: "", - index: 1}, + index: 1, enabled: true}, expected: "/*! no: semicolon */\nwalrus: zebra;\n", changed: {} }, + + { + desc: "create commented-out property", + input: "p: v", + instruction: {type: "create", name: "shoveler", value: "duck", priority: "", + index: 1, enabled: false}, + expected: "p: v;/*! shoveler: duck; */", + }, + { + desc: "disabled create with comment ender in string", + input: "", + instruction: {type: "create", name: "content", value: "'*/'", priority: "", + index: 0, enabled: false}, + expected: "/*! content: '*\\/'; */" + }, ]; function rewriteDeclarations(inputString, instruction, defaultIndentation) { @@ -458,7 +473,8 @@ function rewriteDeclarations(inputString, instruction, defaultIndentation) { case "create": rewriter.createProperty(instruction.index, instruction.name, - instruction.value, instruction.priority); + instruction.value, instruction.priority, + instruction.enabled); break; case "set": diff --git a/devtools/client/styleeditor/test/browser.ini b/devtools/client/styleeditor/test/browser.ini index 5a1404ebe45f..d9c0e3d6b229 100644 --- a/devtools/client/styleeditor/test/browser.ini +++ b/devtools/client/styleeditor/test/browser.ini @@ -95,6 +95,7 @@ skip-if = e10s && debug # Bug 1252201 - Docshell leak on debug e10s [browser_styleeditor_sourcemap_large.js] [browser_styleeditor_sourcemap_watching.js] [browser_styleeditor_sync.js] +[browser_styleeditor_syncAddProperty.js] [browser_styleeditor_syncAddRule.js] [browser_styleeditor_syncAlreadyOpen.js] [browser_styleeditor_syncEditSelector.js] diff --git a/devtools/client/styleeditor/test/browser_styleeditor_syncAddProperty.js b/devtools/client/styleeditor/test/browser_styleeditor_syncAddProperty.js new file mode 100644 index 000000000000..e197157adce3 --- /dev/null +++ b/devtools/client/styleeditor/test/browser_styleeditor_syncAddProperty.js @@ -0,0 +1,45 @@ +/* vim: set ts=2 et sw=2 tw=80: */ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ +"use strict"; + +// Test that adding a new rule is synced to the style editor. + +const TESTCASE_URI = TEST_BASE_HTTP + "sync.html"; + +const expectedText = ` + body { + border-width: 15px; + color: red; + } + + #testid { + font-size: 4em; + /*! background-color: yellow; */ + } + `; + +add_task(function* () { + yield addTab(TESTCASE_URI); + let { inspector, view } = yield openRuleView(); + yield selectNode("#testid", inspector); + + info("Focusing a new property name in the rule-view"); + let ruleEditor = getRuleViewRuleEditor(view, 1); + let editor = yield focusEditableField(view, ruleEditor.closeBrace); + is(inplaceEditor(ruleEditor.newPropSpan), editor, + "The new property editor has focus"); + + let input = editor.input; + input.value = "/* background-color: yellow; */"; + + info("Pressing return to commit and focus the new value field"); + let onModifications = view.once("ruleview-changed"); + EventUtils.synthesizeKey("VK_RETURN", {}, view.styleWindow); + yield onModifications; + + let { ui } = yield openStyleEditor(); + let sourceEditor = yield ui.editors[0].getSourceEditor(); + let text = sourceEditor.sourceEditor.getText(); + is(text, expectedText, "selector edits are synced"); +}); diff --git a/devtools/shared/css-parsing-utils.js b/devtools/shared/css-parsing-utils.js index 45995874b235..fa8a0c2110c8 100644 --- a/devtools/shared/css-parsing-utils.js +++ b/devtools/shared/css-parsing-utils.js @@ -800,10 +800,12 @@ RuleRewriter.prototype = { * @param {String} value value of the new property * @param {String} priority priority of the new property; either * the empty string or "important" + * @param {Boolean} enabled True if the new property should be + * enabled, false if disabled * @return {Promise} a promise that is resolved when the edit has * completed */ - internalCreateProperty: Task.async(function* (index, name, value, priority) { + internalCreateProperty: Task.async(function* (index, name, value, priority, enabled) { this.completeInitialization(index); let newIndentation = ""; if (this.hasNewLine) { @@ -833,13 +835,18 @@ RuleRewriter.prototype = { } } - this.result += newIndentation + CSS.escape(name) + ": " + - this.sanitizeText(value, index); - + let newText = CSS.escape(name) + ": " + this.sanitizeText(value, index); if (priority === "important") { - this.result += " !important"; + newText += " !important"; } - this.result += ";"; + newText += ";"; + + if (!enabled) { + newText = "/*" + COMMENT_PARSING_HEURISTIC_BYPASS_CHAR + " " + + escapeCSSComment(newText) + " */"; + } + + this.result += newIndentation + newText; if (this.hasNewLine) { this.result += "\n"; } @@ -860,10 +867,12 @@ RuleRewriter.prototype = { * @param {String} value value of the new property * @param {String} priority priority of the new property; either * the empty string or "important" + * @param {Boolean} enabled True if the new property should be + * enabled, false if disabled */ - createProperty: function (index, name, value, priority) { + createProperty: function (index, name, value, priority, enabled) { this.editPromise = this.internalCreateProperty(index, name, value, - priority); + priority, enabled); }, /** @@ -884,7 +893,7 @@ RuleRewriter.prototype = { // We might see a "set" on a previously non-existent property; in // that case, act like "create". if (!this.decl) { - this.createProperty(index, name, value, priority); + this.createProperty(index, name, value, priority, true); return; } diff --git a/devtools/shared/fronts/styles.js b/devtools/shared/fronts/styles.js index 0e77e01a4dc8..830a9238a695 100644 --- a/devtools/shared/fronts/styles.js +++ b/devtools/shared/fronts/styles.js @@ -398,7 +398,7 @@ var RuleModificationList = Class({ * Create a new property. This implementation does nothing, because * |setRuleText| is not available. * - * These parameter are passed, but as they are not used in this + * These parameters are passed, but as they are not used in this * implementation, they are omitted. They are documented here as * this code also defined the interface implemented by @see * RuleRewriter. @@ -412,6 +412,8 @@ var RuleModificationList = Class({ * @param {String} value value of the new property * @param {String} priority priority of the new property; either * the empty string or "important" + * @param {Boolean} enabled True if the new property should be + * enabled, false if disabled */ createProperty: function () { // Nothing.