[Webkit-unassigned] [Bug 145657] Web Inspector: Show warning icon for invalid CSS properties and/or values

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=145657

Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #254810|review?                     |review-
              Flags|                            |

--- Comment #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 254810
  --> https://bugs.webkit.org/attachment.cgi?id=254810
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150612/8a546eac/attachment-0001.html>