Bug 141262 – Web Inspector: if a known CSS property has an unsupported value, only strikethrough the value

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

Created attachment 246049 [details]
Current appearance

Use case: accidentally put 'visibility: none;' in the stylesheet. The whole line is strikethrough with red. But we know 'visibility' is a real property (cross references with CSS.getSupportedProperties), so just cross out the property value.

[Radar WebKit Bug Importer]

<rdar://problem/19720851>

[Timothy Hatcher]

Good call!

[Devin Rousso]

Created attachment 254219 [details]
Patch

[Timothy Hatcher]

Comment on attachment 254219 [details]
Patch

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

Awesome!

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:279
> +    nameMatchesValidPropertyExactly(name)

Do we need to do anything special for properties that can have a "-webkit-" prefix or not? Like transform can, and some others.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:542
> +            this._codeMirror.markText(start, end, {className: "invalid"});

Will this marker get cleared when edits are made to make the value valid?

[Devin Rousso]

Comment on attachment 254219 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:279
>> +    nameMatchesValidPropertyExactly(name)
> 
> Do we need to do anything special for properties that can have a "-webkit-" prefix or not? Like transform can, and some others.

I tested this with a few different "-webkit-" prefixed properties and it worked correctly without any issues, so no.  As far as I understand, the property name includes "-webkit-" so it will match with that in mind.  If I had used canonicalName, then this would be a problem as it strips prefixes.

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:542
>> +            this._codeMirror.markText(start, end, {className: "invalid"});
> 
> Will this marker get cleared when edits are made to make the value valid?

Yes the marker is redrawn every time the editor is updated, so if the property is no longer invalid it will not draw.

[WebKit Commit Bot]

Comment on attachment 254219 [details]
Patch

Clearing flags on attachment: 254219

Committed r185184: <http://trac.webkit.org/changeset/185184>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Burg]

Comment on attachment 254219 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:281
> +        for (var property of this._values) {

Array.prototype.includes?

[Joseph Pecoraro]

Comment on attachment 254219 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:279
>>> +    nameMatchesValidPropertyExactly(name)
>> 
>> Do we need to do anything special for properties that can have a "-webkit-" prefix or not? Like transform can, and some others.
> 
> I tested this with a few different "-webkit-" prefixed properties and it worked correctly without any issues, so no.  As far as I understand, the property name includes "-webkit-" so it will match with that in mind.  If I had used canonicalName, then this would be a problem as it strips prefixes.

This name reads weird to me. How about isValidPropertyName or isSupportedPropertyName?

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:281
>> +        for (var property of this._values) {
> 
> Array.prototype.includes?

Yes!

[Devin Rousso]

Created a new bug for additional changes: https://bugs.webkit.org/show_bug.cgi?id=145668.