Bug 239092

<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.