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>
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.
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 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 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.
Created attachment 448787 [details] Patch
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)
Created attachment 448799 [details] Patch
Comment on attachment 448799 [details] Patch r=me
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].