DevTools: Source color picker experiment. Make color swatches clickable

third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js

Index: third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
diff --git a/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js b/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
index 58bdf45e533b57dca5820d254a73a6d9d2db28a3..e0e30b78b3ec18015077c7751bc5aff6d358fd8d 100644
--- a/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
+++ b/third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js
@@ -39,6 +39,8 @@ WebInspector.CSSSourceFrame = function(uiSourceCode)
     this.textEditor.setAutocompleteDelegate(new WebInspector.CSSSourceFrame.AutocompleteDelegate());
     this._registerShortcuts();
     this._colorBookmarks = [];
+    this._swatchPopoverHelper = new WebInspector.SwatchPopoverHelper();
+    this._analyzeTextColors = true;

[lushnikov, 2016/07/19 21:12:16]

We usually call this kind of flags like muteXXX, e.g.:

this._muteColorProcessing = false;

[flandy, 2016/07/19 22:58:15]

Done.

 }
 
 /** @type {number} */
@@ -152,12 +154,8 @@ WebInspector.CSSSourceFrame.prototype = {
 
         for (var i = 0; i < colorPositions.length; i++) {
             var colorPosition = colorPositions[i];
-
-            var swatch = WebInspector.ColorSwatch.create();
-            swatch.setColorText(colorPosition.color.asString(WebInspector.Color.Format.Original));
-            swatch.hideText(true);
-
-            var bookmark = this.textEditor.addBookmark(colorPosition.textRange.startLine, colorPosition.textRange.startColumn, swatch);
+            var swatch = new WebInspector.CSSSourceFrame.ColorSwatch(this, colorPosition);
+            var bookmark = this.textEditor.addBookmark(colorPosition.textRange.startLine, colorPosition.textRange.startColumn, swatch.getElement());
             this._colorBookmarks.push(bookmark);
         }
     },
@@ -170,12 +168,71 @@ WebInspector.CSSSourceFrame.prototype = {
     },
 
     /**
+     * @param {!WebInspector.ColorSwatch} swatch
+     */
+    _showSpectrum: function(swatch)
+    {
+        if (this._swatchPopoverHelper.isShowing()) {
+            this._swatchPopoverHelper.hide();
+            return;
+        }
+        var color = swatch.color();
+        var format = swatch.format();
+        if (format === WebInspector.Color.Format.Original)
+            format = color.format();
+
+        this._spectrum = new WebInspector.Spectrum();
+        this._spectrum.setColor(color, format);
+        this._spectrum.addEventListener(WebInspector.Spectrum.Events.SizeChanged, this._spectrumResized, this);

[lushnikov, 2016/07/19 21:12:16]

When does this event fire?

[flandy, 2016/07/19 22:58:15]

This event fires when changing the color palette of the spectrum or adding a
color to the custom color palette. The reposition deals with growing or
shrinking the size of the palette.

+        this._spectrum.addEventListener(WebInspector.Spectrum.Events.ColorChanged, this._spectrumChanged, this);
+        this._swatchPopoverHelper.show(this._spectrum, swatch.iconElement(), this._spectrumHidden.bind(this));
+    },
+
+    /**
+     * @param {!WebInspector.Event} event
+     */
+    _spectrumResized: function(event)
+    {
+        this._swatchPopoverHelper.reposition();
+    },
+
+    /**
+     * @param {!WebInspector.Event} event
+     */
+    _spectrumChanged: function(event)
+    {
+        this._analyzeTextColors = false;
+        var colorString = /** @type {string} */ (event.data);
+        this._currentSwatch.setColorText(colorString);
+        this._replaceColorText(this._currentSwatch.getColorPosition(), colorString);
+    },
+
+    _spectrumHidden: function()
+    {
+        this._analyzeTextColors = true;
+        this._spectrum.removeEventListener(WebInspector.Spectrum.Events.SizeChanged, this._spectrumResized, this);
+        this._spectrum.removeEventListener(WebInspector.Spectrum.Events.ColorChanged, this._spectrumChanged, this);
+        delete this._spectrum;

[lushnikov, 2016/07/19 21:12:16]

you should schedule updateColorSwatches here

[flandy, 2016/07/19 22:58:15]

Why? We already change the color and position of the existing color swatch when
the spectrum is changed. There should be no need to also update the swatches
when the spectrum is hidden. 

[lushnikov, 2016/07/20 00:49:23]

Imagine you have multiple color swatches in the line and edit the first one

[flandy, 2016/07/20 01:56:18]

Yes, good point! Done.

+    },
+
+    /**
+     * @param {!WebInspector.CSSSourceFrame.ColorPosition} colorPosition
+     * @param {string} colorString
+     */
+    _replaceColorText: function(colorPosition, colorString)
+    {
+        this._textEditor.editRange(colorPosition.textRange, colorString);
+        colorPosition.color = WebInspector.Color.parse(colorString);
+        colorPosition.textRange.endColumn = colorPosition.textRange.startColumn + colorString.length;
+    },
+
+    /**
      * @override
      */
     onTextEditorContentSet: function()
     {
         WebInspector.UISourceCodeFrame.prototype.onTextEditorContentSet.call(this);
-        if (Runtime.experiments.isEnabled("sourceColorPicker"))
+        if (Runtime.experiments.isEnabled("sourceColorPicker") && this._analyzeTextColors)

[lushnikov, 2016/07/19 21:12:16]

nit: I'd check for the flag first

[flandy, 2016/07/19 22:58:15]

Done.

             this._updateColorSwatches();
     },
 
@@ -187,13 +244,24 @@ WebInspector.CSSSourceFrame.prototype = {
     onTextChanged: function(oldRange, newRange)
     {
         WebInspector.UISourceCodeFrame.prototype.onTextChanged.call(this, oldRange, newRange);
-        if (Runtime.experiments.isEnabled("sourceColorPicker")) {
+        if (Runtime.experiments.isEnabled("sourceColorPicker") && this._analyzeTextColors) {
             if (this._updateTimeout)
                 clearTimeout(this._updateTimeout);
             this._updateTimeout = setTimeout(this._updateColorSwatches.bind(this), WebInspector.CSSSourceFrame.UpdateTimeout);
         }
     },
 
+    /**
+     * @override
+     * @param {number} lineNumber
+     */
+    scrollChanged: function(lineNumber)
+    {
+        WebInspector.UISourceCodeFrame.prototype.scrollChanged.call(this, lineNumber);
+        if (this._swatchPopoverHelper.isShowing())
+            this._swatchPopoverHelper.hide();
+    },
+
     __proto__: WebInspector.UISourceCodeFrame.prototype
 }
 
@@ -212,6 +280,67 @@ WebInspector.CSSSourceFrame.ColorPosition = function(color, lineNumber, startCol
 
 /**
  * @constructor
+ * @param {!WebInspector.CSSSourceFrame} cssSourceFrame
+ * @param {!WebInspector.CSSSourceFrame.ColorPosition} colorPosition
+ */
+WebInspector.CSSSourceFrame.ColorSwatch = function(cssSourceFrame, colorPosition)
+{
+    this._cssSourceFrame = cssSourceFrame;
+    this._colorPosition = colorPosition;
+
+    this._swatch = WebInspector.ColorSwatch.create();
+    this._swatch.setColorText(colorPosition.color.asString(WebInspector.Color.Format.Original) || "white");
+    var shiftClickMessage = WebInspector.UIString("Shift + Click to change color format.");
+    this._swatch.iconElement().title = WebInspector.UIString("Open color picker. %s", shiftClickMessage);
+    this._swatch.setClickHandler(this._handleClick.bind(this));
+    this._swatch.hideText(true);
+}
+
+WebInspector.CSSSourceFrame.ColorSwatch.prototype = {
+    /**
+     * @param {!Event} event
+     */
+    _handleClick: function(event)
+    {
+        if (event.shiftKey) {

[lushnikov, 2016/07/19 21:12:16]

I'm not convinced we need the shift-click in the source frame swatch - mb remove
this functionality for now?

[flandy, 2016/07/19 22:58:15]

I think it would be nice to have, in order to mirror the functionality in the
styles sidebar pane. Removed for now.

+            this._swatch.toggleNextFormat();
+            this._cssSourceFrame._analyzeTextColors = false;
+            this._cssSourceFrame._replaceColorText(this._colorPosition, this._swatch.color().asString(this._swatch.format()) || "white");

[lushnikov, 2016/07/19 21:12:16]

where does "white" come from?

[flandy, 2016/07/19 22:58:15]

color.asString() may return null|string according to closure compiler.
cssSourceFrame._replaceColorText() expects a string, so this is to make the
compiler happy. 

I can change the annotation for _replaceColorText() to use {?string} and
introduce a check on that string

Or I think it may make more sense to add the get colorText() method on
WI.ColorSwatch and just call that.

+            this._cssSourceFrame._analyzeTextColors = true;
+        } else {
+            this._cssSourceFrame._currentSwatch = this;

[lushnikov, 2016/07/19 21:12:16]

let's not reach to internal fields of cssSourceFrame from here - you already
call it's delegate
method _showSpectrum, it should do everything

[flandy, 2016/07/19 22:58:15]

Done.

+            this._cssSourceFrame._showSpectrum(this._swatch);

[lushnikov, 2016/07/19 21:12:16]

I think it would be easier if you just pass the WI.CSSSourceFrame.ColorSwatch
instance
to the CSSSourceFrame:

this._cssSourceFrame._showSpectrum(this);

[flandy, 2016/07/19 22:58:15]

Done.

+        }
+        event.consume(true);
+    },
+
+    /**
+     * @return {!WebInspector.ColorSwatch}
+     */
+    getElement: function()

[lushnikov, 2016/07/19 21:12:16]

element: ...

(we use nouns for getters and verbs for setters)

[flandy, 2016/07/19 22:58:15]

Done.

+    {
+        return this._swatch;
+    },
+
+    /**
+     * @return {!WebInspector.CSSSourceFrame.ColorPosition}
+     */
+    getColorPosition: function()

[lushnikov, 2016/07/19 21:12:16]

colorPosition: ..

[flandy, 2016/07/19 22:58:15]

Done.

+    {
+        return this._colorPosition;
+    },
+
+    /**
+     * @param {string} colorString
+     */
+    setColorText: function(colorString)
+    {
+        this._swatch.setColorText(colorString);
+    }
+}
+
+/**
+ * @constructor
  * @implements {WebInspector.TextEditorAutocompleteDelegate}
  */
 WebInspector.CSSSourceFrame.AutocompleteDelegate = function()