Bug 153702 – Web Inspector: Show error icons if invalid values already exist for properties in the Visual sidebar

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

[Devin Rousso]

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).

[Radar WebKit Bug Importer]

<rdar://problem/24424025>

[Devin Rousso]

Created attachment 270302 [details]
Patch

[Devin Rousso]

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?

[Blaze Burg]

Please upload a screenshot with the change.

[Blaze Burg]

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.

[Devin Rousso]

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.

[Blaze Burg]

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.

[Devin Rousso]

Created attachment 270603 [details]
Patch

[Timothy Hatcher]

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.

[Devin Rousso]

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?

[Timothy Hatcher]

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.

[Devin Rousso]

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>

[WebKit Commit Bot]

Comment on attachment 270603 [details]
Patch

Clearing flags on attachment: 270603

Committed r196146: <http://trac.webkit.org/changeset/196146>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.