Bug 1069829 - 2 - Don't use cssPropertyIsValid in the rule-view UI, get the info from the server instead; r=tromey

- StyleRuleActors now parse declarations and send them within the form
- Each declaration also contains isValid flag
- Parsing is still done client side for old backends (back compat)
- Declarations are sent with the form, so updated every time the rule changes
- Also made StyleRuleActors send the declarations array for element styles, and made canSetRuleText true for this, since we can simply set the style attribute

MozReview-Commit-ID: 2nI4bRyvwwi

--HG--
extra : rebase_source : 0ccfaeb0edb0b1a60a7c0d741295d61ad66ac57e
This commit is contained in:
Patrick Brosset 2016-04-29 09:30:02 +02:00
Родитель 6deb1d00c9
Коммит e57616990f
11 изменённых файлов: 249 добавлений и 134 удалений

Просмотреть файл

@ -150,6 +150,9 @@ const StyleRuleFront = FrontClassWithSpec(styleRuleSpec, {
get authoredText() {
return this._form.authoredText || this._form.cssText;
},
get declarations() {
return this._form.declarations || [];
},
get keyText() {
return this._form.keyText;
},

Просмотреть файл

@ -39,12 +39,19 @@ EditingSession.prototype = {
/**
* Gets the value of a single property from the CSS rule.
*
* @param rule The CSS rule
* @param property The name of the property
* @param {StyleRuleFront} rule The CSS rule.
* @param {String} property The name of the property.
* @return {String} The value.
*/
getPropertyFromRule: function (rule, property) {
let dummyStyle = this._element.style;
// Use the parsed declarations in the StyleRuleFront object if available.
let index = this.getPropertyIndex(property, rule);
if (index !== -1) {
return rule.declarations[index].value;
}
// Fallback to parsing the cssText locally otherwise.
let dummyStyle = this._element.style;
dummyStyle.cssText = rule.cssText;
return dummyStyle.getPropertyValue(property);
},
@ -77,49 +84,91 @@ EditingSession.prototype = {
},
/**
* Sets a number of properties on the node. Returns a promise that will be
* resolved when the modifications are complete.
*
* Get the index of a given css property name in a CSS rule.
* Or -1, if there are no properties in the rule yet.
* @param {String} name The property name.
* @param {StyleRuleFront} rule Optional, defaults to the element style rule.
* @return {Number} The property index in the rule.
*/
getPropertyIndex: function (name, rule = this._rules[0]) {
let elementStyleRule = this._rules[0];
if (!elementStyleRule.declarations.length) {
return -1;
}
return elementStyleRule.declarations.findIndex(p => p.name === name);
},
/**
* Sets a number of properties on the node.
* @param properties An array of properties, each is an object with name and
* value properties. If the value is "" then the property
* is removed.
* @return {Promise} Resolves when the modifications are complete.
*/
setProperties: function (properties) {
let modifications = this._rules[0].startModifyingProperties();
setProperties: Task.async(function* (properties) {
for (let property of properties) {
// Get a RuleModificationList or RuleRewriter helper object from the
// StyleRuleActor to make changes to CSS properties.
// Note that RuleRewriter doesn't support modifying several properties at
// once, so we do this in a sequence here.
let modifications = this._rules[0].startModifyingProperties();
// Remember the property so it can be reverted.
if (!this._modifications.has(property.name)) {
this._modifications.set(property.name,
this.getPropertyFromRule(this._rules[0], property.name));
}
if (property.value == "") {
modifications.removeProperty(-1, property.name);
} else {
modifications.setProperty(-1, property.name, property.value, "");
// Find the index of the property to be changed, or get the next index to
// insert the new property at.
let index = this.getPropertyIndex(property.name);
if (index === -1) {
index = this._rules[0].declarations.length;
}
}
return modifications.apply().then(null, console.error);
},
if (property.value == "") {
modifications.removeProperty(index, property.name);
} else {
modifications.setProperty(index, property.name, property.value, "");
}
yield modifications.apply();
}
}),
/**
* Reverts all of the property changes made by this instance. Returns a
* promise that will be resolved when complete.
* Reverts all of the property changes made by this instance.
* @return {Promise} Resolves when all properties have been reverted.
*/
revert: function () {
let modifications = this._rules[0].startModifyingProperties();
revert: Task.async(function* () {
// Revert each property that we modified previously, one by one. See
// setProperties for information about why.
for (let [property, value] of this._modifications) {
if (value != "") {
modifications.setProperty(-1, property, value, "");
} else {
modifications.removeProperty(-1, property);
}
}
let modifications = this._rules[0].startModifyingProperties();
return modifications.apply().then(null, console.error);
},
// Find the index of the property to be reverted.
let index = this.getPropertyIndex(property);
if (value != "") {
// If the property doesn't exist anymore, insert at the beginning of the
// rule.
if (index === -1) {
index = 0;
}
modifications.setProperty(index, property, value, "");
} else {
// If the property doesn't exist anymore, no need to remove it. It had
// not been added after all.
if (index === -1) {
continue;
}
modifications.removeProperty(index, property);
}
yield modifications.apply();
}
}),
destroy: function () {
this._doc = null;
@ -342,14 +391,15 @@ LayoutView.prototype = {
}
}
session.setProperties(properties);
session.setProperties(properties).catch(e => console.error(e));
},
done: (value, commit) => {
editor.elt.parentNode.classList.remove("layout-editing");
if (!commit) {
session.revert();
session.destroy();
session.revert().then(() => {
session.destroy();
}, e => console.error(e));
}
}
}, event);

Просмотреть файл

@ -196,7 +196,11 @@ Rule.prototype = {
this.applyProperties((modifications) => {
modifications.createProperty(ind, name, value, priority);
// 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();
});
return prop;
},
@ -241,6 +245,9 @@ Rule.prototype = {
return modifications.apply().then(() => {
let cssProps = {};
// Note that even though StyleRuleActors normally provide parsed
// declarations already, _applyPropertiesNoAuthored is only used when
// connected to older backend that do not provide them. So parse here.
for (let cssProp of parseDeclarations(this.style.authoredText)) {
cssProps[cssProp.name] = cssProp;
}
@ -433,7 +440,13 @@ Rule.prototype = {
_getTextProperties: function () {
let textProps = [];
let store = this.elementStyle.store;
let props = parseDeclarations(this.style.authoredText, true);
// Starting with FF49, StyleRuleActors provide parsed declarations.
let props = this.style.declarations;
if (!props) {
props = parseDeclarations(this.style.authoredText, true);
}
for (let prop of props) {
let name = prop.name;
// If the authored text has an invalid property, it will show up

Просмотреть файл

@ -199,12 +199,30 @@ TextProperty.prototype = {
/**
* Validate this property. Does it make sense for this value to be assigned
* to this property name? This does not apply the property value
* to this property name?
*
* @return {Boolean} true if the property value is valid, false otherwise.
*/
isValid: function () {
return domUtils.cssPropertyIsValid(this.name, this.value);
// Starting with FF49, StyleRuleActors provide a list of parsed
// declarations, with data about their validity, but if we don't have this,
// compute validity locally (which might not be correct, but better than
// nothing).
if (!this.rule.domRule.declarations) {
return domUtils.cssPropertyIsValid(this.name, this.value);
}
let selfIndex = this.rule.textProps.indexOf(this);
// When adding a new property in the rule-view, the TextProperty object is
// created right away before the rule gets updated on the server, so we're
// not going to find the corresponding declaration object yet. Default to
// true.
if (!this.rule.domRule.declarations[selfIndex]) {
return true;
}
return this.rule.domRule.declarations[selfIndex].isValid;
}
};

Просмотреть файл

@ -15,7 +15,7 @@ add_task(function* () {
yield selectNode("#target", inspector);
info("Setting a font-weight property on all rules");
setPropertyOnAllRules(view);
yield setPropertyOnAllRules(view);
info("Reselecting the element");
yield selectNode("body", inspector);
@ -24,10 +24,14 @@ add_task(function* () {
checkPropertyOnAllRules(view);
});
function setPropertyOnAllRules(view) {
function* setPropertyOnAllRules(view) {
// Wait for the properties to be properly created on the backend and for the
// view to be updated.
let onRefreshed = view.once("ruleview-refreshed");
for (let rule of view._elementStyle.rules) {
rule.editor.addProperty("font-weight", "bold", "");
}
yield onRefreshed;
}
function checkPropertyOnAllRules(view) {

Просмотреть файл

@ -4,31 +4,29 @@
"use strict";
//
// Test adding a valid property to a CSS rule, and navigating through the fields
// by pressing ENTER.
const TEST_URI = `
<style type="text/css">
#testid {
background-color: blue;
color: blue;
}
.testclass, .unmatched {
background-color: green;
};
</style>
<div id='testid' class='testclass'>Styled Node</div>
<div id='testid2'>Styled Node</div>
<div id='testid'>Styled Node</div>
`;
add_task(function* () {
yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
let {inspector, view} = yield openRuleView();
yield selectNode("#testid", inspector);
info("Focus the new property name field");
let elementRuleEditor = getRuleViewRuleEditor(view, 0);
let editor = yield focusNewRuleViewProperty(elementRuleEditor);
let ruleEditor = getRuleViewRuleEditor(view, 1);
let editor = yield focusNewRuleViewProperty(ruleEditor);
let input = editor.input;
is(inplaceEditor(elementRuleEditor.newPropSpan), editor,
is(inplaceEditor(ruleEditor.newPropSpan), editor,
"Next focused editor should be the new property editor.");
ok(input.selectionStart === 0 && input.selectionEnd === input.value.length,
"Editor contents are selected.");
@ -42,28 +40,27 @@ add_task(function* () {
editor.input.value = "background-color";
info("Pressing RETURN and waiting for the value field focus");
let onNameAdded = view.once("ruleview-changed");
// Pressing ENTER triggeres 2 changed events, one for the new property, and
// one for the preview.
let onNameAdded = waitForNEvents(view, "ruleview-changed", 2);
EventUtils.synthesizeKey("VK_RETURN", {}, view.styleWindow);
yield onNameAdded;
editor = inplaceEditor(view.styleDocument.activeElement);
is(elementRuleEditor.rule.textProps.length, 1,
is(ruleEditor.rule.textProps.length, 2,
"Should have created a new text property.");
is(elementRuleEditor.propertyList.children.length, 1,
is(ruleEditor.propertyList.children.length, 2,
"Should have created a property editor.");
let textProp = elementRuleEditor.rule.textProps[0];
let textProp = ruleEditor.rule.textProps[1];
is(editor, inplaceEditor(textProp.editor.valueSpan),
"Should be editing the value span now.");
info("Entering the property value");
// We're editing inline style, so expect a style attribute mutation.
let onMutated = inspector.once("markupmutation");
let onValueAdded = view.once("ruleview-changed");
editor.input.value = "purple";
EventUtils.synthesizeKey("VK_RETURN", {}, view.styleWindow);
yield onValueAdded;
yield onMutated;
is(textProp.value, "purple", "Text prop should have been changed.");
});

Просмотреть файл

@ -59,13 +59,12 @@ function* checkCopySelection(view) {
range.setStart(prop, 0);
range.setEnd(values[4], 2);
win.getSelection().addRange(range);
info("Checking that _Copy() returns the correct clipboard value");
let expectedPattern = " margin: 10em;[\\r\\n]+" +
" font-size: 14pt;[\\r\\n]+" +
" font-family: helvetica,sans-serif;[\\r\\n]+" +
" color: rgb\\(170, 170, 170\\);[\\r\\n]+" +
" font-family: helvetica, sans-serif;[\\r\\n]+" +
" color: #AAA;[\\r\\n]+" +
"}[\\r\\n]+" +
"html {[\\r\\n]+" +
" color: #000000;[\\r\\n]*";
@ -101,8 +100,8 @@ function* checkSelectAll(view) {
let expectedPattern = "element {[\\r\\n]+" +
" margin: 10em;[\\r\\n]+" +
" font-size: 14pt;[\\r\\n]+" +
" font-family: helvetica,sans-serif;[\\r\\n]+" +
" color: rgb\\(170, 170, 170\\);[\\r\\n]+" +
" font-family: helvetica, sans-serif;[\\r\\n]+" +
" color: #AAA;[\\r\\n]+" +
"}[\\r\\n]+" +
"html {[\\r\\n]+" +
" color: #000000;[\\r\\n]+" +

Просмотреть файл

@ -6,7 +6,7 @@
// Test "Copy color" item of the context menu #1: Test _isColorPopup.
const TEST_URI = `
<div style="color: #123ABC; margin: 0px; background: span[data-color];">
<div style="color:rgb(18, 58, 188);margin:0px;background:span[data-color];">
Test "Copy color" context menu option
</div>
`;

Просмотреть файл

@ -908,6 +908,12 @@ RuleRewriter.prototype = {
*/
removeProperty: function (index, name) {
this.completeInitialization(index);
// If asked to remove a property that does not exist, bail out.
if (!this.decl) {
return;
}
let copyOffset = this.decl.offsets[1];
// Maybe removing this rule left us with a completely blank
// line. In this case, we'll delete the whole thing. We only

Просмотреть файл

@ -12,23 +12,16 @@ const events = require("sdk/event/core");
const {Class} = require("sdk/core/heritage");
const {PageStyleFront, StyleRuleFront} = require("devtools/client/fronts/styles");
const {LongStringActor} = require("devtools/server/actors/string");
const {
getDefinedGeometryProperties
} = require("devtools/server/actors/highlighters/geometry-editor");
const {getDefinedGeometryProperties} = require("devtools/server/actors/highlighters/geometry-editor");
const {parseDeclarations} = require("devtools/client/shared/css-parsing-utils");
// This will also add the "stylesheet" actor type for protocol.js to recognize
const {UPDATE_PRESERVING_RULES, UPDATE_GENERAL} =
require("devtools/server/actors/stylesheets");
const {UPDATE_PRESERVING_RULES, UPDATE_GENERAL} = require("devtools/server/actors/stylesheets");
const {pageStyleSpec, styleRuleSpec} = require("devtools/shared/specs/styles");
loader.lazyRequireGetter(this, "CSS", "CSS");
loader.lazyGetter(this, "CssLogic", () => {
return require("devtools/shared/inspector/css-logic").CssLogic;
});
loader.lazyGetter(this, "DOMUtils", () => {
return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
});
loader.lazyGetter(this, "CssLogic", () => require("devtools/shared/inspector/css-logic").CssLogic);
loader.lazyGetter(this, "DOMUtils", () => Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils));
// The PageStyle actor flattens the DOM CSS objects a little bit, merging
// Rules and their Styles into one actor. For elements (which have a style
@ -992,17 +985,16 @@ var StyleRuleActor = protocol.ActorClassWithSpec(styleRuleSpec, {
// True if this rule supports as-authored styles, meaning that the
// rule text can be rewritten using setRuleText.
get canSetRuleText() {
// Special case about:PreferenceStyleSheet, as it is
// generated on the fly and the URI is not registered with the
// about: handler.
// https://bugzilla.mozilla.org/show_bug.cgi?id=935803#c37
return !!(this._parentSheet &&
// If a rule does not have source, then it has been
// modified via CSSOM; and we should fall back to
// non-authored editing.
// https://bugzilla.mozilla.org/show_bug.cgi?id=1224121
this.sheetActor.allRulesHaveSource() &&
this._parentSheet.href !== "about:PreferenceStyleSheet");
return this.type === ELEMENT_STYLE ||
(this._parentSheet &&
// If a rule does not have source, then it has been modified via
// CSSOM; and we should fall back to non-authored editing.
// https://bugzilla.mozilla.org/show_bug.cgi?id=1224121
this.sheetActor.allRulesHaveSource() &&
// Special case about:PreferenceStyleSheet, as it is generated on
// the fly and the URI is not registered with the about:handler
// https://bugzilla.mozilla.org/show_bug.cgi?id=935803#c37
this._parentSheet.href !== "about:PreferenceStyleSheet");
},
getDocument: function(sheet) {
@ -1080,6 +1072,7 @@ var StyleRuleActor = protocol.ActorClassWithSpec(styleRuleSpec, {
let doc = this.rawNode.ownerDocument;
form.href = doc.location ? doc.location.href : "";
form.cssText = this.rawStyle.cssText || "";
form.authoredText = this.rawNode.getAttribute("style");
break;
case Ci.nsIDOMCSSRule.CHARSET_RULE:
form.encoding = this.rawRule.encoding;
@ -1097,6 +1090,18 @@ var StyleRuleActor = protocol.ActorClassWithSpec(styleRuleSpec, {
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) {
let declarations = parseDeclarations(form.authoredText ||
form.cssText, true);
form.declarations = declarations.map(decl => {
decl.isValid = DOMUtils.cssPropertyIsValid(decl.name, decl.value);
return decl;
});
}
return form;
},
@ -1239,21 +1244,26 @@ var StyleRuleActor = protocol.ActorClassWithSpec(styleRuleSpec, {
* @returns the rule with updated properties
*/
setRuleText: Task.async(function* (newText) {
if (!this.canSetRuleText ||
(this.type !== Ci.nsIDOMCSSRule.STYLE_RULE &&
this.type !== Ci.nsIDOMCSSRule.KEYFRAME_RULE)) {
if (!this.canSetRuleText) {
throw new Error("invalid call to setRuleText");
}
let parentStyleSheet = this.pageStyle._sheetRef(this._parentSheet);
let {str: cssText} = yield parentStyleSheet.getText();
if (this.type === ELEMENT_STYLE) {
// For element style rules, set the node's style attribute.
this.rawNode.setAttribute("style", newText);
} else {
// For stylesheet rules, set the text in the stylesheet.
let parentStyleSheet = this.pageStyle._sheetRef(this._parentSheet);
let {str: cssText} = yield parentStyleSheet.getText();
let {offset, text} = getRuleText(cssText, this.line, this.column);
cssText = cssText.substring(0, offset) + newText +
cssText.substring(offset + text.length);
let {offset, text} = getRuleText(cssText, this.line, this.column);
cssText = cssText.substring(0, offset) + newText +
cssText.substring(offset + text.length);
yield parentStyleSheet.update(cssText, false, UPDATE_PRESERVING_RULES);
}
this.authoredText = newText;
yield parentStyleSheet.update(cssText, false, UPDATE_PRESERVING_RULES);
return this;
}),

Просмотреть файл

@ -22,62 +22,77 @@ var gWalker = null;
var gStyles = null;
var gClient = null;
addTest(function setup() {
addAsyncTest(function* setup() {
let url = document.getElementById("inspectorContent").href;
attachURL(url, function(err, client, tab, doc) {
gInspectee = doc;
let {InspectorFront} = require("devtools/server/actors/inspector");
let inspector = InspectorFront(client, tab);
promiseDone(inspector.getWalker().then(walker => {
ok(walker, "getWalker() should return an actor.");
let inspector;
yield new Promise(resolve => {
attachURL(url, function(err, client, tab, doc) {
gInspectee = doc;
gClient = client;
gWalker = walker;
return inspector.getPageStyle();
}).then(styles => {
gStyles = styles;
}).then(runNextTest));
let {InspectorFront} = require("devtools/server/actors/inspector");
inspector = InspectorFront(client, tab);
resolve();
});
});
gWalker = yield inspector.getWalker();
gStyles = yield inspector.getPageStyle();
runNextTest();
});
addTest(function modifyProperties() {
addAsyncTest(function* modifyProperties() {
let localNode = gInspectee.querySelector("#inheritable-rule-inheritable-style");
let elementStyle = null;
promiseDone(gWalker.querySelector(gWalker.rootNode, "#inheritable-rule-inheritable-style").then(node => {
return gStyles.getApplied(node, { inherited: false, filter: "user" });
}).then(applied => {
elementStyle = applied[0].rule;
is(elementStyle.cssText, localNode.style.cssText, "Got expected css text");
// Will start with "color:blue"
let changes = elementStyle.startModifyingProperties();
let node = yield gWalker.querySelector(gWalker.rootNode,
"#inheritable-rule-inheritable-style");
// Change an existing property...
changes.setProperty(-1, "color", "black");
// Create a new property
changes.setProperty(-1, "background-color", "green");
let applied = yield gStyles.getApplied(node,
{ inherited: false, filter: "user" });
// Create a new property and then change it immediately.
changes.setProperty(-1, "border", "1px solid black");
changes.setProperty(-1, "border", "2px solid black");
let elementStyle = applied[0].rule;
is(elementStyle.cssText, localNode.style.cssText, "Got expected css text");
return changes.apply();
}).then(() => {
is(elementStyle.cssText, "color: black; background-color: green; border: 2px solid black;", "Should have expected cssText");
is(elementStyle.cssText, localNode.style.cssText, "Local node and style front match.");
// Change an existing property...
yield setProperty(elementStyle, 0, "color", "black");
// Create a new property
yield setProperty(elementStyle, 1, "background-color", "green");
// Remove all the properties
let changes = elementStyle.startModifyingProperties();
changes.removeProperty(-1, "color");
changes.removeProperty(-1, "background-color");
changes.removeProperty(-1, "border");
// Create a new property and then change it immediately.
yield setProperty(elementStyle, 2, "border", "1px solid black");
yield setProperty(elementStyle, 2, "border", "2px solid black");
return changes.apply();
}).then(() => {
is(elementStyle.cssText, "", "Should have expected cssText");
is(elementStyle.cssText, localNode.style.cssText, "Local node and style front match.");
}).then(runNextTest));
is(elementStyle.cssText,
"color: black; background-color: green; border: 2px solid black;",
"Should have expected cssText");
is(elementStyle.cssText, localNode.style.cssText,
"Local node and style front match.");
// Remove all the properties
yield removeProperty(elementStyle, 0, "color");
yield removeProperty(elementStyle, 0, "background-color");
yield removeProperty(elementStyle, 0, "border");
is(elementStyle.cssText, "", "Should have expected cssText");
is(elementStyle.cssText, localNode.style.cssText,
"Local node and style front match.");
runNextTest();
});
function* setProperty(rule, index, name, value) {
let changes = rule.startModifyingProperties();
changes.setProperty(index, name, value);
yield changes.apply();
}
function* removeProperty(rule, index, name) {
let changes = rule.startModifyingProperties();
changes.removeProperty(index, name);
yield changes.apply();
}
addTest(function cleanup() {
delete gStyles;
delete gWalker;