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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 12 12:39:35 PDT 2015


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

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.

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>

More information about the webkit-unassigned mailing list