Bug 148580

Summary: Web Inspector: Visual style editor shouldn't allow alpha characters in numeric input fields
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=148678
Attachments:
Description Flags
[Patch] Proposed Fix none

Description Matt Baker 2015-08-28 10:59:15 PDT
* 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"
Comment 1 Radar WebKit Bug Importer 2015-08-28 11:00:08 PDT
<rdar://problem/22478271>
Comment 2 Matt Baker 2015-08-30 17:35:39 PDT
Created attachment 260257 [details]
[Patch] Proposed Fix
Comment 3 BJ Burg 2015-08-31 06:14:08 PDT
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.
Comment 4 Matt Baker 2015-08-31 11:06:03 PDT
(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.
Comment 5 BJ Burg 2015-08-31 11:52:52 PDT
(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).
Comment 6 Matt Baker 2015-08-31 12:01:01 PDT
(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.
Comment 7 BJ Burg 2015-09-01 08:41:44 PDT
Comment on attachment 260257 [details]
[Patch] Proposed Fix

r=me
Comment 8 WebKit Commit Bot 2015-09-01 09:29:53 PDT
Comment on attachment 260257 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 260257

Committed r189210: <http://trac.webkit.org/changeset/189210>
Comment 9 WebKit Commit Bot 2015-09-01 09:30:08 PDT
All reviewed patches have been landed.  Closing bug.