Bug 198629 – Web Inspector: longhand CSS properties overridden by shorthands miss strikethrough

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Example:

    border-color: red;
    border: 1px solid green;

The 1st property should be displayed as overridden and have a strikethrough.

<rdar://problem/45341525>

[Radar WebKit Bug Importer]

<rdar://problem/51504160>

[Nikita Vasilyev]

Created attachment 371536 [details]
Patch

Tip for reviewers: LayoutTests/inspector/css/overridden-property.html doubles as a reduction page. You can open it in an old Safari to see how it worked before, and in ToT WebKit with the patch applied to see what changed.

[EWS Watchlist]

Attachment 371536 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikita Vasilyev]

(In reply to Build Bot from comment #3)
> Attachment 371536 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312:  Line
> contains single-quote character.  [js/syntax] [5]
> Total errors found: 1 in 6 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

`Property "${this.formattedText}" can't override itself.`
                                     ^
                                     It didn't like this single-quote :/

[Devin Rousso]

Comment on attachment 371536 [details]
Patch

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

As a side note,  I'd expect a shorthand property to be overridden  if ALL of its shorthands are present and override this property, but that's probably going to be more work, so it may deserve its own followup.

> LayoutTests/inspector/css/overridden-property-expected.txt:13
> +border-top-color: red; overridden

These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected.

> LayoutTests/inspector/css/overridden-property.html:10
> +        if (property.implicit)

Should this instead be an `InspectorTest.assert`?  I'd expect that `matchedRule.style.enabledProperties` shouldn't contain any implicit properties.

> LayoutTests/inspector/css/overridden-property.html:47
> +                if (rule.selectorText === selector) {

Why not use `WI.DOMNodeStyles.prototype.rulesForSelector`?  Although, it doesn't appear to be used anywhere, so you could also just remove it :P

> LayoutTests/inspector/css/overridden-property.html:54
> +                InspectorTest.log(`Couldn't find ${selector}`);

This should be an `InspectorTest.fail`.

> LayoutTests/inspector/css/overridden-property.html:113
> +        name: "NotOverriddenByLonghandImportant",

We also have a `NotOverriddeenByLonghand` for more test coverage/completeness.

> LayoutTests/inspector/css/overridden-property.html:-71
> -    <p>Test that CSSProperty.prototype.overridden is cleared when the overriding (effective) property is removed.</p>

There should always be a description in the test that explains overall what the test is for.

> LayoutTests/inspector/css/overridden-property.html:134
> +    .shorthand-important {

These names aren't very clear.  If anything i'd expect this to be `.shorthand-overridden-by-important-longhand`.

> LayoutTests/inspector/css/overridden-property.html:139
> +    .longhand-important {

Ditto (>134), but with `.longhand-overridden-by-important-shorthand` instead.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:302
> +        console.assert(this !== effectiveProperty, `Property "${this.formattedText}" can't override itself.`);

NIT: I'd also log `this` as another argument so that if inspector2 is already open, we can do more investigation immediately.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:862
> +            else {

Style: no `else` when the `if` is an early-return.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:907
> +                if (isOverriddenBy(property, property.relatedShorthandProperty)) {

To prevent future foot-guns, I think we should rename this function to be `isOverriddenByRelatedShorthand` and have the logic for fetching `property.relatedShorthandProperty` be inside the function itself.  The reason for this is that we don't want to mark a shorthand property as overridden if only one of it's longhands exists (see >overridden-property.html:140).

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #4)
> (In reply to Build Bot from comment #3)
> > Attachment 371536 [details] did not pass style-queue:
> > 
> > 
> > ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312:  Line contains single-quote character.  [js/syntax] [5]
> > Total errors found: 1 in 6 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> `Property "${this.formattedText}" can't override itself.`
>                                      ^
>                                      It didn't like this single-quote :/
This is definitely a bug :(

Please file a bug against check-webkit-style.

[Matt Baker]

(In reply to Devin Rousso from comment #6)
> (In reply to Nikita Vasilyev from comment #4)
> > (In reply to Build Bot from comment #3)
> > > Attachment 371536 [details] did not pass style-queue:
> > > 
> > > 
> > > ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312:  Line contains single-quote character.  [js/syntax] [5]
> > > Total errors found: 1 in 6 files
> > > 
> > > 
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > `Property "${this.formattedText}" can't override itself.`
> >                                      ^
> >                                      It didn't like this single-quote :/
> This is definitely a bug :(
> 
> Please file a bug against check-webkit-style.

I filed a related style checker bug a while back:

<https://webkit.org/b/165453> check-webkit-style: false positive caused by regex literal matching in SingleQuoteChecker

[Nikita Vasilyev]

Comment on attachment 371536 [details]
Patch

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

Thanks for reviewing!

>> LayoutTests/inspector/css/overridden-property-expected.txt:13
>> +border-top-color: red; overridden
> 
> These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected.

Where would you put a "PASS"?

It seems to me that as long as the actual text matches the expected text, it's a pass. Having "PASS" logged and also having mismatched expected/actual text would only make it confusing.

>> LayoutTests/inspector/css/overridden-property.html:10
>> +        if (property.implicit)
> 
> Should this instead be an `InspectorTest.assert`?  I'd expect that `matchedRule.style.enabledProperties` shouldn't contain any implicit properties.

No, matchedRule.style.enabledProperties does contain implicit properties. `visibleProperties` does NOT. I could use either, I don't have a preference.

[Devin Rousso]

Comment on attachment 371536 [details]
Patch

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

>>> LayoutTests/inspector/css/overridden-property-expected.txt:13
>>> +border-top-color: red; overridden
>> 
>> These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected.
> 
> Where would you put a "PASS"?
> 
> It seems to me that as long as the actual text matches the expected text, it's a pass. Having "PASS" logged and also having mismatched expected/actual text would only make it confusing.

I mean using the `InspectorText.expect*` functions.  It would show as a PASS if they match, and a FAIL if not.  I had to read the expected file a few times before I understood what it was trying to tell me.

The point of the PASS is to make it clear to _other_ people what they should be looking for.  Sometimes, dumping the raw text makes sense.  I don't think that's the case here because it's not obvious what the "overridden" means since the rest of the text is real CSS.

[Matt Baker]

(In reply to Devin Rousso from comment #9)
> Comment on attachment 371536 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371536&action=review
> 
> >>> LayoutTests/inspector/css/overridden-property-expected.txt:13
> >>> +border-top-color: red; overridden
> >> 
> >> These results really need a `PASS` somewhere, as otherwise it's really hard to tell if it's working as expected.
> > 
> > Where would you put a "PASS"?
> > 
> > It seems to me that as long as the actual text matches the expected text, it's a pass. Having "PASS" logged and also having mismatched expected/actual text would only make it confusing.
> 
> I mean using the `InspectorText.expect*` functions.  It would show as a PASS
> if they match, and a FAIL if not.  I had to read the expected file a few
> times before I understood what it was trying to tell me.

Some tests are less straightforward to express in the form "actual vs. expected". Many of our tests log stack traces or other state, and just compare it against future logs.

In general these are more difficult to read and understand, and rely on the author ensuring the initial output is correct. Tests of the form "actual vs. expected" are a bit better in both regards, IMO.

> The point of the PASS is to make it clear to _other_ people what they should
> be looking for.  Sometimes, dumping the raw text makes sense.  I don't think
> that's the case here because it's not obvious what the "overridden" means
> since the rest of the text is real CSS.

[Matt Baker]

Comment on attachment 371536 [details]
Patch

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

>> LayoutTests/inspector/css/overridden-property.html:54
>> +                InspectorTest.log(`Couldn't find ${selector}`);
> 
> This should be an `InspectorTest.fail`.

Whatever you do, you should resolve/reject the promise or call InspectorTest.completeTest, so that the test doesn't time out.

[Nikita Vasilyev]

Created attachment 371613 [details]
Patch

[EWS Watchlist]

Attachment 371613 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:312:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Comment on attachment 371613 [details]
Patch

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

r=me, with some final fixes before landing.

> LayoutTests/inspector/css/overridden-property-expected.txt:15
> +PASS: border-top-color is overridden.

Nice! This is MUCH easier to read/understand.

> LayoutTests/inspector/css/overridden-property.html:38
> +    function getStyleDeclaration(selector, callback, resolve) {

Now that you're passing a `callback`, I'd replace the `resolve` with a `reject`, as we really should be throwing an error if we couldn't find the selector.

> LayoutTests/inspector/css/overridden-property.html:92
> +        name: "OverriddenByShorthand",

Style: please prefix the test case name with the suite name, so "OverriddenProperty.OverriddenByShorthand".

> LayoutTests/inspector/css/overridden-property.html:95
> +                InspectorTest.expectTrue(style.visibleProperties[0].overridden, "border-top-color is overridden.");

We should be using `propertyForName` instead of expecting that it's the first visible property.
```
    const dontCreateIfMissing = true;
    let borderTopColorProperty = style.propertyForName("border-top-color", dontCreateIfMissing);
```
Alternatively, adding an `InspectorTest.assert(style.visibleProperties[0].name === "border-top-color");` would be fine too.

> LayoutTests/inspector/css/overridden-property.html:120
> +            });

Shouldn't you also pass `reject` in here?

> LayoutTests/inspector/css/overridden-property.html:131
> +            });

Ditto (>120).

> LayoutTests/inspector/css/overridden-property.html:146
> +    .shorthand {

A better name could be used here, like `.longhand-overridden-by-shorthand`.

> LayoutTests/inspector/css/overridden-property.html:161
> +    .shorthand-not-overridden-longhand {

NIT: this is missing a `-by-`, so `.shorthand-not-overridden-by-longhand`.

> LayoutTests/inspector/css/overridden-property.html:170
> +    <div class="shorthand-overridden-by-important-longhand"></div>
> +    <div class="shorthand-overridden-by-important-longhand"></div>

Duplicate.  Please remove (or explain why it's still needed).

[Nikita Vasilyev]

Created attachment 371618 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 371618 [details]
Patch

Clearing flags on attachment: 371618

Committed r246223: <https://trac.webkit.org/changeset/246223>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.