Bug 146859 - Web Inspector: Warning icon tooltip for numbers with no units could be improved
Summary: Web Inspector: Warning icon tooltip for numbers with no units could be improved
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-10 16:01 PDT by Timothy Hatcher
Modified: 2015-07-11 01:43 PDT (History)
8 users (show)

See Also:


Attachments
Screenshot (61.45 KB, image/png)
2015-07-10 16:01 PDT, Timothy Hatcher
no flags Details
Patch (3.81 KB, patch)
2015-07-10 19:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (3.82 KB, patch)
2015-07-10 23:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 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).
Comment 1 Radar WebKit Bug Importer 2015-07-10 16:03:01 PDT
<rdar://problem/21776057>
Comment 2 Devin Rousso 2015-07-10 19:19:05 PDT
Created attachment 256637 [details]
Patch
Comment 3 Timothy Hatcher 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+$/?
Comment 4 Devin Rousso 2015-07-10 23:56:20 PDT
Created attachment 256650 [details]
Patch
Comment 5 Timothy Hatcher 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?
Comment 6 Timothy Hatcher 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.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2015-07-11 01:43:47 PDT
All reviewed patches have been landed.  Closing bug.