Bug 141262

Summary: Web Inspector: if a known CSS property has an unsupported value, only strikethrough the value
Product: WebKit Reporter: Brian Burg <burg>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, hi, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Current appearance
none
Patch none

Description Brian Burg 2015-02-04 14:22:58 PST
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.
Comment 1 Radar WebKit Bug Importer 2015-02-04 14:23:29 PST
<rdar://problem/19720851>
Comment 2 Timothy Hatcher 2015-02-04 14:25:28 PST
Good call!
Comment 3 Devin Rousso 2015-06-03 16:47:20 PDT
Created attachment 254219 [details]
Patch
Comment 4 Timothy Hatcher 2015-06-03 18:07:17 PDT
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?
Comment 5 Devin Rousso 2015-06-03 18:49:30 PDT
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.
Comment 6 WebKit Commit Bot 2015-06-03 20:42:56 PDT
Comment on attachment 254219 [details]
Patch

Clearing flags on attachment: 254219

Committed r185184: <http://trac.webkit.org/changeset/185184>
Comment 7 WebKit Commit Bot 2015-06-03 20:43:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Brian Burg 2015-06-03 21:03:03 PDT
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?
Comment 9 Joseph Pecoraro 2015-06-04 12:31:21 PDT
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!
Comment 10 Devin Rousso 2015-06-04 15:49:29 PDT
Created a new bug for additional changes: https://bugs.webkit.org/show_bug.cgi?id=145668.