Bug 215680

Created attachment 406902 [details]
Patch

Screenshot?

Comment on attachment 406902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406902&action=review

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:114
> +body:not(.meta-key-pressed) .spreadsheet-style-declaration-editor > .property:matches(.invalid-name, .invalid-value, .other-vendor, .overridden):not(.disabled) > .content > :matches(.name, .value-container):not(.editing),
> +body:not(.meta-key-pressed) .spreadsheet-style-declaration-editor > .property:matches(.invalid-name, .invalid-value, .other-vendor, .overridden):not(.disabled) > .content > .value-container > .value:not(.editing),
> +body.meta-key-pressed .spreadsheet-style-declaration-editor > .property:matches(.invalid-name, .invalid-value, .other-vendor, .overridden):not(.disabled) > .content > :matches(.name, .value-container):not(:hover, .editing),
> +body.meta-key-pressed .spreadsheet-style-declaration-editor > .property:matches(.invalid-name, .invalid-value, .other-vendor, .overridden):not(.disabled) > .content > .value-container > .value:not(:hover, .editing) {

That's one long selector. I see it isn't particularly new — two rules below use very similar selector.
The `body:not(.meta-key-pressed)` and `body.meta-key-pressed` gives me a pause in particular. I wonder how we can simplify it.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:115
> +    color: hsla(0, 0%, var(--foreground-lightness), 0.6);

Any particular reason not to use `var(--text-color-secondary)`?

Comment on attachment 406902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406902&action=review

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:114
>> +body.meta-key-pressed .spreadsheet-style-declaration-editor > .property:matches(.invalid-name, .invalid-value, .other-vendor, .overridden):not(.disabled) > .content > .value-container > .value:not(:hover, .editing) {
> 
> That's one long selector. I see it isn't particularly new — two rules below use very similar selector.
> The `body:not(.meta-key-pressed)` and `body.meta-key-pressed` gives me a pause in particular. I wonder how we can simplify it.

`body:not(.meta-key-pressed)` includes `:hover`
`body.meta-key-pressed` excludes `:hover`

this is also "necessary" because of specificity conflicts with other rules (which possibly could be resolved using additional `:not`, but that's likely a larger change)

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:115
>> +    color: hsla(0, 0%, var(--foreground-lightness), 0.6);
> 
> Any particular reason not to use `var(--text-color-secondary)`?

I wanted to match the color of the strikethrough.

Comment on attachment 406902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406902&action=review

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:114
>>> +body.meta-key-pressed .spreadsheet-style-declaration-editor > .property:matches(.invalid-name, .invalid-value, .other-vendor, .overridden):not(.disabled) > .content > .value-container > .value:not(:hover, .editing) {
>> 
>> That's one long selector. I see it isn't particularly new — two rules below use very similar selector.
>> The `body:not(.meta-key-pressed)` and `body.meta-key-pressed` gives me a pause in particular. I wonder how we can simplify it.
> 
> `body:not(.meta-key-pressed)` includes `:hover`
> `body.meta-key-pressed` excludes `:hover`
> 
> this is also "necessary" because of specificity conflicts with other rules (which possibly could be resolved using additional `:not`, but that's likely a larger change)

Okay, I think it's good for now.

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:115
>>> +    color: hsla(0, 0%, var(--foreground-lightness), 0.6);
>> 
>> Any particular reason not to use `var(--text-color-secondary)`?
> 
> I wanted to match the color of the strikethrough.

I think that strikethrough should also be `var(--text-color-secondary)`.

Created attachment 406966 [details]
[Image] Screenshot of issue

Created attachment 406967 [details]
[Image] After Patch is applied

Comment on attachment 406967 [details]
[Image] After Patch is applied

Looking at this more closely, it appears that the `:` and `;` are also re-colored, which is probably not necessary.  I'll tweak the selector to not include those.

Created attachment 407085 [details]
Patch

Comment on attachment 407085 [details]
Patch

r=me, these huge selectors make me dizzy though.

Committed r266066: <https://trac.webkit.org/changeset/266066>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407085 [details].

<rdar://problem/67685695>