Bug 145657 – Web Inspector: Show warning icon for invalid CSS properties and/or values

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

[Jon Davis]

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.

[Radar WebKit Bug Importer]

<rdar://problem/21248517>

[Devin Rousso]

Created attachment 254385 [details]
Patch

[Devin Rousso]

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

[Timothy Hatcher]

Comment on attachment 254385 [details]
Patch

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.

[Devin Rousso]

Created attachment 254672 [details]
Patch

[Devin Rousso]

Comment on attachment 254672 [details]
Patch

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?

[Devin Rousso]

Created attachment 254810 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 254810 [details]
Patch

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.

    property.name.startsWith("-webkit-")

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

[Devin Rousso]

Created attachment 255127 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 255127 [details]
Patch

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.

[Devin Rousso]

Created attachment 255469 [details]
Patch

[Timothy Hatcher]

Comment on attachment 255469 [details]
Patch

Looks great!

[WebKit Commit Bot]

Comment on attachment 255469 [details]
Patch

Clearing flags on attachment: 255469

Committed r185928: <http://trac.webkit.org/changeset/185928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.