Summary: | REGRESSION (r283723): Web Inspector: CSS declarations unexpectedly removed when editing property value | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ews-watchlist, hi, inspector-bugzilla-changes, pangle, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2021-11-16 09:18:45 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. 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]. |