Bug 146859 – Web Inspector: Warning icon tooltip for numbers with no units could be improved

• Component: Web Inspector
• Status: RESOLVED FIXED

[Timothy Hatcher]

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).

[Radar WebKit Bug Importer]

<rdar://problem/21776057>

[Devin Rousso]

Created attachment 256637 [details]
Patch

[Timothy Hatcher]

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]

Created attachment 256650 [details]
Patch

[Timothy Hatcher]

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 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 on attachment 256650 [details]
Patch

Clearing flags on attachment: 256650

Committed r186708: <http://trac.webkit.org/changeset/186708>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.