CSS AtRules, Pseudo elements and Pseudo selector code hints #13295

swmitra · 2017-04-10T08:40:58Z

This PR adds CSS restricted block and pseudo selector/element code hints. In the implementation, priority of the hint providers have been kept at 0 while the priority of the default CSS code hint provider has been increased to 1. This has been done to ensure no additional overhead in case of CSS property:value code hints. Attached some screen shots of the corresponding code hints for illustration.

@rules
Pseudo selectors
Pseudo elements

Copying @ingorichter @petetnt @ficristo @zaggino @peterflynn for code review.

Note: Additional description have been captured in the json def files to enable description tooltip at a later point of time.

petetnt

Great job, some initial comments! The feature itself seems to work just fine!

petetnt · 2017-04-10T15:36:07Z

src/extensions/default/CSSCodeHints/AtRuleCodeHints.js

+        if (this.filter) {
+
+            // Filter the property list based on the token string
+            var result = $.map(Object.keys(AtRules), function (key, value) {


Would prefer Array.prototype.map here, maybe with something like:

Object.keys(AtRules).map(function (key) { if (key.indexOf(token.string) === 0) { return key } }).filter(function (key) { return key; }).sort();

petetnt · 2017-04-10T15:40:23Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+        this.editor = editor;
+
+        // Check if we are at ':' pseudo rule or in 'variable-3' 'def' context
+        if (token.state.state === "pseudo" || token.type === "variable-3") {


return token.state.state === "pseudo" || token.type === "variable-3"

petetnt · 2017-04-10T15:42:18Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+
+        if (this.context !== -1) {
+            // Filter the property list based on the token string
+            var result = $.map(this.context === TOKEN_TYPE_PSEUDO_SELECTOR ? Object.keys(PseudoRules.selectors) : Object.keys(PseudoRules.elements), function (value, key) {


Prefer Array.prototype.map, maybe do the context check first.

var keys = this.context === TOKEN_TYPE_PSEUDO_SELECTOR ? Object.keys(PseudoRules.selectors) : Object.keys(PseudoRules.elements); var result = keys.map(...)

petetnt · 2017-04-10T15:43:07Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+                handleWideResults: false
+            };
+        } else {
+            return false;


The other similar function returns null but I guess it doesn't matter

petetnt · 2017-04-10T15:44:00Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+            this.editor.setCursorPos({line: cursor.line, ch: cursor.ch - 1});
+        }
+
+        return false;


Does this need to return anything?

petetnt · 2017-04-10T15:44:11Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+        return false;
+    };
+
+    function _init() {


Empty method

AFAICT unused, remove.

petetnt · 2017-04-10T15:45:02Z

src/extensions/default/CSSCodeHints/PseudoSelectors.json

+{
+    "selectors" :
+    {
+        "active" : { "desc": "Selects the active link"},


Normalize whitespace in this file between the keys and values

petetnt · 2017-04-10T15:45:51Z

src/extensions/default/CSSCodeHints/main.js

@@ -468,7 +472,7 @@ define(function (require, exports, module) {

    AppInit.appReady(function () {
        var cssPropHints = new CssPropHints();
-        CodeHintManager.registerHintProvider(cssPropHints, ["css", "scss", "less"], 0);
+        CodeHintManager.registerHintProvider(cssPropHints, ["css", "scss", "less"], 1);


Q: What's the second parameter?

And why we need this change?

I have mentioned it in the opening note of the PR 😄 .

In the implementation, priority of the hint providers have been kept at 0 while the priority of the default CSS code hint provider has been increased to 1. This has been done to ensure no additional overhead in case of CSS property:value code hints.

To explain, the second parameter is for priority. CodeHintManager keeps the providers for a particular mode in order of priority, If there are priority collisions, then the lists are sorted based on loading order(this is not in our control). CodeHintManager first checks the current doc mode and then iterates over this sorted providers by calling 'hasHints', once it gets affirmative answer from a particular provider, it breaks there and selects that provider as the current session hint provider. By increasing the priority of the existing provider I am trying to ensure that, it gets the opportunity first to provide hints ( as hasHints() of this provider will be called first). This will ensure that, there is zero overhead on the current property:value hint provider.

@swmitra Sorry I already forgot about the opening note of PR 😸 thanks for the detailed explanation!

Same here...

ficristo · 2017-04-10T17:58:03Z

This fixes #12486

ficristo · 2017-04-10T17:59:10Z

src/extensions/default/CSSCodeHints/AtRuleCodeHints.js

+
+
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, brackets, $ */


Remove both lines. Nowadays they are applyed globally.
And ideally there should be a jsdoc with a brief description of this file.

ficristo · 2017-04-10T18:00:37Z

src/extensions/default/CSSCodeHints/AtRuleCodeHints.js

+        }
+
+        // Check if we are at '@' rule 'def' context
+        if ((token.type === "def" && cmState.context.type === "at") || (token.type === "variable-2" && (cmState.context.type === "top" || cmState.context.type === "block"))) {


Split in two lines

ficristo · 2017-04-10T18:05:08Z

src/extensions/default/CSSCodeHints/AtRuleCodeHints.js

+        this.filter = token.string;
+        this.token = token;
+
+        if (this.filter) {


I prefer an early return here:

if (!this.filter) { return null; } var result...

ficristo · 2017-04-10T18:06:45Z

src/extensions/default/CSSCodeHints/AtRuleCodeHints.js

+     *
+     * @return {boolean}
+     * Indicates whether the manager should follow hint insertion with an
+     * additional explicit hint request.


We should use an uniform style to doc comments: the description should be always inline or always at new line.
I think the current style is on the same line.

ficristo · 2017-04-10T18:06:53Z

src/extensions/default/CSSCodeHints/AtRuleCodeHints.js

+    /**
+     * Inserts a given @<rule> hint into the current editor context.
+     *
+     * @param {string} hint


Wrong param name

ficristo · 2017-04-10T18:08:48Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+
+
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, brackets, $ */


As before, remove.

ficristo · 2017-04-10T18:09:35Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+    }
+
+    // As we are only going to provide :<pseudo> name hints
+    // we should claim that we don't have hints for anything else


Use JSDoc and add the params

ficristo · 2017-04-10T18:10:23Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+        this.token = token;
+
+
+        if (this.context !== -1) {


Early return.

ficristo · 2017-04-10T18:10:49Z

src/extensions/default/CSSCodeHints/PseudoSelectorHints.js

+        return false;
+    };
+
+    function _init() {


AFAICT unused, remove.

ficristo · 2017-04-10T18:11:45Z

src/extensions/default/CSSCodeHints/main.js

@@ -468,7 +472,7 @@ define(function (require, exports, module) {

    AppInit.appReady(function () {
        var cssPropHints = new CssPropHints();
-        CodeHintManager.registerHintProvider(cssPropHints, ["css", "scss", "less"], 0);
+        CodeHintManager.registerHintProvider(cssPropHints, ["css", "scss", "less"], 1);


And why we need this change?

ficristo · 2017-04-10T18:30:43Z

I briefly tested, but I cannot use the pseudo selectors in a SCSS file, but I can in a CSS one.

Do you have thought of using multiple extensions like suggested by @redmunds in #12486 (comment)?

I don't like much the requires with side effects in main.js, so if we go on this route,
do you think is possible to have AtRuleCodeHints, PseudoSelectorHints and CssPropHints files which exports their class and a main.js file with a single AppInit.appReady function that create an instance for every of the three classes?

swmitra · 2017-04-11T09:11:06Z

@Petent @ficristo I have addressed all the review comments in my last commit. Also extracted these providers as separate default extensions. Need your review once more 😄

ficristo · 2017-04-11T17:35:54Z

In general LGTM, but I still cannot see pseudo selector hints for SCSS code.
Also add some tests.

I've use this snippet to test:

@charset "utf8";
/*comment*/

.test1: {
}

.test2:: {
}

swmitra · 2017-04-12T08:50:45Z

@ficristo You probably have 'brackets-saas' extension installed. That extension registers 'SaasHints' provider with high priority and claims to provide hints ( 'hasHints' call returns true) even when it can't.
Please disable that extension and try. It should work.

ficristo · 2017-04-12T11:21:02Z

I disabled all my extensions and I can still reproduce.
If you write .test: in a SCSS file the hints work.
If you write .test:: {}, move the cursor after the last : and use Ctrl-space to ask hints, nothing happen. But the same works in a CSS file.

swmitra · 2017-04-13T06:28:42Z

@ficristo WIth the SCSS tokenizer in CM2, the token identifier is not able to detect the pseudo context in the second scenario you have mentioned. I have filed an issue with CM codemirror/codemirror5#4688 just now. Lets wait for a response from @marijnh.

swmitra · 2017-04-13T11:09:44Z

@ficristo Please refer to the CM issue and the discussion at codemirror/codemirror5#4688, we might have to live with this issue for sometime 😞 . But fortunately the moment you put the first char after :, you will get hints if the typed characters matches any of them. I tried to get some workaround with regex, but it's not going to work anyway as CM matches /:\s*\{/ and combines them to a single token. So distinguishing between /:\s*\{/ and /::\s*\{/ is virtually impossible for me (unless someone can figure out a hack not dependent on the CM token).

marijnh · 2017-04-13T11:12:39Z

Just figuring out what the SCSS mode is doing and fixing it is going to be a lot easier than trying to work around this from the outside.

swmitra · 2017-04-13T11:16:41Z

Thanks @marijnh for joining in 👍
I was just going through some of the syntax's in SCSS and most likely need to spend some more time to abstractly make sure that I don't break anything while fixing the CM CSS mode parser 😄

ficristo · 2017-04-13T12:18:57Z

According to http://sass-lang.com/documentation/file.SASS_REFERENCE.html#nested_properties you can "split" properties like this:

.test {
  font: {
    family: serif;
  }
}

instead of writing

.test {
  font-family: serif;
}

I don't know what we can do about it for our case.

@swmitra is the "problem" also present with :: ?

swmitra · 2017-04-13T12:34:09Z

yes @ficristo. The regex used in the mode parser matches both the cases. This is what it does for punctuation : handling in mode/css/css.js @line 752 -

":": function(stream) {
        if (stream.match(/\s*\{/))
          return [null, "{"];
        return false;
      }

Now if I remove this or try to provide a punctuation handler without wrapping the /\s{/ , our property:value handling will go for a toss 😞 in the following SCSS construct

.test {
  font: {
    family: serif;
  }
}

I can think of one particular way though where we look backward for CSS property keywords and decide whether to identify this token as 'pseudo' or 'variable-3'. If the backward lookup tells us that the preceding token is actually a CSS property keyword then we don't handle the punctuation to identify as the start of 'pseudo'.

@marijnh Badly need your input here...

swmitra · 2017-04-13T12:48:01Z

@marijnh I see that the following codemirror/codemirror5@2c46d1b fixes the issue 👍
[Edit]
I was excited to see the commit and the issue getting closed. It's not exactly fixed the problem here. The token string is : though, but state has not changed.
[Edit]
Thanks a lot @marijnh . it works now 👍 with some modification in the hint provider.

swmitra · 2017-04-13T13:43:25Z

@ficristo The issue in SCSS mode is fixed now using CM codemirror/codemirror5@2c46d1b and my last commit. Can you please check it once?

ficristo · 2017-04-16T17:48:40Z

To see the last changes in action we need to wait for a CodeMirror upgrade, is it correct?
Also this PR needs some tests.

swmitra · 2017-04-17T04:30:41Z

Yes @ficristo, I will upgrade CM once a release is available with the mentioned commit. I am writing tests for both the extensions...

ingorichter

A couple of nits. Otherwise LGTM

ingorichter · 2017-04-18T00:29:00Z

src/extensions/default/CSSAtRuleCodeHints/main.js

+        }
+
+        // Filter the property list based on the token string
+        var result = Object.keys(AtRules).map(function (key) {


I might be wrong, but I think filter would be more appropriate than map for the first loop. This would also make the following filter loop unnecessary.

ingorichter · 2017-04-18T00:30:04Z

src/extensions/default/CSSPseudoSelectorHints/PseudoSelectors.json

+        "active":              {"desc": "Selects the active link"},
+        "checked":             {"desc": "Selects every checked <input> element"},
+        "disabled":            {"desc": "Selects every disabled <input> element"},
+        "empty"	:              {"desc": "Selects every element that has no children/text (including text nodes)"},


nit: formatting is a bit off

ingorichter · 2017-04-18T00:30:50Z

src/extensions/default/CSSPseudoSelectorHints/PseudoSelectors.json

+    },
+    "elements" :
+    {
+        "after":        {"desc": "Insert something after the content of each element identified by this selctor"},


nit: selector

ingorichter · 2017-04-18T00:31:16Z

src/extensions/default/CSSPseudoSelectorHints/PseudoSelectors.json

+    "elements" :
+    {
+        "after":        {"desc": "Insert something after the content of each element identified by this selctor"},
+        "before":       {"desc": "Insert something before the content of each element identified by this selctor"},


nit: same selector spelling

basically all instances of selctor in this object ;-)

ingorichter · 2017-04-18T00:34:25Z

src/extensions/default/CSSPseudoSelectorHints/main.js

+                return key;
+            }
+        }).filter(function (key) {
+            return key;


replacing the previous map call with filter should make this obsolete

ingorichter · 2017-04-18T00:35:52Z

src/extensions/default/CSSPseudoSelectorHints/main.js

+        this.editor = editor;
+
+        // Check if we are at ':' pseudo rule or in 'variable-3' 'def' context
+        return token.state.state === "pseudo" || token.type === "variable-3" || token.string === PUNCTUATION_CHAR;


nit: this could be a function determining the state. The same statement (negated) is used in getHints and could be reused there. Plus, it makes testing this function in isolation easier.

…ra/EnhancedCSSCodeHints

swmitra · 2017-04-26T08:41:30Z

@petetnt @ingorichter @ficristo Can you guys please have a look at this PR now?
CM dependency has been resolved with #13331 and also I have addressed all review comments and added unit tests for all style modes (css, less, scss).

ficristo · 2017-04-26T18:43:59Z

There is still the nit of @ingorichter to fix. No time to look again.

AtRules, Pseudo elements and Pseudo selector code hints

e690a96

swmitra requested review from ingorichter, zaggino, petetnt, ficristo and peterflynn April 10, 2017 08:40

petetnt suggested changes Apr 10, 2017

View reviewed changes

ficristo suggested changes Apr 10, 2017

View reviewed changes

Adress review comments and restructure using different extensions

79afc18

Handle SCSS mode seperately

4df0985

ingorichter reviewed Apr 18, 2017

View reviewed changes

swmitra mentioned this pull request Apr 24, 2017

Update Codemirror to version 5.25.2 #13331

Merged

swmitra added 2 commits April 26, 2017 08:38

Merge branch 'master' of https://github.com/adobe/brackets into swmit…

30ebdde

…ra/EnhancedCSSCodeHints

Adding unit tests for the newly added extensions

60d97a9

Fix for eslint errors

7b4bdf4

ficristo approved these changes Apr 26, 2017

View reviewed changes

Extract pseudo context validation as a function

03d1789

petetnt approved these changes Apr 27, 2017

View reviewed changes

zaggino approved these changes Apr 27, 2017

View reviewed changes

zaggino merged commit 515fb5c into master Apr 27, 2017

zaggino deleted the swmitra/EnhancedCSSCodeHints branch April 27, 2017 13:31

marcelgerber mentioned this pull request Jun 17, 2017

auto complete for pseudo-selectors? #12486

Closed

ficristo added this to the Release 1.10 milestone Jun 18, 2017

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] CSS AtRules, Pseudo elements and Pseudo selector code hints brackets-archive/bracketsIssues#11262

Open



		/jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 /
		/global define, brackets, $ /

CSS AtRules, Pseudo elements and Pseudo selector code hints #13295

CSS AtRules, Pseudo elements and Pseudo selector code hints #13295

Conversation

swmitra commented Apr 10, 2017 • edited Loading

petetnt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swmitra Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Apr 10, 2017 • edited Loading

swmitra commented Apr 11, 2017

ficristo commented Apr 11, 2017

swmitra commented Apr 12, 2017

ficristo commented Apr 12, 2017

swmitra commented Apr 13, 2017

swmitra commented Apr 13, 2017

marijnh commented Apr 13, 2017

swmitra commented Apr 13, 2017

ficristo commented Apr 13, 2017

swmitra commented Apr 13, 2017 • edited Loading

swmitra commented Apr 13, 2017 • edited Loading

swmitra commented Apr 13, 2017

ficristo commented Apr 16, 2017

swmitra commented Apr 17, 2017

ingorichter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swmitra commented Apr 26, 2017

ficristo commented Apr 26, 2017

swmitra commented Apr 10, 2017 •

edited

Loading

swmitra Apr 11, 2017 •

edited

Loading

ficristo commented Apr 10, 2017 •

edited

Loading

swmitra commented Apr 13, 2017 •

edited

Loading

swmitra commented Apr 13, 2017 •

edited

Loading