Skip to content
Permalink
Browse files
Web Inspector: Adding a new style property to a rule after a commente…
…d out property results in incorrect style text/bad frontend state

https://bugs.webkit.org/show_bug.cgi?id=247139
rdar://88824802

Reviewed by Devin Rousso.

Two problems existed that together caused us to incorrectly associate incoming edits to properties following a
commented-out property with that commented out property, or to create a new WI.CSSProperty instead of using the existing
one that is being edited, resulting in various garbage text being written to the style sheet.

The first issue is in how we handle `pendingProperties` in WI.CSSStyleDeclaration. We currently consider a pending
property to be one that is not enabled (e.g. commented out), which does not match the intent of the comments in the
code and nor does it match its usage. The intended use for `pendingProperties` is to house properties that are no
longer part of the CSSStyleDeclaration after an update, presumedly to allow us to reuse `WI.CSSProperty`s during
undo/redo operations. It is also imaginable that this is where properties being edited should briefly exist if after
their creation, we receive an update from the backend before we have received the update that commits the property under
edit.

The misclassification of what belongs in `pendingProperties` is was not enough by itself to cause the issue however. The
issues arose when DOMNodeStyles._parseStylePropertyPayload attempted to associate incoming property payloads with existing
WI.CSSProperty objects. For pending properties, we only have the property name as a point of verification, which is ripe
for collision when looking at the array of "Pending Properties" that includes commented-out properties.

In addition to properly classifying a "pending" property we need to consider all existing properties for an earlier
comparison between payload properties and WI.CSSProperty objects in WI.DOMNodeStyles.prototype._parseStylePropertyPayload.
We currently are only considering enabled properties for direct matches, even though a commented-out property could
still be a direct match by name and index to an existing property. Failing to make this association results in us
unnecessarily creating a new WI.CSSProperty for the commented out rule, which is turn grows the number of "Pending
Properties" as we toss objects out of the main set of properties for a CSSStyleDeclaration and into the "Pending
Properties", where they then can end up colliding by name inside WI.DOMNodeStyles.prototype._parseStylePropertyPayload.

We instead need to use all of the existing properties to check for a direct name+index match to catch all possible
properties for reuse.

It seems likely that this regressed sometime during the bringup of the Spreadsheet* editor.
https://commits.webkit.org/208214@main, while likely not the cause, renamed the trinity of property arrays as follows:
`properties` -> `enabledProperties`
`allProperties` -> `properties`
`allVisibleProperties` -> `visibleProperties`

It seems ripe for misunderstanding that previously we had both an `allProperties` and a `properties` and I suspect that
the root misunderstanding of this issue stems from that. A quick audit of other uses of `enabledProperties` in our
current code shows generally reasonable usage as far as I can tell.

* LayoutTests/inspector/css/add-css-property-expected.txt:
* LayoutTests/inspector/css/add-css-property.html:

* Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:
* Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:
(WI.DOMNodeStyles.prototype._parseStylePropertyPayload):

Canonical link: https://commits.webkit.org/256410@main
  • Loading branch information
patrickangle committed Nov 7, 2022
1 parent 1513963 commit 0d990da3c01519907affff83cfdee71283f2ef4f
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 3 deletions.
@@ -18,3 +18,13 @@ PASS: `display: inline` property should be appended.
-- Running test case: AddCSSProperty.AppendAfterCommentedOutPropertyWithoutSemicolon
PASS: `display: inline` property should be appended.

-- Running test case: AddCSSProperty.CommentOutAndThenAppendAfterCommentedOutProperty.DifferentPropertyName
PASS: 'color: red' property should be commented out.
PASS: 'display:' part of new property should be appended.
PASS: 'display: flex' property should be fully appended.

-- Running test case: AddCSSProperty.CommentOutAndThenAppendAfterCommentedOutProperty.ExistingPropertyName
PASS: 'color: red' property should be commented out.
PASS: 'color:' part of new property should be appended.
PASS: 'color: rebeccapurple' property should be fully appended.

@@ -110,6 +110,52 @@
}
});

function addTestCaseForCommentingOutAndAppendingProperty({subtestName, description, ruleSelector, newPropertyName, newPropertyValue})
{
suite.addTestCase({
name: `AddCSSProperty.CommentOutAndThenAppendAfterCommentedOutProperty.${subtestName}`,
description,
async test() {
let styleDeclaration = getMatchedStyle(ruleSelector);

const commentOut = true;
styleDeclaration.properties[1].commentOut(commentOut);

const expectedAfterCommentOut = `\n font-size: 14px;\n /* color: red; */\n`;
InspectorTest.expectEqual(styleDeclaration.text, expectedAfterCommentOut, "'color: red' property should be commented out.");

let newProperty = styleDeclaration.newBlankProperty(2);
styleDeclaration.locked = true;
newProperty.name = newPropertyName;
const expectedAfterAddingName = `\n font-size: 14px;\n /* color: red; */\n ${newPropertyName}: ;\n`;
InspectorTest.expectEqual(styleDeclaration.text, expectedAfterAddingName, `'${newPropertyName}:' part of new property should be appended.`);


newProperty.rawValue = newPropertyValue;
const expectedafterAddingValue = `\n font-size: 14px;\n /* color: red; */\n ${newPropertyName}: ${newPropertyValue};\n`;
InspectorTest.expectEqual(styleDeclaration.text, expectedafterAddingValue, `'${newPropertyName}: ${newPropertyValue}' property should be fully appended.`);

styleDeclaration.locked = false;
},
});
}

addTestCaseForCommentingOutAndAppendingProperty({
subtestName: "DifferentPropertyName",
description: "Ensure that commenting out a property and then appending a new property below it behaves as intended.",
ruleSelector: ".rule-f",
newPropertyName: "display",
newPropertyValue: "flex",
});

addTestCaseForCommentingOutAndAppendingProperty({
subtestName: "ExistingPropertyName",
description: "Ensure that commenting out a property and then appending a new property with the same name below it behaves as intended.",
ruleSelector: ".rule-g",
newPropertyName: "color",
newPropertyValue: "rebeccapurple",
});

WI.domManager.requestDocument((documentNode) => {
documentNode.querySelector("#x", (contentNodeId) => {
if (contentNodeId) {
@@ -151,7 +197,15 @@
font-size: 14px;
/* color: red */
}
.rule-f {
font-size: 14px;
color: red;
}
.rule-g {
font-size: 14px;
color: red;
}
</style>
<div id="x" class="rule-a rule-b rule-c rule-d rule-e" style="width: 100px"></div>
<div id="x" class="rule-a rule-b rule-c rule-d rule-e rule-f rule-g" style="width: 100px"></div>
</body>
</html>
@@ -217,7 +217,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
}

for (let oldProperty of oldProperties) {
if (this.enabledProperties.includes(oldProperty))
if (this._properties.includes(oldProperty))
continue;

// Clear the index, since it is no longer valid.
@@ -549,7 +549,7 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object

if (styleDeclaration) {
// Use propertyForName when the index is NaN since propertyForName is fast in that case.
var property = isNaN(index) ? styleDeclaration.propertyForName(name) : styleDeclaration.enabledProperties[index];
var property = isNaN(index) ? styleDeclaration.propertyForName(name) : styleDeclaration.properties[index];

// Reuse a property if the index and name matches. Otherwise it is a different property
// and should be created from scratch. This works in the simple cases where only existing

0 comments on commit 0d990da

Please sign in to comment.