RESOLVED FIXED 146859
Web Inspector: Warning icon tooltip for numbers with no units could be improved
https://bugs.webkit.org/show_bug.cgi?id=146859
Summary Web Inspector: Warning icon tooltip for numbers with no units could be improved
Timothy Hatcher
Reported 2015-07-10 16:01:58 PDT
Created attachment 256621 [details] Screenshot The warning says a number (other than zero) is not supported for properties like margin-top. The action and tooltip for the warning icon offer to delete the value and autocomplete. We might want to suggest adding "px" to the number to make it valid (since px is assumed for numbers in non-strict mode).
Attachments
Screenshot (61.45 KB, image/png)
2015-07-10 16:01 PDT, Timothy Hatcher
no flags
Patch (3.81 KB, patch)
2015-07-10 19:19 PDT, Devin Rousso
no flags
Patch (3.82 KB, patch)
2015-07-10 23:56 PDT, Devin Rousso
no flags
Radar WebKit Bug Importer
Comment 1 2015-07-10 16:03:01 PDT
Devin Rousso
Comment 2 2015-07-10 19:19:05 PDT
Timothy Hatcher
Comment 3 2015-07-10 19:58:21 PDT
Comment on attachment 256637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256637&action=review > Source/WebInspectorUI/ChangeLog:12 > + (WebInspector.CSSStyleDeclarationTextEditor.prototype._createTextMarkerForPropertyIfNeeded): > + If the property's value is incorrect and is comprised of only numbers, that must mean that the value needs > + to have units (like "px") after the number. Added another warning icon case to support this scenario. I guess this could be too generic, as px might not be the right unit (% or deg come to mind). But I can't think of any property off the top of my head. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1076 > + if (/\d+$/.test(property.value)) { /^\d+$/?
Devin Rousso
Comment 4 2015-07-10 23:56:20 PDT
Timothy Hatcher
Comment 5 2015-07-11 00:56:20 PDT
Comment on attachment 256650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256650&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1076 > + if (/^(?:\d+)$/.test(property.value)) { Why does it need to be a no capturing group?
Timothy Hatcher
Comment 6 2015-07-11 00:57:17 PDT
Comment on attachment 256650 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256650&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1079 > + title: WebInspector.UIString("The value '%s' needs units.\nClick to add 'px' to the value.").format(property.value), All the single quotes should be changed to curly (“”) double quotes. You should do that in a follow up patch.
WebKit Commit Bot
Comment 7 2015-07-11 01:43:43 PDT
Comment on attachment 256650 [details] Patch Clearing flags on attachment: 256650 Committed r186708: <http://trac.webkit.org/changeset/186708>
WebKit Commit Bot
Comment 8 2015-07-11 01:43:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.