* 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"
<rdar://problem/22478271>
Created attachment 260257 [details] [Patch] Proposed Fix
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.
(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.
(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).
(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 on attachment 260257 [details] [Patch] Proposed Fix r=me
Comment on attachment 260257 [details] [Patch] Proposed Fix Clearing flags on attachment: 260257 Committed r189210: <http://trac.webkit.org/changeset/189210>
All reviewed patches have been landed. Closing bug.