Web Inspector: Color Picker doesn't update color of CSS property

[Diego Pino]

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.

[Radar WebKit Bug Importer]

<rdar://problem/16063920>

[Diego Pino]

Created attachment 224113 [details]
Patch

[Diego Pino]

Comment on attachment 224113 [details]
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.

[Timothy Hatcher]

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.

[Diego Pino]

Agree. I will try to take a further look into this.

[Chris J. Shull]

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])

[Chris J. Shull]

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.

[Diego Pino]

(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!

[Diego Pino]

(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.

[Timothy Hatcher]

Antoine fixed this in bug 128965.

*** This bug has been marked as a duplicate of bug 128965 ***

[Timothy Hatcher]

Thanks for investigating.