Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 71 additions & 5 deletions src/language/CSSUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
*/


/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50, regexp: true */
/*global define, $, CodeMirror, _parseRuleList: true */

// JSLint Note: _parseRuleList() is cyclical dependency, not a global function.
Expand All @@ -48,6 +48,11 @@ define(function (require, exports, module) {
PROP_VALUE = "prop.value",
IMPORT_URL = "import.url";

var RESERVED_FLOW_NAMES = ["content", "element"],
INVALID_FLOW_NAMES = ["none", "inherit", "default", "auto", "initial"],
IGNORED_FLOW_NAMES = RESERVED_FLOW_NAMES.concat(INVALID_FLOW_NAMES);


/**
* @private
* Checks if the current cursor position is inside the property name context
Expand Down Expand Up @@ -501,7 +506,7 @@ define(function (require, exports, module) {
declListStartChar: column in line where the declaration list for the rule starts
declListEndLine: line where the declaration list for the rule ends
declListEndChar: column in the line where the declaration list for the rule ends
* @param text {!String} CSS text to extract from
* @param text {!string} CSS text to extract from
* @return {Array.<Object>} Array with objects specifying selectors.
*/
function extractAllSelectors(text) {
Expand Down Expand Up @@ -851,8 +856,8 @@ define(function (require, exports, module) {
* jquery and ask what matches. If the node that the user's cursor is in comes back from jquery, then
* we know the selector applies.
*
* @param text {!String} CSS text to search
* @param selector {!String} selector to search for
* @param text {!string} CSS text to search
* @param selector {!string} selector to search for
* @return {Array.<{selectorGroupStartLine:number, declListEndLine:number, selector:string}>}
* Array of objects containing the start and end line numbers (0-based, inclusive range) for each
* matched selector.
Expand Down Expand Up @@ -984,7 +989,7 @@ define(function (require, exports, module) {
* div .foo .bar {}
* .foo.bar {}
*
* @param {!String} selector The selector to match. This can be a tag selector, class selector or id selector
* @param {!string} selector The selector to match. This can be a tag selector, class selector or id selector
* @param {?Document} htmlDocument An HTML file for context (so we can search <style> blocks)
* @return {$.Promise} that will be resolved with an Array of objects containing the
* source document, start line, and end line (0-based, inclusive range) for each matching declaration list.
Expand Down Expand Up @@ -1115,10 +1120,71 @@ define(function (require, exports, module) {
return _stripAtRules(selector);
}

/**
* removes CSS comments from the content
* @param {!string} content to reduce
* @return {string} reduced content
*/
function _removeComments(content) {
return content.replace(/\/\*(?:(?!\*\/)[\s\S])*\*\//g, '');
}

/**
* removes strings from the content
* @param {!string} content to reduce
* @return {string} reduced content
*/
function _removeStrings(content) {
return content.replace(/[^\\]\"(.*)[^\\]\"|[^\\]\'(.*)[^\\]\'+/g, '');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We use double-quotes for strings. So the second argument should be "". Same for the one on line 1124.

}

/**
* Reduces the style sheet by removing comments and strings
* so that the content can be parsed using a regular expression
* @param {!string} content to reduce

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to add ! for primitive types, since those are not null by default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear about the use of ! in JSDoc. JSDoc documents ! is a Non-nullable type:

{!number} Indicates that the value is of the specified type, but cannot be null.

This is confusing because undefined and null are considered types in Javascript so how could anything be null or undefined if a type is specified?

var s = null; // would indicate that the variable is of type null 

There are valid times when undefined and null could mean different things so doesn't it also mean that type type cannot be null or undefined?

JSDoc has ? to indicate an optional type to account for the specified type | undefined but you could still pass undefined explicitly to the function which makes sense if there is another parameter that follows that you don't want to omit or isn't optional. Passing undefined isn't technically an omitted parameter:

removeStrings();  // omitted parameter
removeStrings(undefined);  // has same behavior as if the parameter was omitted 

It seems that a null parameter isn't one that was omitted and shouldn't be ignored so is isn't {!string} telling JSDoc that the argument cannot be null or undefined and must be String?

Anyway, this comment block was copied from another place in this same file so I assumed it was correct. There are many others like it throughout the codebase (in many, many files) It very well may not be the correct usage but at least it's consistent with the rest of the code so I think leave it for now.

I will bring it up at the next architecture review, however, and see if anyone feels strongly about changing it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, the problem seems that http://usejsdoc.org/tags-type.html is a bit different than https://developers.google.com/closure/compiler/docs/js-for-compiler#types.

According to the Google's JSDocs, functions, booleans, numbers, and strings are non-nullable by default. So if you say {number} you can't pass undefined or null, since is the same as {!number}. If you want it to to be able to be undefined or null, you need to use {?number}.

But it seems that usejsdoc.org, does not use this convention, so {!number} is always a number and {?number} is number or null. So then what does {number} means? The first or the second option? Or should all the types have a ! or ? or =?

* @return {string} reduced content
*/
function reduceStyleSheetForRegExParsing(content) {
return _removeStrings(_removeComments(content));
}

/**
* Extracts all named flow instances
* @param {!string} text to extract from
* @return {Array.<string>} array of unique flow names found in the content (empty if none)
*/
function extractAllNamedFlows(text) {
var namedFlowRegEx = /(?:flow\-(into|from)\:\s*)([\w\-_]+)(?:\s*;)/gi,
result = [],
names = {},
thisMatch;

// Reduce the content so that matches
// inside strings and comments are ignored
text = reduceStyleSheetForRegExParsing(text);

// Find the first match
thisMatch = namedFlowRegEx.exec(text);

// Iterate over the matches and add them to result
while (thisMatch) {
var thisName = thisMatch[2];

if (IGNORED_FLOW_NAMES.indexOf(thisName) === -1 && !names.hasOwnProperty(thisName)) {
names[thisName] = result.push(thisName);
}
thisMatch = namedFlowRegEx.exec(text);
}

return result;
}

exports._findAllMatchingSelectorsInText = _findAllMatchingSelectorsInText; // For testing only
exports.findMatchingRules = findMatchingRules;
exports.extractAllSelectors = extractAllSelectors;
exports.extractAllNamedFlows = extractAllNamedFlows;
exports.findSelectorAtDocumentPos = findSelectorAtDocumentPos;
exports.reduceStyleSheetForRegExParsing = reduceStyleSheetForRegExParsing;

exports.SELECTOR = SELECTOR;
exports.PROP_NAME = PROP_NAME;
Expand Down
93 changes: 93 additions & 0 deletions test/spec/CSSUtils-test-files/regions.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/* basic tests */
article.content {
flow-into: main;
}

section.layout > div {
flow-from: main;
}


#jeff.content {
flow-into: jeff;
}

#jeff.layout > div {
flow-from: jeff;
}

/* exclude matches inside comments tests */

/*
p.content {
flow-into: carter;
}

p.layout > div {
flow-from: carter;
}
*/

/* exclude matches inside strings tests */

div {
content: "/* p.content { flow-into: carter; } p.layout > div { flow-from: carter; } */";
}


div {
content: "html.content { flow-into: dexter; } html.layout > div { flow-from: dexter; }";
}


/*
div.content {
content: "flow-into: martin;";
}

div.layout > div {
content: "flow-from: martin;";
}
*/

/* multi-line property tests */

#randy.content {
flow-into:
randy
;
}

#randy.layout > div {
flow-from: randy;
}

/* test to exclude duplicates */
#yin.content {
flow-into: jeff;
}

#yin.layout > div {
flow-from: jeff;
}

/* flow-from only tests */
#raymond.layout > div {
flow-from: lim;
}

/* underscores and dashes */
#ingo.content {
flow-from: edge-code_now_shipping;
}

/* invalid / ignored flows */
#test364.content {
flow-from: default;
flow-from: none;
flow-from: inherit;
flow-from: auto;
flow-from: initial;
flow-from: content;
flow-from: element;
}
20 changes: 19 additions & 1 deletion test/spec/CSSUtils-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ define(function (require, exports, module) {
offsetsCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/offsets.css"),
bootstrapCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/bootstrap.css"),
escapesCssFileEntry = new NativeFileSystem.FileEntry(testPath + "/escaped-identifiers.css"),
embeddedHtmlFileEntry = new NativeFileSystem.FileEntry(testPath + "/embedded.html");
embeddedHtmlFileEntry = new NativeFileSystem.FileEntry(testPath + "/embedded.html"),
cssRegionsFileEntry = new NativeFileSystem.FileEntry(testPath + "/regions.css");

var contextTestCss = require("text!spec/CSSUtils-test-files/contexts.css"),
selectorPositionsTestCss = require("text!spec/CSSUtils-test-files/selector-positions.css");
Expand Down Expand Up @@ -1922,4 +1923,21 @@ define(function (require, exports, module) {
});
});
});

describe("CSS Regions", function () {
beforeEach(function () {
init(this, cssRegionsFileEntry);
});

it("should find named flows", function () {
var namedFlows = CSSUtils.extractAllNamedFlows(this.fileContent);
expect(namedFlows.length).toBe(5);
expect(namedFlows[0]).toBe("main");
expect(namedFlows[1]).toBe("jeff");
expect(namedFlows[2]).toBe("randy");
expect(namedFlows[3]).toBe("lim");
expect(namedFlows[4]).toBe("edge-code_now_shipping");
});

});
});