Bug 153702 - Web Inspector: Show error icons if invalid values already exist for properties in the Visual sidebar
Summary: Web Inspector: Show error icons if invalid values already exist for propertie...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-01-29 22:45 PST by Devin Rousso
Modified: 2016-02-04 13:51 PST (History)
8 users (show)

See Also:


Attachments
Patch (11.97 KB, patch)
2016-01-29 22:52 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After patch is applied (34.89 KB, image/png)
2016-02-01 13:17 PST, Devin Rousso
no flags Details
Patch (12.16 KB, patch)
2016-02-03 14:47 PST, 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 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 Brian Burg 2016-02-01 06:02:08 PST
Please upload a screenshot with the change.
Comment 5 Brian 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 Brian 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.