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.
<rdar://problem/91593601>
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.
Broke in https://trac.webkit.org/changeset/266069/webkit
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.
Created attachment 458087 [details] Patch
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?
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.
(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.
(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.
Created attachment 459124 [details] Patch
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.
Pull request: https://github.com/WebKit/WebKit/pull/1193
Committed 253576@main (544098974025): <https://commits.webkit.org/253576@main> Reviewed commits have been landed. Closing PR #1193 and removing active labels.