Merge pull request #10401 from MarcelGerber/color-editor-fixes

redmunds · redmunds · commit c321002c37ef · 2015-01-22T15:25:32.000-08:00
Multiple ColorEditor fixes
diff --git a/src/extensions/default/InlineColorEditor/ColorEditor.js b/src/extensions/default/InlineColorEditor/ColorEditor.js
@@ -391,6 +391,15 @@ define(function (require, exports, module) {
         });
     };
 
+    /**
+     * Checks whether colorVal is a valid color
+     * @param {!string} colorVal
+     * @return {boolean} Whether colorVal is valid
+     */
+    ColorEditor.prototype.isValidColor = function (colorVal) {
+        return tinycolor(colorVal).isValid();
+    };
+
     /**
      * Sets _hsv and _color based on an HSV input, and updates the UI. Attempts to preserve
      * the previous color format.
@@ -518,7 +527,7 @@ define(function (require, exports, module) {
     
     /**
      * Handles undo gestures while color picker has focus. We don't want to let CodeMirror's
-     * usual undo logic run since it will destroy our bookmarks.
+     * usual undo logic run since it will destroy our marker.
      */
     ColorEditor.prototype.undo = function () {
         if (this._originalColor.toString() !== this._color.toString()) {
diff --git a/src/extensions/default/InlineColorEditor/InlineColorEditor.js b/src/extensions/default/InlineColorEditor/InlineColorEditor.js
@@ -41,13 +41,11 @@ define(function (require, exports, module) {
     /**
      * Inline widget containing a ColorEditor control
      * @param {!string} color  Initially selected color
-     * @param {!CodeMirror.Bookmark} startBookmark
-     * @param {!CodeMirror.Bookmark} endBookmark
+     * @param {!CodeMirror.TextMarker} marker
      */
-    function InlineColorEditor(color, startBookmark, endBookmark) {
+    function InlineColorEditor(color, marker) {
         this._color = color;
-        this._startBookmark = startBookmark;
-        this._endBookmark = endBookmark;
+        this._marker = marker;
         this._isOwnChange = false;
         this._isHostChange = false;
         this._origin = "+InlineColorEditor_" + (lastOriginId++);
@@ -69,17 +67,10 @@ define(function (require, exports, module) {
     InlineColorEditor.prototype._color = null;
     
     /**
-     * Start of the range of code we're attached to; _startBookmark.find() may by null if sync is lost.
-     * @type {!CodeMirror.Bookmark}
+     * Range of code we're attached to; _marker.find() may by null if sync is lost.
+     * @type {!CodeMirror.TextMarker}
      */
-    InlineColorEditor.prototype._startBookmark = null;
-    
-    /**
-     * End of the range of code we're attached to; _endBookmark.find() may by null if sync is lost or even
-     * in some cases when it's not. Call getCurrentRange() for the definitive text range we're attached to.
-     * @type {!CodeMirror.Bookmark}
-     */
-    InlineColorEditor.prototype._endBookmark = null;
+    InlineColorEditor.prototype._marker = null;
     
     /** @type {boolean} True while we're syncing a color picker change into the code editor */
     InlineColorEditor.prototype._isOwnChange = null;
@@ -97,25 +88,24 @@ define(function (require, exports, module) {
      * @return {?{start:{line:number, ch:number}, end:{line:number, ch:number}}}
      */
     InlineColorEditor.prototype.getCurrentRange = function () {
-        var start, end;
+        var pos, start, end;
         
-        start = this._startBookmark.find();
+        pos = this._marker && this._marker.find();
+
+        start = pos && pos.from;
         if (!start) {
             return null;
         }
         
-        end = this._endBookmark.find();
+        end = pos.to;
         if (!end) {
-            end = { line: start.line };
+            end = {line: start.line};
         }
         
-        // Even if we think we have a good end bookmark, we want to run the
-        // regexp match to see if there's a valid match that extends past the bookmark.
+        // Even if we think we have a good range end, we want to run the
+        // regexp match to see if there's a valid match that extends past the marker.
         // This can happen if the user deletes the end of the existing color and then
         // types some more.
-        // TODO: when we migrate to CodeMirror v3, we might be able to use markText()
-        // instead of two bookmarks to track the range. (In our current old version of
-        // CodeMirror v2, markText() isn't robust enough for this case.)
         
         var line = this.hostEditor.document.getLine(start.line),
             matches = line.substr(start.ch).match(ColorUtils.COLOR_REGEX);
@@ -124,12 +114,12 @@ define(function (require, exports, module) {
         // the matched length here.
         if (matches && (end.ch === undefined || end.ch - start.ch < matches[0].length)) {
             end.ch = start.ch + matches[0].length;
-            this._endBookmark.clear();
-            this._endBookmark = this.hostEditor._codeMirror.setBookmark(end);
+            this._marker.clear();
+            this._marker = this.hostEditor._codeMirror.markText(start, end);
         }
         
         if (end.ch === undefined) {
-            // We were unable to resync the end bookmark.
+            // We were unable to resync the marker.
             return null;
         } else {
             return {start: start, end: end};
@@ -150,14 +140,20 @@ define(function (require, exports, module) {
 
             // Don't push the change back into the host editor if it came from the host editor.
             if (!this._isHostChange) {
+                var endPos = {
+                        line: range.start.line,
+                        ch: range.start.ch + colorString.length
+                    };
                 this._isOwnChange = true;
                 this.hostEditor.document.batchOperation(function () {
                     // Replace old color in code with the picker's color, and select it
+                    self.hostEditor.setSelection(range.start, range.end); // workaround for #2805
                     self.hostEditor.document.replaceRange(colorString, range.start, range.end, self._origin);
-                    self.hostEditor.setSelection(range.start, {
-                        line: range.start.line,
-                        ch: range.start.ch + colorString.length
-                    });
+                    self.hostEditor.setSelection(range.start, endPos);
+                    if (self._marker) {
+                        self._marker.clear();
+                        self._marker = self.hostEditor._codeMirror.markText(range.start, endPos);
+                    }
                 });
                 this._isOwnChange = false;
             }
@@ -202,11 +198,8 @@ define(function (require, exports, module) {
     InlineColorEditor.prototype.onClosed = function () {
         InlineColorEditor.prototype.parentClass.onClosed.apply(this, arguments);
 
-        if (this._startBookmark) {
-            this._startBookmark.clear();
-        }
-        if (this._endBookmark) {
-            this._endBookmark.clear();
+        if (this._marker) {
+            this._marker.clear();
         }
 
         var doc = this.hostEditor.document;
@@ -273,15 +266,17 @@ define(function (require, exports, module) {
         if (range) {
             var newColor = this.hostEditor.document.getRange(range.start, range.end);
             if (newColor !== this._color) {
-                this._isHostChange = true;
-                this.colorEditor.setColorFromString(newColor);
-                this._isHostChange = false;
+                if (this.colorEditor.isValidColor(newColor)) { // only update the editor if the color string is valid
+                    this._isHostChange = true;
+                    this.colorEditor.setColorFromString(newColor);
+                    this._isHostChange = false;
+                }
             }
         } else {
             // The edit caused our range to become invalid. Close the editor.
             this.close();
         }
     };
 
-    module.exports.InlineColorEditor = InlineColorEditor;
+    exports.InlineColorEditor = InlineColorEditor;
 });
diff --git a/src/extensions/default/InlineColorEditor/main.js b/src/extensions/default/InlineColorEditor/main.js
@@ -39,10 +39,10 @@ define(function (require, exports, module) {
      *
      * @param {Editor} hostEditor
      * @param {{line:Number, ch:Number}} pos
-     * @return {?{color:String, start:TextMarker, end:TextMarker}}
+     * @return {?{color:String, marker:TextMarker}}
      */
     function prepareEditorForProvider(hostEditor, pos) {
-        var colorRegEx, cursorLine, match, sel, start, end, startBookmark, endBookmark;
+        var colorRegEx, cursorLine, match, sel, start, end, endPos, marker;
         
         sel = hostEditor.getSelection();
         if (sel.start.line !== sel.end.line) {
@@ -68,16 +68,14 @@ define(function (require, exports, module) {
         // Adjust pos to the beginning of the match so that the inline editor won't get 
         // dismissed while we're updating the color with the new values from user's inline editing.
         pos.ch = start;
+        endPos = {line: pos.line, ch: end};
         
-        startBookmark = hostEditor._codeMirror.setBookmark(pos);
-        endBookmark = hostEditor._codeMirror.setBookmark({ line: pos.line, ch: end });
-        
-        hostEditor.setSelection(pos, { line: pos.line, ch: end });
+        marker = hostEditor._codeMirror.markText(pos, endPos);
+        hostEditor.setSelection(pos, endPos);
         
         return {
             color: match[0],
-            start: startBookmark,
-            end: endBookmark
+            marker: marker
         };
     }
     
@@ -98,7 +96,7 @@ define(function (require, exports, module) {
         if (!context) {
             return null;
         } else {
-            inlineColorEditor = new InlineColorEditor(context.color, context.start, context.end);
+            inlineColorEditor = new InlineColorEditor(context.color, context.marker);
             inlineColorEditor.load(hostEditor);
     
             result = new $.Deferred();
diff --git a/src/extensions/default/InlineColorEditor/unittests.js b/src/extensions/default/InlineColorEditor/unittests.js
@@ -213,6 +213,16 @@ define(function (require, exports, module) {
                         expect(inline.getCurrentRange()).toBeTruthy();
                     });
                 });
+
+                it("should update correct range of host document when the in-editor color string is invalid", function () {
+                    makeColorEditor({line: 1, ch: 18});
+                    runs(function () {
+                        testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 24});
+                        inline.colorEditor.setColorFromString("#c0c0c0");
+                        expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 23}});
+                        expect(testDocument.getRange({line: 1, ch: 16}, {line: 1, ch: 23})).toBe("#c0c0c0");
+                    });
+                });
     
             });
             
@@ -232,12 +242,12 @@ define(function (require, exports, module) {
                     });
                 });
                 
-                it("should close itself if edit is made that destroys end bookmark and leaves color invalid", function () {
+                it("should close itself if edit is made that destroys end textmark and leaves color invalid", function () {
                     makeColorEditor({line: 1, ch: 18});
                     runs(function () {
                         spyOn(inline, "close");
                         
-                        // Replace everything including the semicolon, so it crosses the bookmark boundary.
+                        // Replace everything including the semicolon, so it crosses the textmark boundary.
                         testDocument.replaceRange("rgb(255, 25", {line: 1, ch: 16}, {line: 1, ch: 24});
                         expect(inline.close).toHaveBeenCalled();
                     });
@@ -256,25 +266,23 @@ define(function (require, exports, module) {
                     });
                 });
                 
-                it("should not update the end bookmark to a shorter valid match if the bookmark still exists and the color becomes invalid", function () {
+                it("should not update the end textmark and the color shown to a shorter valid match if the marker still exists and the color becomes invalid", function () {
                     makeColorEditor({line: 1, ch: 18});
                     runs(function () {
                         testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 23});
-                        expect(inline._color).toBe("#abcde");
+                        expect(inline._color).toBe("#abcdef");
                         expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 22}});
                     });
                 });
                 
-                // TODO: (issue #2166) The following test fails because if the end bookmark is deleted, we match the shorter
-                // #xxx string at the beginning of the color and assume that's valid, and then reset the bookmark
-                // to the end of that location.
-    //            it("should not update the end bookmark to a shorter valid match if the bookmark no longer exists and the color becomes invalid", function () {
-    //                makeColorEditor({line: 1, ch: 18}).done(function (inline) {
-    //                    testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 24});
-    //                    expect(inline._color).toBe("#abcde");
-    //                    expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 22}});
-    //                });
-    //            });
+                it("should not update the end textmark and the color shown to a shorter valid match if the marker no longer exists and the color becomes invalid", function () {
+                    makeColorEditor({line: 1, ch: 18});
+                    runs(function () {
+                        testDocument.replaceRange("", {line: 1, ch: 22}, {line: 1, ch: 24});
+                        expect(inline._color).toBe("#abcdef");
+                        expect(inline.getCurrentRange()).toEqual({start: {line: 1, ch: 16}, end: {line: 1, ch: 22}});
+                    });
+                });
                 
             });
             
