Bug 145657 - Web Inspector: Show warning icon for invalid CSS properties and/or values
Summary: Web Inspector: Show warning icon for invalid CSS properties and/or values
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2015-06-04 14:29 PDT by Jon Davis
Modified: 2015-06-24 15:18 PDT (History)
9 users (show)

See Also:

Patch (5.86 KB, patch)
2015-06-05 14:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.77 KB, patch)
2015-06-10 13:21 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (11.01 KB, patch)
2015-06-12 11:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (15.97 KB, patch)
2015-06-18 13:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.74 KB, patch)
2015-06-23 19:15 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 Jon Davis 2015-06-04 14:29:18 PDT
When an invalid CSS property or invalid value is entered into the style sidebar in Web Inspector, a strikethrough is currently used. A strikethrough is also used for overridden styles. Although the colors are different, the visual notice is so subtle, and the display style so similar, it would be difficult to understand the meaning at a glance. I recommend using a warning icon before the name of either the invalid property or invalid values in the style sidebar. The strikethrough should remain to notify the user that the style will not be used.

It would also be nice for a tooltip to explicitly call out the invalid property or invalid value with a warning. To go above and beyond, the tooltip could suggest the most closely correct value/property name.
Comment 1 Radar WebKit Bug Importer 2015-06-04 14:29:46 PDT
Comment 2 Devin Rousso 2015-06-05 14:20:39 PDT
Created attachment 254385 [details]
Comment 3 Devin Rousso 2015-06-05 14:22:49 PDT
I am also planning on making functionality for when the property has a -webkit- and no longer needs it (transform, animation, etc.) or when the property doesn't have a -webkit- and does need it (font-smoothing, etc.).
Comment 4 Timothy Hatcher 2015-06-05 17:35:05 PDT
Comment on attachment 254385 [details]

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

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:289
> +    getFirstMatchingProperty(name, clipFront)

Any way to make this faster? I worry it will be a performance bottleneck.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:296
> +        return this.getFirstMatchingProperty(clipFront ? name.substring(1): name.slice(0, -1), clipFront);

This is pretty heavy, and recursive.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:548
> +                    title: "The value \"" + property.value + "\" is not valid.",

UIString here too.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:556
> +                    title: "Did you mean " + closestPropertyName + "?",

This should be a format string with WebInspector.UIString.
Comment 5 Devin Rousso 2015-06-10 13:21:49 PDT
Created attachment 254672 [details]
Comment 6 Devin Rousso 2015-06-10 13:24:20 PDT
Comment on attachment 254672 [details]

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

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:289
> +    getFirstMatchingProperty(name, clipFront)

I have tried using a different function (http://en.wikibooks.org/wiki/Algorithm_Implementation/Strings/Levenshtein_distance#JavaScript) to get better performance and more accurate results, but it is MIT licensed.  Is this a problem and if not, is there a specific way in which I need to show that to be able to use the code?
Comment 7 Devin Rousso 2015-06-12 11:35:10 PDT
Created attachment 254810 [details]
Comment 8 Joseph Pecoraro 2015-06-12 12:39:35 PDT
Comment on attachment 254810 [details]

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

r- while we think about how best to warn about prefixes.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:289
> +    getFirstMatchingProperty(name)

This name does not accurately describe what this function is doing.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:291
> +        function levenshteinDistance(s, t) {

This should really be a Utility function. Probably in Utilities.js.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:328
> +            if (distance < bestMatches[0].distance)
> +                bestMatches = [{distance: distance, name: property}];
> +            else if (distance === bestMatches[0].distance)
> +                bestMatches.push({distance: distance, name: property});

Style: Use the shorthand literal syntax where possible.

So instead of:

    {distance: distance, name: property}

These can be:

    {distance, name: property}

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.css:87
> +}
> +.css-style-text-editor > .CodeMirror .CodeMirror-lines .invalid-warning-marker.clickable {

Style: Newline between new rules.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:552
> +        var propertyHasUnnecessaryPrefix = property.name.includes("-webkit-") && WebInspector.CSSCompletions.cssNameCompletions.isValidPropertyName(property.name.replace("-webkit-", ""));

Nit: When checking for a prefix you only need to check if the string starts with the prefix, instead of includes, which might be wrong. We support String.prototype.startsWith so you don't need to use String.prototype.includes.


> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:585
> +            if (propertyHasUnnecessaryPrefix) {
> +                generateInvalidMarker.call(this, {
> +                    from: from,
> +                    title: WebInspector.UIString("The 'webkit' prefix is not necessary."),
> +                    correction: property.text.replace("-webkit-", ""),
> +                    autocomplete: false 
> +                });
> +            }

We need to be careful about this warning. It may not be a good idea. If someone removes the -webkit- prefix they may be losing support for legacy WebKit browsers that still need the prefix.

A better warning might be if a prefixed version is found but a non-prefixed version of the same property is not found.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:596
> +                    var valueReplacement = property.value.length ? WebInspector.UIString("The value '%s' is not valid.").format(property.value) : WebInspector.UIString("This property needs a value.");

Could be: "Value '%s' is not supported for this property." or "Value '%s' is not valid.". This may be a property value supported by newer / other browsers. It is tough to know if saying unsupported or invalid is better.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:610
> +                    invalidMarkerInfo = {
> +                        from: from,
> +                        title: WebInspector.UIString("The 'webkit' prefix is needed for this property."),
> +                        correction: "-webkit-" + property.text,
> +                        autocomplete: false 
> +                    };

Again, this can be misleading.

Take for instance iOS 8 and iOS 9 which unprefixed a bunch of properties. It sounds like when inspecting iOS 8 you'll get warnings "hey you need prefixes!". And Inspecting iOS 9 you'll get warnings "hey you don't need prefixes!". Really what would be best in most cases is a check that there are both a prefixed (for legacy) and unprefixed versions (future proof) of a property.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:624
> +                            title: WebInspector.UIString("The property '%s' does not exist.").format(property.name)

Could be "Property '%s' is not supported". This may be a property supported by newer / other browsers.
Comment 9 Devin Rousso 2015-06-18 13:32:08 PDT
Created attachment 255127 [details]
Comment 10 Joseph Pecoraro 2015-06-18 15:12:33 PDT
Comment on attachment 255127 [details]

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

Drive by, I haven't actually reviewed the logic yet.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:614
> +                title: WebInspector.UIString("The 'webkit' prefix is not necessary.\nDuplicate without the prefix?"),

I think it might be better to say "[...]. Click to insert a duplicate without the prefix."

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:621
> +                title: WebInspector.UIString("Duplicate property '%s'.\nDelete this property?").format(property.name),

Same here. "[...]. Click to delete this property." And so on for others.
Comment 11 Devin Rousso 2015-06-23 19:15:36 PDT
Created attachment 255469 [details]
Comment 12 Timothy Hatcher 2015-06-24 14:30:25 PDT
Comment on attachment 255469 [details]

Looks great!
Comment 13 WebKit Commit Bot 2015-06-24 15:18:32 PDT
Comment on attachment 255469 [details]

Clearing flags on attachment: 255469

Committed r185928: <http://trac.webkit.org/changeset/185928>
Comment 14 WebKit Commit Bot 2015-06-24 15:18:37 PDT
All reviewed patches have been landed.  Closing bug.