128768 2014-02-13 14:41:37 -0800 Web Inspector: Color Picker doesn't update color of CSS property 2014-02-18 13:14:14 -0800 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) All All RESOLVED DUPLICATE 128965 InRadar P2 Normal --- 1 dpino webkit-unassigned chrisjshull joepeck timothy webkit-bug-importer oldest_to_newest 980462 0 dpino 2014-02-13 14:41:37 -0800 When choosing the color for a CSS property using the color picker, the first pick works fine but subsequent color picks don't update the color of the CSS property. Steps to reproduce (test file attached: test-red-color-square.html): 1. Open the Web Inspector, click on Inspect and select the red square. 2. In the Style side panel, click on the red swatch of the CSS property 'background-color'. The color picker will pop up. 3. Select a new color on the color circle. The color of the red square and the swatch get updated. 4. Without closing the pop-up pick a new color again. Expected result: the square and the swatch get updated. Actual result: the square and the swatch remain the same. 980464 1 webkit-bug-importer 2014-02-13 14:42:08 -0800 <rdar://problem/16063920> 980468 2 224113 dpino 2014-02-13 14:44:48 -0800 Created attachment 224113 Patch 980479 3 224113 dpino 2014-02-13 15:04:33 -0800 Comment on attachment 224113 Patch To be honest I don't really understand what are these markers :) I fixed the bug by checking what was inside the markers and suspecting that at least one of them has to be correct. First, an explanation about why the code is not currently working. The first time a color is picked, the conditional is false and that part is skipped. The second time a color is picked 'colorTextMarker.find()' is null, so it dives into the 'if'. The code tries to find a marker with a flag '__markedColor', which was set on the previous pick but what I noticed is that none of the markers returned by 'findMarksAt(range.from)' have this flag, so 'colorTextMarker' is null and the function exists (no update of color). Debugging the properties of the markers I noticed that the right marker is of type "range" (on the first pick colorTextMarker was of type "range") and doesn't have a property called '__cssProperty'. Interestingly, this marker is always the second marker of the array (at least in the example I tested), so maybe this code can be simplified further. I don't know whether the patch is correct but in case is not I hope it can bring some light to the right solution. 980960 4 timothy 2014-02-14 13:57:36 -0800 Interesting! I am curious why the range looses __markedColor. Overall I think the patch is correct and should work. However, it might fail in the future when we use markers for more things. Finding out why __markedColor is missing would be good. 981002 5 dpino 2014-02-14 14:45:13 -0800 Agree. I will try to take a further look into this. 981806 6 chrisjshull 2014-02-17 23:37:22 -0800 So I think I have this traced back. Any change in the CodeMirror editor for a CSSStyleDeclarationTextEditor updates the internal data model thru CSSAgent.setStyleText. This change percolates back as a refresh() on the DOMNodeStyles on the frontend, which then goes back to the backend with calls to CSSAgent.get*StylesForNode. The callbacks for this update() the CSSStyleDeclaration, which events WebInspector.CSSStyleDeclaration.Event.PropertiesChanged. CSSStyleDeclarationTextEditor picks up the event and calls _updateTextMarkers(), which first clears out all the existing text markers, then recreates the normal markers. In essence, any CSSStyleDeclarationTextEditor change blows away any temporary markers like the one that was being defined here. Hope that helps. Here are the JS traces. It goes between the backend and frontend, and there are a few setTimeouts as well: DOMNodeStyles.js:367: CONSOLE TRACE: 0: changeStyleText (DOMNodeStyles.js:367) 1: text (CSSStyleDeclaration.js:191) 2: _commitChanges (CSSStyleDeclarationTextEditor.js:273) 3: (unknown) ([native code]) DOMNodeStyles.js:246: CONSOLE TRACE: 0: refresh (DOMNodeStyles.js:246) 1: _refreshNodeStyles (StyleDetailsPanel.js:128) 2: _nodeStylesNeedsRefreshed (StyleDetailsPanel.js:159) 3: dispatch (Object.js:180) 4: dispatchEventToListeners (Object.js:187) 5: _markAsNeedsRefresh (DOMNodeStyles.js:867) 6: _styleSheetContentDidChange (DOMNodeStyles.js:883) 7: dispatch (Object.js:180) 8: dispatchEventToListeners (Object.js:187) 9: noteContentDidChange (CSSStyleSheet.js:150) 10: styleSheetChanged (CSSStyleManager.js:136) 11: styleSheetChanged (CSSObserver.js:43) 12: dispatch (InspectorBackend.js:262) 13: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) 14: (unknown) ([native code]) CSSStyleDeclaration.js:166: CONSOLE TRACE: 0: update (CSSStyleDeclaration.js:166) 1: _parseStyleDeclarationPayload (DOMNodeStyles.js:731) 2: _parseRulePayload (DOMNodeStyles.js:788) 3: parseRuleMatchArrayPayload (DOMNodeStyles.js:98) 4: fetchedMatchedStyles (DOMNodeStyles.js:121) 5: fetchedMatchedStyles ([native code]) 6: dispatch (InspectorBackend.js:220) 7: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) 8: (unknown) ([native code]) CSSStyleDeclarationTextEditor.js:496: CONSOLE TRACE: 0: clear (CSSStyleDeclarationTextEditor.js:496) 1: _clearTextMarkers (CSSStyleDeclarationTextEditor.js:521) 2: update (CSSStyleDeclarationTextEditor.js:320) 3: update ([native code]) 4: runInOp (External/CodeMirror/codemirror.js:1445) 5: operation (External/CodeMirror/codemirror.js:3223) 6: _updateTextMarkers (CSSStyleDeclarationTextEditor.js:386) 7: _propertiesChanged (CSSStyleDeclarationTextEditor.js:736) 8: dispatch (Object.js:180) 9: dispatchEventToListeners (Object.js:187) 10: delayed (CSSStyleDeclaration.js:164) 11: delayed ([native code]) CSSStyleDeclaration.js:166: CONSOLE TRACE: 0: update (CSSStyleDeclaration.js:166) 1: _parseStyleDeclarationPayload (DOMNodeStyles.js:731) 2: fetchedInlineStyles (DOMNodeStyles.js:153) 3: fetchedInlineStyles ([native code]) 4: dispatch (InspectorBackend.js:220) 5: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) 6: (unknown) ([native code]) CSSStyleDeclaration.js:166: CONSOLE TRACE: 0: update (CSSStyleDeclaration.js:166) 1: fetchedComputedStyle (DOMNodeStyles.js:173) 2: fetchedComputedStyle ([native code]) 3: dispatch (InspectorBackend.js:220) 4: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) 5: (unknown) ([native code]) CSSStyleDeclarationTextEditor.js:496: CONSOLE TRACE: 0: clear (CSSStyleDeclarationTextEditor.js:496) 1: _clearTextMarkers (CSSStyleDeclarationTextEditor.js:521) 2: update (CSSStyleDeclarationTextEditor.js:320) 3: update ([native code]) 4: runInOp (External/CodeMirror/codemirror.js:1445) 5: operation (External/CodeMirror/codemirror.js:3223) 6: _updateTextMarkers (CSSStyleDeclarationTextEditor.js:386) 7: _propertiesChanged (CSSStyleDeclarationTextEditor.js:736) 8: dispatch (Object.js:180) 9: dispatchEventToListeners (Object.js:187) 10: delayed (CSSStyleDeclaration.js:164) 11: delayed ([native code]) 981827 7 chrisjshull 2014-02-18 00:49:12 -0800 On line 644 there is this check: if (mark.type === "range" && !mark.__cssProperty) { I'm thinking that might be replaced this with this: if (WebInspector.TextMarker.textMarkerForCodeMirrorTextMarker(mark).type === WebInspector.TextMarker.Type.Color) { This might be less brittle to changes down the road. 981864 8 dpino 2014-02-18 03:23:03 -0800 (In reply to comment #6) > So I think I have this traced back. Any change in the CodeMirror editor for a CSSStyleDeclarationTextEditor updates the internal data model thru CSSAgent.setStyleText. This change percolates back as a refresh() on the DOMNodeStyles on the frontend, which then goes back to the backend with calls to CSSAgent.get*StylesForNode. The callbacks for this update() the CSSStyleDeclaration, which events WebInspector.CSSStyleDeclaration.Event.PropertiesChanged. CSSStyleDeclarationTextEditor picks up the event and calls _updateTextMarkers(), which first clears out all the existing text markers, then recreates the normal markers. > > In essence, any CSSStyleDeclarationTextEditor change blows away any temporary markers like the one that was being defined here. > > Hope that helps. > > Here are the JS traces. It goes between the backend and frontend, and there are a few setTimeouts as well: > > DOMNodeStyles.js:367: CONSOLE TRACE: > 0: changeStyleText (DOMNodeStyles.js:367) > 1: text (CSSStyleDeclaration.js:191) > 2: _commitChanges (CSSStyleDeclarationTextEditor.js:273) > 3: (unknown) ([native code]) > > DOMNodeStyles.js:246: CONSOLE TRACE: > 0: refresh (DOMNodeStyles.js:246) > 1: _refreshNodeStyles (StyleDetailsPanel.js:128) > 2: _nodeStylesNeedsRefreshed (StyleDetailsPanel.js:159) > 3: dispatch (Object.js:180) > 4: dispatchEventToListeners (Object.js:187) > 5: _markAsNeedsRefresh (DOMNodeStyles.js:867) > 6: _styleSheetContentDidChange (DOMNodeStyles.js:883) > 7: dispatch (Object.js:180) > 8: dispatchEventToListeners (Object.js:187) > 9: noteContentDidChange (CSSStyleSheet.js:150) > 10: styleSheetChanged (CSSStyleManager.js:136) > 11: styleSheetChanged (CSSObserver.js:43) > 12: dispatch (InspectorBackend.js:262) > 13: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) > 14: (unknown) ([native code]) > > CSSStyleDeclaration.js:166: CONSOLE TRACE: > 0: update (CSSStyleDeclaration.js:166) > 1: _parseStyleDeclarationPayload (DOMNodeStyles.js:731) > 2: _parseRulePayload (DOMNodeStyles.js:788) > 3: parseRuleMatchArrayPayload (DOMNodeStyles.js:98) > 4: fetchedMatchedStyles (DOMNodeStyles.js:121) > 5: fetchedMatchedStyles ([native code]) > 6: dispatch (InspectorBackend.js:220) > 7: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) > 8: (unknown) ([native code]) > > CSSStyleDeclarationTextEditor.js:496: CONSOLE TRACE: > 0: clear (CSSStyleDeclarationTextEditor.js:496) > 1: _clearTextMarkers (CSSStyleDeclarationTextEditor.js:521) > 2: update (CSSStyleDeclarationTextEditor.js:320) > 3: update ([native code]) > 4: runInOp (External/CodeMirror/codemirror.js:1445) > 5: operation (External/CodeMirror/codemirror.js:3223) > 6: _updateTextMarkers (CSSStyleDeclarationTextEditor.js:386) > 7: _propertiesChanged (CSSStyleDeclarationTextEditor.js:736) > 8: dispatch (Object.js:180) > 9: dispatchEventToListeners (Object.js:187) > 10: delayed (CSSStyleDeclaration.js:164) > 11: delayed ([native code]) > > CSSStyleDeclaration.js:166: CONSOLE TRACE: > 0: update (CSSStyleDeclaration.js:166) > 1: _parseStyleDeclarationPayload (DOMNodeStyles.js:731) > 2: fetchedInlineStyles (DOMNodeStyles.js:153) > 3: fetchedInlineStyles ([native code]) > 4: dispatch (InspectorBackend.js:220) > 5: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) > 6: (unknown) ([native code]) > > CSSStyleDeclaration.js:166: CONSOLE TRACE: > 0: update (CSSStyleDeclaration.js:166) > 1: fetchedComputedStyle (DOMNodeStyles.js:173) > 2: fetchedComputedStyle ([native code]) > 3: dispatch (InspectorBackend.js:220) > 4: dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:31) > 5: (unknown) ([native code]) > > CSSStyleDeclarationTextEditor.js:496: CONSOLE TRACE: > 0: clear (CSSStyleDeclarationTextEditor.js:496) > 1: _clearTextMarkers (CSSStyleDeclarationTextEditor.js:521) > 2: update (CSSStyleDeclarationTextEditor.js:320) > 3: update ([native code]) > 4: runInOp (External/CodeMirror/codemirror.js:1445) > 5: operation (External/CodeMirror/codemirror.js:3223) > 6: _updateTextMarkers (CSSStyleDeclarationTextEditor.js:386) > 7: _propertiesChanged (CSSStyleDeclarationTextEditor.js:736) > 8: dispatch (Object.js:180) > 9: dispatchEventToListeners (Object.js:187) > 10: delayed (CSSStyleDeclaration.js:164) > 11: delayed ([native code]) Good job! 981866 9 dpino 2014-02-18 03:25:25 -0800 (In reply to comment #7) > On line 644 there is this check: > if (mark.type === "range" && !mark.__cssProperty) { > > I'm thinking that might be replaced this with this: > if (WebInspector.TextMarker.textMarkerForCodeMirrorTextMarker(mark).type === WebInspector.TextMarker.Type.Color) { > > This might be less brittle to changes down the road. Tested it and it works. I agree is better this way. 982053 10 timothy 2014-02-18 13:10:46 -0800 Antoine fixed this in bug 128965. *** This bug has been marked as a duplicate of bug 128965 *** 982056 11 timothy 2014-02-18 13:14:14 -0800 Thanks for investigating. 224113 2014-02-13 14:44:48 -0800 2014-02-18 13:11:12 -0800 Patch bug-128768-20140213234446.patch text/plain 2627 dpino U3VidmVyc2lvbiBSZXZpc2lvbjogMTY0MDI4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCAw NzI0Y2IwMmUwYTMwM2Q0NmY3NGZkZmJlYTJjZjFlZjcxY2NlNzNhLi41MzgzNjY4NjVlYTFiZjhj OGZhMjhmMGY3ZjQxYWZlOTBlMDJhNzYwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNCBAQAorMjAxNC0wMi0xMyAgRGllZ28gUGlubyBHYXJjaWEgIDxkcGlub0BpZ2FsaWEu Y29tPgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IENvbG9yIFBpY2tlciBkb2Vzbid0IHVwZGF0 ZSBjb2xvciBvZiBDU1MgcHJvcGVydHkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEyODc2OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogVXNlckludGVyZmFjZS9DU1NTdHlsZURlY2xhcmF0aW9uVGV4dEVk aXRvci5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5DU1NTdHlsZURlY2xhcmF0aW9uVGV4dEVk aXRvci5wcm90b3R5cGUuX2NvbG9yU3dhdGNoQ2xpY2tlZC51cGRhdGVDb2RlTWlycm9yLnVwZGF0 ZSk6CisgICAgICAgIChXZWJJbnNwZWN0b3IuQ1NTU3R5bGVEZWNsYXJhdGlvblRleHRFZGl0b3Iu cHJvdG90eXBlLl9jb2xvclN3YXRjaENsaWNrZWQudXBkYXRlQ29kZU1pcnJvcik6CisKIDIwMTQt MDItMTEgIEJyaWFuIEJ1cmcgIDxiYnVyZ0BhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3Bl Y3RvcjogRE9NU3RvcmFnZVZpZXcgc2hvdWxkIGxpc3RlbiBmb3IgZXZlbnRzIGZyb20gRE9NU3Rv cmFnZU9iamVjdApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZh Y2UvQ1NTU3R5bGVEZWNsYXJhdGlvblRleHRFZGl0b3IuanMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvVXNlckludGVyZmFjZS9DU1NTdHlsZURlY2xhcmF0aW9uVGV4dEVkaXRvci5qcwppbmRleCBm YzVlM2VmNzA3YTdiMDc5OWRlNTFiOWE1ZDc3NTI3MGEwMDUxNzc3Li45NDc2ZDAwYmE0MGMyMDIx YThjYjRjYWQ5NmFjZjE2NjNhODBmMDBhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvVXNlckludGVyZmFjZS9DU1NTdHlsZURlY2xhcmF0aW9uVGV4dEVkaXRvci5qcworKysgYi9T b3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9DU1NTdHlsZURlY2xhcmF0aW9uVGV4 dEVkaXRvci5qcwpAQCAtNjQwLDE2ICs2NDAsMTEgQEAgV2ViSW5zcGVjdG9yLkNTU1N0eWxlRGVj bGFyYXRpb25UZXh0RWRpdG9yLnByb3RvdHlwZSA9IHsKICAgICAgICAgICAgICAgICBpZiAoIWNv bG9yVGV4dE1hcmtlciB8fCAhY29sb3JUZXh0TWFya2VyLmZpbmQoKSkgewogICAgICAgICAgICAg ICAgICAgICBjb2xvclRleHRNYXJrZXIgPSBudWxsOwogCi0gICAgICAgICAgICAgICAgICAgIHZh ciBtYXJrcyA9IHRoaXMuX2NvZGVNaXJyb3IuZmluZE1hcmtzQXQocmFuZ2UuZnJvbSk7Ci0gICAg ICAgICAgICAgICAgICAgIGlmICghbWFya3MubGVuZ3RoKQotICAgICAgICAgICAgICAgICAgICAg ICAgcmV0dXJuOwotCi0gICAgICAgICAgICAgICAgICAgIGZvciAodmFyIGkgPSAwOyBpIDwgbWFy a3MubGVuZ3RoOyArK2kpIHsKLSAgICAgICAgICAgICAgICAgICAgICAgIHZhciBtYXJrID0gbWFy a3NbaV07Ci0gICAgICAgICAgICAgICAgICAgICAgICBpZiAoIW1hcmsuX19tYXJrZWRDb2xvcikK LSAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb250aW51ZTsKLSAgICAgICAgICAgICAgICAg ICAgICAgIGNvbG9yVGV4dE1hcmtlciA9IG1hcms7Ci0gICAgICAgICAgICAgICAgICAgICAgICBi cmVhazsKKyAgICAgICAgICAgICAgICAgICAgZm9yICh2YXIgbWFyayBvZiB0aGlzLl9jb2RlTWly cm9yLmZpbmRNYXJrc0F0KHJhbmdlLmZyb20pKSB7CisgICAgICAgICAgICAgICAgICAgICAgICBp ZiAobWFyay50eXBlID09PSAicmFuZ2UiICYmICFtYXJrLl9fY3NzUHJvcGVydHkpIHsKKyAgICAg ICAgICAgICAgICAgICAgICAgICAgICBjb2xvclRleHRNYXJrZXIgPSBtYXJrOworICAgICAgICAg ICAgICAgICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgICAgICAgICAgICAgfQogICAg ICAgICAgICAgICAgICAgICB9CiAgICAgICAgICAgICAgICAgfQogCkBAIC02NzIsNyArNjY3LDYg QEAgV2ViSW5zcGVjdG9yLkNTU1N0eWxlRGVjbGFyYXRpb25UZXh0RWRpdG9yLnByb3RvdHlwZSA9 IHsKICAgICAgICAgICAgICAgICByYW5nZS50by5jaCA9IHJhbmdlLmZyb20uY2ggKyBuZXdDb2xv clRleHQubGVuZ3RoOwogCiAgICAgICAgICAgICAgICAgY29sb3JUZXh0TWFya2VyID0gdGhpcy5f Y29kZU1pcnJvci5tYXJrVGV4dChyYW5nZS5mcm9tLCByYW5nZS50byk7Ci0gICAgICAgICAgICAg ICAgY29sb3JUZXh0TWFya2VyLl9fbWFya2VkQ29sb3IgPSB0cnVlOwogCiAgICAgICAgICAgICAg ICAgc3dhdGNoLl9fY29sb3JUZXh0TWFya2VyID0gY29sb3JUZXh0TWFya2VyOwogICAgICAgICAg ICAgfQo=