148580 2015-08-28 10:59:15 -0700 Web Inspector: Visual style editor shouldn't allow alpha characters in numeric input fields 2015-09-01 09:30:08 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=148678 InRadar P2 Normal --- 1 mattbaker mattbaker bburg commit-queue graouts joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1121752 0 mattbaker 2015-08-28 10:59:15 -0700 * 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" 1121755 1 webkit-bug-importer 2015-08-28 11:00:08 -0700 <rdar://problem/22478271> 1122092 2 260257 mattbaker 2015-08-30 17:35:39 -0700 Created attachment 260257 [Patch] Proposed Fix 1122148 3 260257 bburg 2015-08-31 06:14:08 -0700 Comment on attachment 260257 [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. 1122217 4 mattbaker 2015-08-31 11:06:03 -0700 (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. 1122240 5 bburg 2015-08-31 11:52:52 -0700 (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). 1122243 6 mattbaker 2015-08-31 12:01:01 -0700 (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. 1122542 7 260257 bburg 2015-09-01 08:41:44 -0700 Comment on attachment 260257 [Patch] Proposed Fix r=me 1122551 8 260257 commit-queue 2015-09-01 09:29:53 -0700 Comment on attachment 260257 [Patch] Proposed Fix Clearing flags on attachment: 260257 Committed r189210: <http://trac.webkit.org/changeset/189210> 1122552 9 commit-queue 2015-09-01 09:30:08 -0700 All reviewed patches have been landed. Closing bug. 260257 2015-08-30 17:35:39 -0700 2015-09-01 09:29:53 -0700 [Patch] Proposed Fix bug-148580-20150830173523.patch text/plain 3397 mattbaker U3VidmVyc2lvbiBSZXZpc2lvbjogMTg5MTYxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCA2 ZmE1MGZmYThjNWQxODhkNGE4YjIxODEyZmUwZTFjMDhlNmQ4OWU3Li5lYTY2MWY3ZDU4MGE2MTY0 Y2IzMGQ0YTdlNjU0YzFlNGVjNDk1MGQ3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxNS0wOC0zMCAgTWF0dCBCYWtlciAgPG1hdHRiYWtlckBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogVmlzdWFsIHN0eWxlIGVkaXRvciBzaG91bGRuJ3Qg YWxsb3cgYWxwaGEgY2hhcmFjdGVycyBpbiBudW1lcmljIGlucHV0IGZpZWxkcworICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ4NTgwCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1Zp ZXdzL1Zpc3VhbFN0eWxlTnVtYmVySW5wdXRCb3guanM6CisgICAgICAgIChXZWJJbnNwZWN0b3Iu VmlzdWFsU3R5bGVOdW1iZXJJbnB1dEJveCk6CisgICAgICAgIEFkZGVkIGlucHV0IGVsZW1lbnQg ImNoYW5nZWQiIGhhbmRsZXIsIHJlbmFtZWQgImtleXVwIiBoYW5kbGVyIHRvIG1hdGNoICJrZXlk b3duIiBoYW5kbGVyLgorICAgICAgICAoV2ViSW5zcGVjdG9yLlZpc3VhbFN0eWxlTnVtYmVySW5w dXRCb3gucHJvdG90eXBlLl92YWx1ZU51bWJlcklucHV0Q2hhbmdlZCk6CisgICAgICAgIFZhbGlk YXRlIGlucHV0IGluIHJlc3BvbnNlIHRvIGNoYW5nZWQgZXZlbnQuCisKIDIwMTUtMDgtMjggIEpv c2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KIAogICAgICAgICBXZWIgSW5zcGVj dG9yOiAiYW5pbWF0aW9uRW5kIiBldmVudCBuYW1lcyBzaG91bGQgYmUgImFuaW1hdGlvbmVuZCIg KGJyb2tlbiBkYXNoYm9hcmQgYW5pbWF0aW9uIGFmdGVyIHBhdXNlKQpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvVmlzdWFsU3R5bGVOdW1iZXJJ bnB1dEJveC5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1Zp c3VhbFN0eWxlTnVtYmVySW5wdXRCb3guanMKaW5kZXggZWMzYmUxNDgyMDllYjMwMzdjYTNkZjg5 OWI3NDFhYjRjZmJlYmY3MS4uMDFkZDg0NGE2NDFiNzQ3YzE4MTE2Yjg1ZmU5ZjllZjcwYjNmNTIy OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3Mv VmlzdWFsU3R5bGVOdW1iZXJJbnB1dEJveC5qcworKysgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkv VXNlckludGVyZmFjZS9WaWV3cy9WaXN1YWxTdHlsZU51bWJlcklucHV0Qm94LmpzCkBAIC03NSw4 ICs3NSw5IEBAIFdlYkluc3BlY3Rvci5WaXN1YWxTdHlsZU51bWJlcklucHV0Qm94ID0gY2xhc3Mg VmlzdWFsU3R5bGVOdW1iZXJJbnB1dEJveCBleHRlbmRzCiAgICAgICAgIHRoaXMuX3ZhbHVlTnVt YmVySW5wdXRFbGVtZW50LnNwZWxsY2hlY2sgPSBmYWxzZTsKICAgICAgICAgdGhpcy5fdmFsdWVO dW1iZXJJbnB1dEVsZW1lbnQuYWRkRXZlbnRMaXN0ZW5lcigiZm9jdXMiLCB0aGlzLl9mb2N1c0Nv bnRlbnRFbGVtZW50LmJpbmQodGhpcykpOwogICAgICAgICB0aGlzLl92YWx1ZU51bWJlcklucHV0 RWxlbWVudC5hZGRFdmVudExpc3RlbmVyKCJrZXlkb3duIiwgdGhpcy5fdmFsdWVOdW1iZXJJbnB1 dEtleURvd24uYmluZCh0aGlzKSk7Ci0gICAgICAgIHRoaXMuX3ZhbHVlTnVtYmVySW5wdXRFbGVt ZW50LmFkZEV2ZW50TGlzdGVuZXIoImtleXVwIiwgdGhpcy5fbnVtYmVySW5wdXRDaGFuZ2VkLmJp bmQodGhpcykpOworICAgICAgICB0aGlzLl92YWx1ZU51bWJlcklucHV0RWxlbWVudC5hZGRFdmVu dExpc3RlbmVyKCJrZXl1cCIsIHRoaXMuX3ZhbHVlTnVtYmVySW5wdXRLZXlVcC5iaW5kKHRoaXMp KTsKICAgICAgICAgdGhpcy5fdmFsdWVOdW1iZXJJbnB1dEVsZW1lbnQuYWRkRXZlbnRMaXN0ZW5l cigiYmx1ciIsIHRoaXMuX2JsdXJDb250ZW50RWxlbWVudC5iaW5kKHRoaXMpKTsKKyAgICAgICAg dGhpcy5fdmFsdWVOdW1iZXJJbnB1dEVsZW1lbnQuYWRkRXZlbnRMaXN0ZW5lcigiY2hhbmdlIiwg dGhpcy5fdmFsdWVOdW1iZXJJbnB1dENoYW5nZWQuYmluZCh0aGlzKSk7CiAgICAgICAgIHRoaXMu X251bWJlclVuaXRzQ29udGFpbmVyLmFwcGVuZENoaWxkKHRoaXMuX3ZhbHVlTnVtYmVySW5wdXRF bGVtZW50KTsKIAogICAgICAgICB0aGlzLl91bml0c0VsZW1lbnQgPSBkb2N1bWVudC5jcmVhdGVF bGVtZW50KCJzcGFuIik7CkBAIC0zMDEsNyArMzAyLDcgQEAgV2ViSW5zcGVjdG9yLlZpc3VhbFN0 eWxlTnVtYmVySW5wdXRCb3ggPSBjbGFzcyBWaXN1YWxTdHlsZU51bWJlcklucHV0Qm94IGV4dGVu ZHMKICAgICAgICAgdGhpcy5fdmFsdWVEaWRDaGFuZ2UoKTsKICAgICB9CiAKLSAgICBfbnVtYmVy SW5wdXRDaGFuZ2VkKCkKKyAgICBfdmFsdWVOdW1iZXJJbnB1dEtleVVwKGV2ZW50KQogICAgIHsK ICAgICAgICAgaWYgKCF0aGlzLl9udW1iZXJJbnB1dElzRWRpdGFibGUpCiAgICAgICAgICAgICBy ZXR1cm47CkBAIC0zNzYsNiArMzc3LDE5IEBAIFdlYkluc3BlY3Rvci5WaXN1YWxTdHlsZU51bWJl cklucHV0Qm94ID0gY2xhc3MgVmlzdWFsU3R5bGVOdW1iZXJJbnB1dEJveCBleHRlbmRzCiAgICAg ICAgIHRoaXMuY29udGVudEVsZW1lbnQuY2xhc3NMaXN0LnJlbW92ZSgiZm9jdXNlZCIpOwogICAg IH0KIAorICAgIF92YWx1ZU51bWJlcklucHV0Q2hhbmdlZChldmVudCkKKyAgICB7CisgICAgICAg IGxldCBuZXdWYWx1ZSA9IHRoaXMudmFsdWU7CisgICAgICAgIGlmICghbmV3VmFsdWUgJiYgaXNO YU4obmV3VmFsdWUpKQorICAgICAgICAgICAgbmV3VmFsdWUgPSB0aGlzLnBsYWNlaG9sZGVyICYm ICFpc05hTih0aGlzLnBsYWNlaG9sZGVyKSA/IHBhcnNlRmxvYXQodGhpcy5wbGFjZWhvbGRlcikg OiAwOworCisgICAgICAgIGlmICghdGhpcy5fYWxsb3dOZWdhdGl2ZVZhbHVlcyAmJiBuZXdWYWx1 ZSA8IDApCisgICAgICAgICAgICBuZXdWYWx1ZSA9IDA7CisKKyAgICAgICAgdGhpcy52YWx1ZSA9 IE1hdGgucm91bmQobmV3VmFsdWUgKiAxMDApIC8gMTAwOworICAgICAgICB0aGlzLl92YWx1ZURp ZENoYW5nZSgpOworICAgIH0KKwogICAgIF90b2dnbGVUYWJiaW5nT2ZTZWxlY3RhYmxlRWxlbWVu dHMoZGlzYWJsZWQpCiAgICAgewogICAgICAgICB0aGlzLl9rZXl3b3JkU2VsZWN0RWxlbWVudC50 YWJJbmRleCA9IGRpc2FibGVkID8gIi0xIiA6IG51bGw7Cg==