Summary: | Web Inspector: Visual style editor shouldn't allow alpha characters in numeric input fields | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||
Component: | Web Inspector | Assignee: | 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
Matt Baker
2015-08-28 10:59:15 PDT
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. |