Bug 215681 - Web Inspector: Elements: Styles: don't show swatches for properties that aren't used/apply
Summary: Web Inspector: Elements: Styles: don't show swatches for properties that aren...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 215680
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-19 20:03 PDT by Devin Rousso
Modified: 2020-08-24 10:24 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.51 KB, patch)
2020-08-19 20:05 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2020-08-19 20:03:41 PDT
a common pattern in CSS is to have two of the same property next to each other, the earlier intended as a fallback for the latter

```
    color: black;
    color: var(--color);
```

showing a swatch in front of the former property isn't all that useful as it's not used/applied and just adds to the confusion of what is used/applied (especially after bug 215680)
Comment 1 Devin Rousso 2020-08-19 20:05:28 PDT
Created attachment 406903 [details]
Patch
Comment 2 Nikita Vasilyev 2020-08-19 21:05:09 PDT
I'm a little bit on the fence about this. Leaning towards that the benefits of this patch outweigh the downsides.

On one hand, the strikethrough doesn't go over the swatches and it looks bad. From the visual design perspective, this patch is an improvement.
On the other hand, I find it useful to see what colors are overridden.
Comment 3 Devin Rousso 2020-08-20 12:20:35 PDT
(In reply to Nikita Vasilyev from comment #2)
> On one hand, the strikethrough doesn't go over the swatches and it looks bad. From the visual design perspective, this patch is an improvement.

Agreed.

> On the other hand, I find it useful to see what colors are overridden.

Really?  I've never really found this to be something I cared for.  Not to mention, in my experience the thing that's overridden is often a raw color (i.e. not a variable), meaning that I can just read it to see what color is used.
Comment 4 BJ Burg 2020-08-24 10:00:34 PDT
Comment on attachment 406903 [details]
Patch

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

r=me with a question.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:466
> +        if (this._property.enabled && !this._property.overridden && this._property.valid && !this._property.hasOtherVendorNameOrKeyword())

Do we support any hasOtherVendorNameOrKeyword properties that do not have an unprefixed counterpart? This seems fine, unless it means some obscure --webkit- properties no longer get swatches.
Comment 5 Radar WebKit Bug Importer 2020-08-24 10:11:13 PDT
<rdar://problem/67685687>
Comment 6 Devin Rousso 2020-08-24 10:22:58 PDT
Comment on attachment 406903 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:466
>> +        if (this._property.enabled && !this._property.overridden && this._property.valid && !this._property.hasOtherVendorNameOrKeyword())
> 
> Do we support any hasOtherVendorNameOrKeyword properties that do not have an unprefixed counterpart? This seems fine, unless it means some obscure --webkit- properties no longer get swatches.

IIRC `hasOtherVendorNameOrKeyword` checks to see whether the name/value starts with `-moz-`/`-ms-`/`-o-`, so it shouldn't ever affect anything starting with `-webkit`.
Comment 7 EWS 2020-08-24 10:24:56 PDT
Committed r266070: <https://trac.webkit.org/changeset/266070>

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