Bug 153702

Summary: Web Inspector: Show error icons if invalid values already exist for properties in the Visual sidebar
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
[Image] After patch is applied
none
Patch none

Description Devin Rousso 2016-01-29 22:45:14 PST
Along the lines of <https://bugs.webkit.org/show_bug.cgi?id=148678>, if invalid CSS (meaning that the value is not valid, not that the property is prefixed) already exists, instead of silently failing and showing the computed value, display an error icon explaining that the current value is invalid (similar to what happens if "display: asd;" is entered into the Rules sidebar, but with an Error icon instead of a Warning icon).
Comment 1 Radar WebKit Bug Importer 2016-01-29 22:45:58 PST
<rdar://problem/24424025>
Comment 2 Devin Rousso 2016-01-29 22:52:11 PST
Created attachment 270302 [details]
Patch
Comment 3 Devin Rousso 2016-01-29 22:53:54 PST
Comment on attachment 270302 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:219
> +        WebInspector.VisualStylePropertyEditor.prototype.__lookupSetter__("specialPropertyPlaceholderElementText").call(this, text);

So I realize that this is not the best way to do this, but since I can't call "super.specialPropertyPlaceholderElementText = text", I wasn't sure what to do.  I figured that this solution was better than making an inner function like "this._setSpecialPropertyPlaceholderElementText(text)" since the function's body was so minimal.  Is it preferred to do it this way or using the inner function?
Comment 4 BJ Burg 2016-02-01 06:02:08 PST
Please upload a screenshot with the change.
Comment 5 BJ Burg 2016-02-01 06:03:27 PST
Comment on attachment 270302 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:219
>> +        WebInspector.VisualStylePropertyEditor.prototype.__lookupSetter__("specialPropertyPlaceholderElementText").call(this, text);
> 
> So I realize that this is not the best way to do this, but since I can't call "super.specialPropertyPlaceholderElementText = text", I wasn't sure what to do.  I figured that this solution was better than making an inner function like "this._setSpecialPropertyPlaceholderElementText(text)" since the function's body was so minimal.  Is it preferred to do it this way or using the inner function?

I had no idea this was a thing. I guess it's ok since a FIXME is above it.
Comment 6 Devin Rousso 2016-02-01 13:17:17 PST
Created attachment 270417 [details]
[Image] After patch is applied

There is also a tooltip on the Error icon that says:
The value '%s' is not supported for this property.
Comment 7 BJ Burg 2016-02-03 06:39:10 PST
Comment on attachment 270302 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:8
> +

Please include summary blurb here.

> Source/WebInspectorUI/ChangeLog:33
> +        Before assigning the value of the property, if found, to the editor,

This sentence is awkwards.
Comment 8 Devin Rousso 2016-02-03 14:47:54 PST
Created attachment 270603 [details]
Patch
Comment 9 Timothy Hatcher 2016-02-04 12:37:42 PST
Comment on attachment 270603 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:294
> +                this._warningElement.title = WebInspector.UIString("The value '%s' is not supported for this property.").format(propertyText);

Should use curly quotes (“”) not single quotes.
Comment 10 Devin Rousso 2016-02-04 12:41:49 PST
Comment on attachment 270603 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:294
>> +                this._warningElement.title = WebInspector.UIString("The value '%s' is not supported for this property.").format(propertyText);
> 
> Should use curly quotes (“”) not single quotes.

Really?  OK I have been using ('') incorrectly for a while then.  All of the warning icon's tooltips (both the Rules and Visual sidebars) use '%s'.  Would you rather me make another patch to resolve all of these issues, or just fix this one?
Comment 11 Timothy Hatcher 2016-02-04 13:02:25 PST
Comment on attachment 270603 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:294
>>> +                this._warningElement.title = WebInspector.UIString("The value '%s' is not supported for this property.").format(propertyText);
>> 
>> Should use curly quotes (“”) not single quotes.
> 
> Really?  OK I have been using ('') incorrectly for a while then.  All of the warning icon's tooltips (both the Rules and Visual sidebars) use '%s'.  Would you rather me make another patch to resolve all of these issues, or just fix this one?

You can fix them in a follow up.
Comment 12 Devin Rousso 2016-02-04 13:16:30 PST
Comment on attachment 270603 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:294
>>>> +                this._warningElement.title = WebInspector.UIString("The value '%s' is not supported for this property.").format(propertyText);
>>> 
>>> Should use curly quotes (“”) not single quotes.
>> 
>> Really?  OK I have been using ('') incorrectly for a while then.  All of the warning icon's tooltips (both the Rules and Visual sidebars) use '%s'.  Would you rather me make another patch to resolve all of these issues, or just fix this one?
> 
> You can fix them in a follow up.

Ok will do.  <https://bugs.webkit.org/show_bug.cgi?id=153891>
Comment 13 WebKit Commit Bot 2016-02-04 13:51:31 PST
Comment on attachment 270603 [details]
Patch

Clearing flags on attachment: 270603

Committed r196146: <http://trac.webkit.org/changeset/196146>
Comment 14 WebKit Commit Bot 2016-02-04 13:51:35 PST
All reviewed patches have been landed.  Closing bug.