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.
<rdar://problem/21248517>
Created attachment 254385 [details] Patch
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 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.
Created attachment 254672 [details] Patch
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?
Created attachment 254810 [details] Patch
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.
Created attachment 255127 [details] Patch
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.
Created attachment 255469 [details] Patch
Comment on attachment 255469 [details] Patch Looks great!
Comment on attachment 255469 [details] Patch Clearing flags on attachment: 255469 Committed r185928: <http://trac.webkit.org/changeset/185928>
All reviewed patches have been landed. Closing bug.