From f691db776fdc33bb66af77ed976f3d3a5f415568 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Wed, 29 Feb 2012 18:39:00 -0800 Subject: [PATCH 1/3] Cleanups related to inline editing: - Document EditorManager.createInlineEditorFromText() better; in particular, the precise meaning of 'range' - Add "known issue" unit tests for #338 and #343 (plus another case of #332) --- src/EditorManager.js | 10 +++--- test/spec/CSSManager-test.js | 65 ++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/src/EditorManager.js b/src/EditorManager.js index 1521e41121b..45af1fe6e65 100644 --- a/src/EditorManager.js +++ b/src/EditorManager.js @@ -319,10 +319,12 @@ define(function (require, exports, module) { * Creates a new inline CodeMirror editor instance containing the given text. The editor's mode * is set based on the given filename's extension (the actual file on disk is never examined). * The editor is not yet visible. - * @param {CodeMirror} hostEditor - * @param {string} text - * @param {?{startLine:Number, endLine:Number}} range - * @param {string} fileNameToSelectMode + * @param {CodeMirror} hostEditor Outer CodeMirror instance that inline editor will sit within. + * @param {string} text The text content of the editor. + * @param {?{startLine:Number, endLine:Number}} range If specified, all lines outside the given + * range are hidden from the editor. Range is inclusive. Line numbers start at 0. + * @param {string} fileNameToSelectMode A filename (optionally including path) from which to + * infer the editor's mode. */ function createInlineEditorFromText(hostEditor, text, range, fileNameToSelectMode) { // Container to hold editor & render its stylized frame diff --git a/test/spec/CSSManager-test.js b/test/spec/CSSManager-test.js index 417ea9bc483..18f8269a99f 100644 --- a/test/spec/CSSManager-test.js +++ b/test/spec/CSSManager-test.js @@ -352,7 +352,8 @@ define(function (require, exports, module) { describe("CSS Parsing: ", function () { var manager, - match; + match, + expectParseError; function findMatchingRules(tagInfo) { if (tagInfo) { @@ -393,10 +394,32 @@ define(function (require, exports, module) { return findMatchingRules(tagInfo); } + + /** + * Test helper function: expects CSS parsing to fail on the given 1-based line number with + * the given message. + */ + function _expectParseError(cssCode, lineNumber, errorString) { + try { + manager = new CSSManager._CSSManager(); + manager._loadString(cssCode); + + // shouldn't get here since _loadString() is expected to throw + this.fail("Expected parse error: "+cssCode); + + } catch (error) { + expect(error.index).toBe(lineNumber); + expect(error.message).toBe(errorString); + } + } + + /** To call fail(), these helpers need access to the value of 'this' inside each it() */ beforeEach(function () { match = _match.bind(this); + expectParseError = _expectParseError.bind(this); }); + describe("Simple selectors: ", function () { it("should match a lone type selector given a type", function () { @@ -950,19 +973,41 @@ define(function (require, exports, module) { // The following tests expect "failures" in order to pass. They // will be updated once the associated issues are fixed. describe("Known Issues", function () { + // TODO (issue #332): ParseError for double semi-colon it("should handle an empty declaration (extra semi-colon)", function () { - try { - manager = new CSSManager._CSSManager(); - manager._loadString("h4 { color:red;; }"); - } catch (error) { - expect(error.index).toBe(15); - expect(error.message).toBe("Syntax Error on line 1"); - return; - } + expectParseError("h4 { color:red;; }", 15, "Syntax Error on line 1"); + expectParseError("div{; color:red}", 4, "Syntax Error on line 1"); + }); + + // TODO (issue #338): ParseError for various IE filter syntaxes + it("should handle IE filter syntaxes", function () { + expectParseError("div{opacity:0; filter:alpha(opacity = 0)}", 15, "Syntax Error on line 1"); + expectParseError("div { filter:alpha(opacity = 0) }", 6, "Syntax Error on line 1"); + expectParseError("div { filter:progid:DXImageTransform.Microsoft.Gradient(GradientType=0,StartColorStr='#92CBE0',EndColorStr='#6B9EBC'); }", + 6, "Syntax Error on line 1"); + }); + + // TODO (issue #343): Inline editor has trouble with CSS Hacks + it("should handle unnecessary escape codes", function () { + expectParseError("div { f\\loat: left; }", 6, "Syntax Error on line 1"); + }); + + // TODO (issue #343): Inline editor has trouble with CSS Hacks + it("should handle comments within properties", function () { + expectParseError("div { display/**/: block; }", 6, "Syntax Error on line 1"); - this.fail("Known issue #332"); + // Related cases that DO work already: + match("div/**/ { display: block; }"); + match("div /**/{ display: block; }"); + match("div {/**/ display: block; }"); + match("div { /**/display: block; }"); + match("div { display:/**/ block; }"); + match("div { display: /**/block; }"); + match("div { display: block/**/; }"); + match("div { display: block /**/; }"); }); + }); // describe("Known Issues") From a1d96df6752f736f6b8b7d2357176f870f7afd43 Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Thu, 1 Mar 2012 11:41:41 -0800 Subject: [PATCH 2/3] - Fix JSLint errors - Further improve EditorManager.createInlineEditorFromText() docs --- src/EditorManager.js | 6 +++--- test/spec/CSSManager-test.js | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/EditorManager.js b/src/EditorManager.js index 45af1fe6e65..139750b9b40 100644 --- a/src/EditorManager.js +++ b/src/EditorManager.js @@ -319,11 +319,11 @@ define(function (require, exports, module) { * Creates a new inline CodeMirror editor instance containing the given text. The editor's mode * is set based on the given filename's extension (the actual file on disk is never examined). * The editor is not yet visible. - * @param {CodeMirror} hostEditor Outer CodeMirror instance that inline editor will sit within. - * @param {string} text The text content of the editor. + * @param {!CodeMirror} hostEditor Outer CodeMirror instance that inline editor will sit within. + * @param {!string} text The text content of the editor. * @param {?{startLine:Number, endLine:Number}} range If specified, all lines outside the given * range are hidden from the editor. Range is inclusive. Line numbers start at 0. - * @param {string} fileNameToSelectMode A filename (optionally including path) from which to + * @param {!string} fileNameToSelectMode A filename (optionally including path) from which to * infer the editor's mode. */ function createInlineEditorFromText(hostEditor, text, range, fileNameToSelectMode) { diff --git a/test/spec/CSSManager-test.js b/test/spec/CSSManager-test.js index 18f8269a99f..6a6a5598a38 100644 --- a/test/spec/CSSManager-test.js +++ b/test/spec/CSSManager-test.js @@ -399,19 +399,19 @@ define(function (require, exports, module) { * Test helper function: expects CSS parsing to fail on the given 1-based line number with * the given message. */ - function _expectParseError(cssCode, lineNumber, errorString) { + var _expectParseError = function (cssCode, lineNumber, errorString) { try { manager = new CSSManager._CSSManager(); manager._loadString(cssCode); // shouldn't get here since _loadString() is expected to throw - this.fail("Expected parse error: "+cssCode); + this.fail("Expected parse error: " + cssCode); } catch (error) { expect(error.index).toBe(lineNumber); expect(error.message).toBe(errorString); } - } + }; /** To call fail(), these helpers need access to the value of 'this' inside each it() */ beforeEach(function () { From 1217b32aa23ec16b2d2efbb8d6b99eb51ba7e39e Mon Sep 17 00:00:00 2001 From: Peter Flynn Date: Thu, 1 Mar 2012 12:00:31 -0800 Subject: [PATCH 3/3] - Fix incorrect docs & variable name on CSSManager-test._expectParseError() - Add more expected failures for the additional cases Randy found in issue #343 --- test/spec/CSSManager-test.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/test/spec/CSSManager-test.js b/test/spec/CSSManager-test.js index 6a6a5598a38..40096191463 100644 --- a/test/spec/CSSManager-test.js +++ b/test/spec/CSSManager-test.js @@ -396,10 +396,10 @@ define(function (require, exports, module) { /** - * Test helper function: expects CSS parsing to fail on the given 1-based line number with - * the given message. + * Test helper function: expects CSS parsing to fail at the given 0-based offset within the + * cssCode string, with the given error message. */ - var _expectParseError = function (cssCode, lineNumber, errorString) { + var _expectParseError = function (cssCode, expectedCodeOffset, expectedErrorMessage) { try { manager = new CSSManager._CSSManager(); manager._loadString(cssCode); @@ -408,8 +408,8 @@ define(function (require, exports, module) { this.fail("Expected parse error: " + cssCode); } catch (error) { - expect(error.index).toBe(lineNumber); - expect(error.message).toBe(errorString); + expect(error.index).toBe(expectedCodeOffset); + expect(error.message).toBe(expectedErrorMessage); } }; @@ -715,6 +715,15 @@ define(function (require, exports, module) { result = matchAgain({ tag: "h4" }); expect(result.length).toBe(0); + // Quotes AND braces nested inside string + // TODO (issue #343): these should get parsed correctly + expectParseError("div::after { content: \"\\\"}\\\"\"; }", 13, "Syntax Error on line 1"); + expectParseError("div::after { content: \"\\\"{\"; }", undefined, "Missing closing `}`"); + + css = "@import \"null?\\\"{\"; \n" + // this is a real-world CSS hack that is a case of the above + "div { color: red }"; + expectParseError(css, undefined, "Missing closing `}`"); + // Newline inside string (escaped) result = match("li::before { content: 'foo\\nbar'; } \n div { color:red }", { tag: "div" }); expect(result.length).toBe(1);