Bug 233195 – REGRESSION (r283723): Web Inspector: CSS declarations unexpectedly removed when editing property value

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Steps To Reproduce:
1. In the Styles sidebar, click to edit a CSS property name
2. Press backspace until the entire text field is cleared (this is key!)
3. Type in any property name
4. Hit Enter to navigate to the CSS value
5. Hit Enter again to add a new CSS property 

See attached video recording [in Radar].

Results:
The edited CSS declaration should not be removed

Regression:
The edited CSS declaration is unexpectedly removed

Notes:
Bisection points to Bug 178835 as the regression:

Bug 178835 - Web Inspector: Styles: format style declarations after editing
https://bugs.webkit.org/show_bug.cgi?id=178835

<rdar://85345180>

[Nikita Vasilyev]

Note that even before that regression point, the CSS property would still incorrectly display value as unsupported if you replace step 4 with clicking away from the property.

[Nikita Vasilyev]

Created attachment 444403 [details]
WIP

The problem is when CSS property name is fully cleared (step 2), CSSProperty model gets removed from CSSStyleDeclaration#_properties. Consequent changes to that property don’t get saved. 

The WIP fixes the bug. The fix introduces `_indexBeforeDetached` property, which I don't like too much. I'm going to explore the alternatives; so far they are worse than the WIP.

[Devin Rousso]

Comment on attachment 444403 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=444403&action=review

Can we add a test for this?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:262
> +        } else if (typeof this._indexBeforeDetached === "number" && isNaN(this._index)) {

I think we should use `!isNaN(this._indexBeforeDetached)` instead.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:264
> +            console.assert(!this._ownerStyle.properties.includes(this), "Style declaration already has this CSSProperty.");

This message is redundant from the condition of the assert.  I'd remove it.

It'd also be nice to log `this` as an argument so that if this is hit then we get some information about what caused it.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:266
> +            this._indexBeforeDetached = null;

It seems like other things `.index = NaN`, so maybe we should `this._indexBeforeDetached = NaN;` here for consistency.

[Nikita Vasilyev]

Comment on attachment 444403 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=444403&action=review

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:262
>> +        } else if (typeof this._indexBeforeDetached === "number" && isNaN(this._index)) {
> 
> I think we should use `!isNaN(this._indexBeforeDetached)` instead.

`!isNaN` is essentially a double negative — not not a number, so that's why I didn't use it. !isNaN is more brief though, so I can go either way.

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:266
>> +            this._indexBeforeDetached = null;
> 
> It seems like other things `.index = NaN`, so maybe we should `this._indexBeforeDetached = NaN;` here for consistency.

Makes sense.

[Nikita Vasilyev]

Created attachment 448787 [details]
Patch

[Devin Rousso]

Comment on attachment 448787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448787&action=review

r-, as this is missing an entry in LayoutTests/ChangeLog

looks good otherwise tho :)

> LayoutTests/inspector/css/modify-css-property.html:328
> +                resolve();

NIT: Rather than capture `resolve` and manually calling it (which btw wont prevent the rest of the code from being run, so you may get weird other output), I'd suggest making it an `async test() { ... }` and using `throw "No declaration found.";` instead.

> LayoutTests/inspector/css/modify-css-property.html:343
> +            InspectorTest.expectEqual(styleDeclaration.text, ``, `Style declaration text should be empty.`);

NIT: Both of these should be regular double-quoted strings, not template strings.

> LayoutTests/inspector/css/modify-css-property.html:346
> +            let expectedStyleText = `\n    border-color: darkseagreen;\n`;

ditto (:343)

Also, why not inline this?

> LayoutTests/inspector/css/modify-css-property.html:347
> +            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should have new property name.`);

ditto (:343)

[Nikita Vasilyev]

Created attachment 448799 [details]
Patch

[Devin Rousso]

Comment on attachment 448799 [details]
Patch

r=me

[EWS]

Committed r287891 (245929@main): <https://commits.webkit.org/245929@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448799 [details].