<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:joepeck@webkit.org" title="Joseph Pecoraro <joepeck@webkit.org>"> <span class="fn">Joseph Pecoraro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Show warning icon for invalid CSS properties and/or values"
href="https://bugs.webkit.org/show_bug.cgi?id=145657">bug 145657</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #254810 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Show warning icon for invalid CSS properties and/or values"
href="https://bugs.webkit.org/show_bug.cgi?id=145657#c8">Comment # 8</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Show warning icon for invalid CSS properties and/or values"
href="https://bugs.webkit.org/show_bug.cgi?id=145657">bug 145657</a>
from <span class="vcard"><a class="email" href="mailto:joepeck@webkit.org" title="Joseph Pecoraro <joepeck@webkit.org>"> <span class="fn">Joseph Pecoraro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=254810&action=diff" name="attach_254810" title="Patch">attachment 254810</a> <a href="attachment.cgi?id=254810&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=254810&action=review">https://bugs.webkit.org/attachment.cgi?id=254810&action=review</a>
r- while we think about how best to warn about prefixes.
<span class="quote">> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:289
> + getFirstMatchingProperty(name)</span >
This name does not accurately describe what this function is doing.
<span class="quote">> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:291
> + function levenshteinDistance(s, t) {</span >
This should really be a Utility function. Probably in Utilities.js.
<span class="quote">> 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});</span >
Style: Use the shorthand literal syntax where possible.
So instead of:
{distance: distance, name: property}
These can be:
{distance, name: property}
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.css:87
> +}
> +.css-style-text-editor > .CodeMirror .CodeMirror-lines .invalid-warning-marker.clickable {</span >
Style: Newline between new rules.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:552
> + var propertyHasUnnecessaryPrefix = property.name.includes("-webkit-") && WebInspector.CSSCompletions.cssNameCompletions.isValidPropertyName(property.name.replace("-webkit-", ""));</span >
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-")
<span class="quote">> 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
> + });
> + }</span >
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.
<span class="quote">> 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.");</span >
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.
<span class="quote">> 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
> + };</span >
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.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:624
> + title: WebInspector.UIString("The property '%s' does not exist.").format(property.name)</span >
Could be "Property '%s' is not supported". This may be a property supported by newer / other browsers.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>