Bug 148580 – Web Inspector: Visual style editor shouldn't allow alpha characters in numeric input fields

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Visual style editor shouldn't allow alpha characters in numeric input fields.

* STEPS TO REPRODUCE
1. Open Elements tab > Styles - Visual
2. Select an element in the DOM tree
2. Expand Layout > Position
3. Change Top to "px"
4. Type some non-numeric characters: "abc"
  => Element is updated with style="top: abcpx"

[Radar WebKit Bug Importer]

<rdar://problem/22478271>

[Matt Baker]

Created attachment 260257 [details]
[Patch] Proposed Fix

[Blaze Burg]

Comment on attachment 260257 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=260257&action=review

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:384
> +            newValue = this.placeholder && !isNaN(this.placeholder) ? parseFloat(this.placeholder) : 0;

So if a user had typed in '50', then switched to 'abc', the value will now be '0'? It seems an early break is better if there's a non-placeholder value present.

[Matt Baker]

(In reply to comment #3)
> Comment on attachment 260257 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260257&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:384
> > +            newValue = this.placeholder && !isNaN(this.placeholder) ? parseFloat(this.placeholder) : 0;
> 
> So if a user had typed in '50', then switched to 'abc', the value will now
> be '0'? It seems an early break is better if there's a non-placeholder value
> present.

By the time the input control's change event fires we've lost the previous value. VisualStyleNumberInputBox's value property is a wrapper around the input's value attribute, so we'd need to store the value separately to be able to revert to the last valid state.

[Blaze Burg]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 260257 [details]
> > [Patch] Proposed Fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=260257&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:384
> > > +            newValue = this.placeholder && !isNaN(this.placeholder) ? parseFloat(this.placeholder) : 0;
> > 
> > So if a user had typed in '50', then switched to 'abc', the value will now
> > be '0'? It seems an early break is better if there's a non-placeholder value
> > present.
> 
> By the time the input control's change event fires we've lost the previous
> value. VisualStyleNumberInputBox's value property is a wrapper around the
> input's value attribute, so we'd need to store the value separately to be
> able to revert to the last valid state.

And this is why we shouldn't store state in DOM elements! :_)

I suppose that could be a separate bug, if it will get too far out of scope (or the change has to be applied to every widget class).

[Matt Baker]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 260257 [details]
> > > [Patch] Proposed Fix
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=260257&action=review
> > > 
> > > > Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:384
> > > > +            newValue = this.placeholder && !isNaN(this.placeholder) ? parseFloat(this.placeholder) : 0;
> > > 
> > > So if a user had typed in '50', then switched to 'abc', the value will now
> > > be '0'? It seems an early break is better if there's a non-placeholder value
> > > present.
> > 
> > By the time the input control's change event fires we've lost the previous
> > value. VisualStyleNumberInputBox's value property is a wrapper around the
> > input's value attribute, so we'd need to store the value separately to be
> > able to revert to the last valid state.
> 
> And this is why we shouldn't store state in DOM elements! :_)

Indeed.

> I suppose that could be a separate bug, if it will get too far out of scope
> (or the change has to be applied to every widget class).

The intention was simply to prevent setting invalid styles like "margin: abcpx". I think further refinements to the editing experience should go under a separate bug.

[Blaze Burg]

Comment on attachment 260257 [details]
[Patch] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 260257 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 260257

Committed r189210: <http://trac.webkit.org/changeset/189210>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.