Bug 141262 - Web Inspector: if a known CSS property has an unsupported value, only strikethrough the value
Summary: Web Inspector: if a known CSS property has an unsupported value, only striket...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-02-04 14:22 PST by Brian Burg
Modified: 2015-06-04 15:49 PDT (History)
9 users (show)

See Also:


Attachments
Current appearance (13.29 KB, image/png)
2015-02-04 14:22 PST, Brian Burg
no flags Details
Patch (3.95 KB, patch)
2015-06-03 16:47 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 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.