Bug 233195 - REGRESSION (r283723): Web Inspector: CSS declarations unexpectedly removed when editing property value
Summary: REGRESSION (r283723): Web Inspector: CSS declarations unexpectedly removed wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-16 09:18 PST by Nikita Vasilyev
Modified: 2022-01-11 10:54 PST (History)
5 users (show)

See Also:


Attachments
WIP (3.63 KB, patch)
2021-11-16 09:36 PST, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (7.37 KB, patch)
2022-01-10 12:45 PST, Nikita Vasilyev
hi: review-
Details | Formatted Diff | Diff
Patch (7.99 KB, patch)
2022-01-10 14:11 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2021-11-16 09:18:45 PST
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>
Comment 1 Nikita Vasilyev 2021-11-16 09:20:28 PST
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.
Comment 2 Nikita Vasilyev 2021-11-16 09:36:43 PST
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.
Comment 3 Devin Rousso 2021-12-07 10:59:10 PST
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.
Comment 4 Nikita Vasilyev 2022-01-10 12:45:28 PST
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.
Comment 5 Nikita Vasilyev 2022-01-10 12:45:49 PST
Created attachment 448787 [details]
Patch
Comment 6 Devin Rousso 2022-01-10 13:49:02 PST
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)
Comment 7 Nikita Vasilyev 2022-01-10 14:11:42 PST
Created attachment 448799 [details]
Patch
Comment 8 Devin Rousso 2022-01-11 10:49:09 PST
Comment on attachment 448799 [details]
Patch

r=me
Comment 9 EWS 2022-01-11 10:54:49 PST
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].