RESOLVED FIXED239092
REGRESSION (r266069): Web Inspector: Can't create new CSS properties for inherited rules 
https://bugs.webkit.org/show_bug.cgi?id=239092
Summary REGRESSION (r266069): Web Inspector: Can't create new CSS properties for inhe...
Nikita Vasilyev
Reported 2022-04-11 15:52:17 PDT
Steps: 1. Open https://webkit.org 2. Inspect <body> 3. In the right side bar scroll down to ":root" rule 4. Click on the white space to create a new CSS property Actual: Nothing happens. No uncaught exception either. Notes: Reproduces in STP 143.
Attachments
Patch (1.74 KB, patch)
2022-04-21 13:34 PDT, Nikita Vasilyev 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (5.30 KB, patch)
2022-05-10 10:55 PDT, Nikita Vasilyev 		nvasilyev: review?
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-04-11 15:52:27 PDT
<rdar://problem/91593601>
Nikita Vasilyev
Comment 2 2022-04-13 11:16:12 PDT
Reproduces in r270001 and I can't easily bisect much further back. It has been broken for at least 17 months, it seems, which is surprising to me.
Nikita Vasilyev
Comment 3 2022-04-14 15:41:22 PDT
Broke in https://trac.webkit.org/changeset/266069/webkit
Nikita Vasilyev
Comment 4 2022-04-21 13:29:17 PDT
This broke because blank properties are not inherited. I think the reasonable way to fix this is to display modified properties (which includes blank properties) alongside inherited properties. This would allows to add a new property and it wouldn't disaster once it's added.
Nikita Vasilyev
Comment 5 2022-04-21 13:34:35 PDT
Created attachment 458087 [details] Patch
Devin Rousso
Comment 6 2022-04-21 13:58:03 PDT
Comment on attachment 458087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458087&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:256 > + properties = properties.filter((property) => property.inherited || property.modified); wouldn't this also include any modified properties of any inherited rule? e.g. ``` <div> <span></span> </div> <style> div { color: red; } <style> ``` if I added a `display: block;` after the `color: red;`, that'd be a modified property, right?
Nikita Vasilyev
Comment 7 2022-04-21 14:18:44 PDT
Devin, yes, in your example, if you're inspecting <span>, you'd see both `color: red` and `display: block`. I think that modified CSS is likely relevant to the user and I don't see harm showing it. Also, it's better than some of the alternatives. I'd rather not make inherited rules read-only. I'd rather not roll back r266069. I'd rather not make an added properly disappear if it isn't inherited.
Devin Rousso
Comment 8 2022-04-21 14:31:25 PDT
(In reply to Nikita Vasilyev from comment #7) > I think that modified CSS is likely relevant to the user and I don't see harm showing it. I don't see how a property that is not inheritable is relevant. > Also, it's better than some of the alternatives. I'd rather not make inherited rules read-only. I'd rather not roll back r266069. I'd rather not make an added properly disappear if it isn't inherited. Why are those the only alternatives? We could just as easily add a `WI.CSSProperty.prototype.get isBlankProperty` that's set when creating the `WI.CSSProperty` inside `WI.CSSStyleDeclaration.prototype.newBlankProperty` and then unset in `WI.CSSProperty.prototype._updateStyleText` (or somewhere similar) if both the `name` and `rawValue` are set.
Nikita Vasilyev
Comment 9 2022-05-10 10:47:06 PDT
(In reply to Devin Rousso from comment #8) > ... We could just as easily add a > `WI.CSSProperty.prototype.get isBlankProperty` that's set when creating the > `WI.CSSProperty` inside `WI.CSSStyleDeclaration.prototype.newBlankProperty` > and then unset in `WI.CSSProperty.prototype._updateStyleText` (or somewhere > similar) if both the `name` and `rawValue` are set. We can't unset `isBlankProperty` in _updateStyleText because it would fire as soon as you type the first character of the CSSProperty value. It can be unset on blur event instead, I suppose.
Nikita Vasilyev
Comment 10 2022-05-10 10:55:59 PDT
Created attachment 459124 [details] Patch
Nikita Vasilyev
Comment 11 2022-05-10 10:58:48 PDT
I still like my first patch more. The new patch is more complex and likely more error prone. (In reply to Devin Rousso from comment #8) > (In reply to Nikita Vasilyev from comment #7) > > I think that modified CSS is likely relevant to the user and I don't see harm showing it. > I don't see how a property that is not inheritable is relevant. We are trying to display properties that are relevant to a web developer. Inherited properties are more relevant than non-inherited, it makes sense. I property that the web developer just added and is very likely to be relevant, too.
Nikita Vasilyev
Comment 12 2022-05-31 15:31:59 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1193
EWS
Comment 13 2022-08-18 18:25:43 PDT
Committed 253576@main (544098974025): <https://commits.webkit.org/253576@main> Reviewed commits have been landed. Closing PR #1193 and removing active labels.
Note You need to log in before you can comment on or make changes to this bug.