128965 2014-02-18 03:25:12 -0800 Web Inspector: editing a color in the Styles sidebar using the color picker only works once for a given color 2014-02-18 13:39:40 -0800 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 graouts graouts commit-queue dpino graouts joepeck timothy webkit-bug-importer oldest_to_newest 981865 0 graouts 2014-02-18 03:25:12 -0800 Steps to reproduce: 1. select any DOM node with a style property set to a color value 2. show the Styles sidebar 3. click on the color swatch for that value 4. click anywhere on the color wheel, notice how the color in the editor is changed to match 5. click anywhere on the color wheel, notice how the color _does not_ change to match in the editor I believe the CodeMirror text marker managed by the WebInspector.TextMarker object is no longer present when we try to update its text from the WebInspector.CodeMirrorColorEditingController. 981867 1 webkit-bug-importer 2014-02-18 03:25:38 -0800 <rdar://problem/16096027> 981870 2 graouts 2014-02-18 03:32:19 -0800 The CSSStyleDeclarationTextEditor does not use a CodeMirrorColorEditingController. However, the issue is that in the nested function updateCodeMirror() in CSSStyleDeclarationTextEditor.prototype._colorSwatchClicked() we do not find a valid colorTextMarker past the first update and thus we cannot update the text in the editor. 981877 3 224490 graouts 2014-02-18 03:53:19 -0800 Created attachment 224490 Patch 981921 4 224490 timothy 2014-02-18 07:25:38 -0800 Comment on attachment 224490 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224490&action=review > Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:649 > + if (WebInspector.TextMarker.textMarkerForCodeMirrorTextMarker(mark).type !== WebInspector.TextMarker.Type.Color) Where does the color type get set? The market on line 674 is just a CodeMirror marker. > Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:674 > colorTextMarker = this._codeMirror.markText(range.from, range.to); Doesn't this need to set the type using our API to mark the text? 981928 5 graouts 2014-02-18 07:39:32 -0800 (In reply to comment #4) > (From update of attachment 224490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224490&action=review > > > Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:649 > > + if (WebInspector.TextMarker.textMarkerForCodeMirrorTextMarker(mark).type !== WebInspector.TextMarker.Type.Color) > > Where does the color type get set? The market on line 674 is just a CodeMirror marker. It's set in the CodeMirror extension method .createColorMarkers() which is called from CSSStyleDeclarationTextEditor in ._createColorSwatches(). > > Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:674 > > colorTextMarker = this._codeMirror.markText(range.from, range.to); > > Doesn't this need to set the type using our API to mark the text? Not anymore, we'll get the type from the WebInspector.TextMarker created for this CodeMirror TextMarker. 982054 6 timothy 2014-02-18 13:10:46 -0800 *** Bug 128768 has been marked as a duplicate of this bug. *** 982066 7 224490 commit-queue 2014-02-18 13:39:38 -0800 Comment on attachment 224490 Patch Clearing flags on attachment: 224490 Committed r164312: <http://trac.webkit.org/changeset/164312> 982067 8 commit-queue 2014-02-18 13:39:40 -0800 All reviewed patches have been landed. Closing bug. 224490 2014-02-18 03:53:19 -0800 2014-02-18 13:39:38 -0800 Patch bug-128965-20140218125317.patch text/plain 2551 graouts U3VidmVyc2lvbiBSZXZpc2lvbjogMTY0MjM3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCAx MzYxMmNhN2NjYWZjNTA0MGY1ZWZiMGI0MWY4OWIwMWZmOTlhMGZjLi5hODY0YTZiYTlhZTU3ZTg2 YzI5NTc3NzY1YTMyNDViOGIwYzcyMzY4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxNC0wMi0xOCAgQW50b2luZSBRdWludCAgPGdyYW91dHNAd2Via2l0Lm9y Zz4KKworICAgICAgICBXZWIgSW5zcGVjdG9yOiBlZGl0aW5nIGEgY29sb3IgaW4gdGhlIFN0eWxl cyBzaWRlYmFyIHVzaW5nIHRoZSBjb2xvciBwaWNrZXIgb25seSB3b3JrcyBvbmNlIGZvciBhIGdp dmVuIGNvbG9yCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMjg5NjUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBVc2UgdGhlIFdlYkluc3BlY3Rvci5UZXh0TWFya2VyIGJhY2tpbmcgdGhlIENvZGVNaXJyb3Ig VGV4dE1hcmtlciBvYmplY3QgdG8gaW5zcGVjdCBpdHMgdHlwZSBpbgorICAgICAgICBvcmRlciB0 byBpZGVudGlmeSBjb2xvciBtYXJrZXJzIHRoYXQgd2VyZSBjcmVhdGVkIGZvciBhIGNvbG9yLiBU aGlzIGZpeGVzIGEgcmVncmVzc2lvbiBpbnRyb2R1Y2VkCisgICAgICAgIGluIGh0dHA6Ly93ZWJr aXQub3JnL2IvMTI1Njk1IHdoZW4gd2UgYWJzdHJhY3RlZCBDb2RlTWlycm9yIFRleHRNYXJrZXIg b2JqZWN0cy4KKworICAgICAgICAqIFVzZXJJbnRlcmZhY2UvQ1NTU3R5bGVEZWNsYXJhdGlvblRl eHRFZGl0b3IuanM6CisgICAgICAgIChXZWJJbnNwZWN0b3IuQ1NTU3R5bGVEZWNsYXJhdGlvblRl eHRFZGl0b3IucHJvdG90eXBlLl9jb2xvclN3YXRjaENsaWNrZWQudXBkYXRlQ29kZU1pcnJvci51 cGRhdGUpOgorICAgICAgICAoV2ViSW5zcGVjdG9yLkNTU1N0eWxlRGVjbGFyYXRpb25UZXh0RWRp dG9yLnByb3RvdHlwZS5fY29sb3JTd2F0Y2hDbGlja2VkLnVwZGF0ZUNvZGVNaXJyb3IpOgorCiAy MDE0LTAyLTE3ICBBbnRvaW5lIFF1aW50ICA8Z3Jhb3V0c0B3ZWJraXQub3JnPgogCiAgICAgICAg IFdlYiBJbnNwZWN0b3I6IENTUyBzZWxlY3RvcnMgY29udGFpbmluZyBhIGNvbG9yIG5hbWUgc2hv dWxkbid0IGJlIGNvbnNpZGVyZWQgZm9yIGNvbG9yIGVkaXRpbmcKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL0NTU1N0eWxlRGVjbGFyYXRpb25UZXh0RWRp dG9yLmpzIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvQ1NTU3R5bGVEZWNs YXJhdGlvblRleHRFZGl0b3IuanMKaW5kZXggZmM1ZTNlZjcwN2E3YjA3OTlkZTUxYjlhNWQ3NzUy NzBhMDA1MTc3Ny4uNTkxMmMzNmFkMTk0NjYxMGZiYWU3NGM1NDA0MzQ2OTU3MzM2OTE5NiAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvQ1NTU3R5bGVEZWNs YXJhdGlvblRleHRFZGl0b3IuanMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRl cmZhY2UvQ1NTU3R5bGVEZWNsYXJhdGlvblRleHRFZGl0b3IuanMKQEAgLTY0Niw3ICs2NDYsNyBA QCBXZWJJbnNwZWN0b3IuQ1NTU3R5bGVEZWNsYXJhdGlvblRleHRFZGl0b3IucHJvdG90eXBlID0g ewogCiAgICAgICAgICAgICAgICAgICAgIGZvciAodmFyIGkgPSAwOyBpIDwgbWFya3MubGVuZ3Ro OyArK2kpIHsKICAgICAgICAgICAgICAgICAgICAgICAgIHZhciBtYXJrID0gbWFya3NbaV07Ci0g ICAgICAgICAgICAgICAgICAgICAgICBpZiAoIW1hcmsuX19tYXJrZWRDb2xvcikKKyAgICAgICAg ICAgICAgICAgICAgICAgIGlmIChXZWJJbnNwZWN0b3IuVGV4dE1hcmtlci50ZXh0TWFya2VyRm9y Q29kZU1pcnJvclRleHRNYXJrZXIobWFyaykudHlwZSAhPT0gV2ViSW5zcGVjdG9yLlRleHRNYXJr ZXIuVHlwZS5Db2xvcikKICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb250aW51ZTsKICAg ICAgICAgICAgICAgICAgICAgICAgIGNvbG9yVGV4dE1hcmtlciA9IG1hcms7CiAgICAgICAgICAg ICAgICAgICAgICAgICBicmVhazsKQEAgLTY3Miw3ICs2NzIsNiBAQCBXZWJJbnNwZWN0b3IuQ1NT U3R5bGVEZWNsYXJhdGlvblRleHRFZGl0b3IucHJvdG90eXBlID0gewogICAgICAgICAgICAgICAg IHJhbmdlLnRvLmNoID0gcmFuZ2UuZnJvbS5jaCArIG5ld0NvbG9yVGV4dC5sZW5ndGg7CiAKICAg ICAgICAgICAgICAgICBjb2xvclRleHRNYXJrZXIgPSB0aGlzLl9jb2RlTWlycm9yLm1hcmtUZXh0 KHJhbmdlLmZyb20sIHJhbmdlLnRvKTsKLSAgICAgICAgICAgICAgICBjb2xvclRleHRNYXJrZXIu X19tYXJrZWRDb2xvciA9IHRydWU7CiAKICAgICAgICAgICAgICAgICBzd2F0Y2guX19jb2xvclRl eHRNYXJrZXIgPSBjb2xvclRleHRNYXJrZXI7CiAgICAgICAgICAgICB9Cg==