717369 - Autocomplete CSS properties and values in the Style Editor

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P3)

Description

[Mihai Sucan [:msucan]]

We need to implement some kind of autocomplete for CSS in the Style Editor. This needs support in the Source Editor and Orion.

The initial implementation could rely on a dynamic list of properties and their known possible values. (the dynamic list of properties can come from the Style Inspector - it is determined at runtime.)

Comment 1

[Rob Campbell [:rc] (:robcee)]

Moving to Source Editor component.

Filter on CHELICERAE.

Comment 3

[Girish Sharma [:Optimizer]]

Woo! Now we have an API to get CSS property values !

Comment 4

[Girish Sharma [:Optimizer]]

Let's use this bug as a meta to get autocompletion in Style Editor.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: WIP 1

This is the first prototype of autocompletion in style editor. Open style editor and hit the Ctrl + J (jump to line) shortcut to autocopmlete to the first possible copmletion. No popup UI yet, that is next.

This patch introduces an external (CC0 licensed) library which is the CSS tokenizer written by Tab Atkins (https://github.com/tabatkins/css-parser/blob/master/tokenizer.js). This tokenizer is a crucial part of the completion logic which is capable of completing CSS property names, values, selectors, media and keyframes tags.

I am CC'ing Gerv and also writing a post to dev-developer-tools mailing list too for the same.

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> Created attachment 820161 [details] [diff] [review]
> WIP 1
> 

Isn't this aulx [0]? Why did you not mention that, since it's also a third party library? We didn't have this discussion yet. Am I missing something, or is it something else?

[0] https://github.com/espadrine/aulx/tree/master/css

Comment 7

[Girish Sharma [:Optimizer]]

Its not Aulx as I wrote the CSS part of Aulx. It is heavily modified from what went into Aulx. There is only one function reused and I am writing the post to the list right now with further details. I don't want this bug to become a forum of discussion as some bugs have become in the past.

Comment 8

[Gervase Markham [:gerv]]

https://github.com/tabatkins/css-parser/blob/master/tokenizer.js is fine to use. Please add a comment at the top noting that the file is CC0 and giving the URL to the original source of the file.

Gerv

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: working WIP 2

Now with UI!

Tab and shift tab cycles the suggestions.

Added a comment in the tokenizer file as per the previous comment by gerv.

Also fixed the issue of "Shift Tab" not shifting the text back by one tab, but bring back the text to starting of the line.

Next up:
 - Clean up the parser.
 - Fix some known issues in the parser.
 - Speed things up a little.
 - Write tests.

Comment 10

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

- Integrated selectors suggestion into style editor.
- Everything is now a promise.
- Remoted the inspector's selector suggestions.

I am going to start asking for reviews as this patch is starting to be ready for review.

Asking Anton to review editor.js changes.
Asking Dave to review inspector.js changes.
Asking Mihai for Style Editor files related changes.

css-autocompleter.js is still not ready for review yet.

What about review of css-tokenizer.js ?

Comment 11

[Anton Kovalyov (:anton)]

Comment on attachment 8333523 [details] [diff] [review]
patch v0.1

Review of attachment 8333523 [details] [diff] [review]:
-----------------------------------------------------------------

Changes to editor.js should be an Editor addon. (See sourceeditor/debugger.js for reference.)

Comment 12

[Jason Barnabe (np)]

I would like to include this feature in my extension Stylish, which also uses Orion/CodeMirror to edit CSS. As this feature is being written, can you keep in mind reuse of the code by extensions such as mine?

Comment 13

[Girish Sharma [:Optimizer]]

(In reply to Anton Kovalyov (:anton) from comment #11)
> Comment on attachment 8333523 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 8333523 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes to editor.js should be an Editor addon. (See
> sourceeditor/debugger.js for reference.)

I thought of doing so, but the editor.js contains just the framework for autocopmletion, which will be used by both css and js autocompletion (or maybe htmlmixed too). Since it looked like something deeply integrated with the editor, rather than any specific tool, I kept it in editor.js only.

Comment 14

[Girish Sharma [:Optimizer]]

(In reply to Jason Barnabe (np) from comment #12)
> I would like to include this feature in my extension Stylish, which also
> uses Orion/CodeMirror to edit CSS. As this feature is being written, can you
> keep in mind reuse of the code by extensions such as mine?

Yes, it is reusable by extensions.

Comment 15

[Anton Kovalyov (:anton)]

(In reply to Girish Sharma [:Optimizer] from comment #13)
> (In reply to Anton Kovalyov (:anton) from comment #11)
> > Comment on attachment 8333523 [details] [diff] [review]
> > patch v0.1
> > 
> > Review of attachment 8333523 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Changes to editor.js should be an Editor addon. (See
> > sourceeditor/debugger.js for reference.)
> 
> I thought of doing so, but the editor.js contains just the framework for
> autocopmletion, which will be used by both css and js autocompletion (or
> maybe htmlmixed too). Since it looked like something deeply integrated with
> the editor, rather than any specific tool, I kept it in editor.js only.

What about cssAutoCompleter? That doesn't sound very generic to me. In any case, when in future we will have some other auto-completion feature we can always move it into editor.js and make it generic. But for now it should be an addon.

This happened with debugger.js, there were more methods in it but when shader editor needed them we simply moved generic ones over.

Comment 16

[Girish Sharma [:Optimizer]]

(In reply to Anton Kovalyov (:anton) from comment #15)
> (In reply to Girish Sharma [:Optimizer] from comment #13)
> > (In reply to Anton Kovalyov (:anton) from comment #11)
> > > Comment on attachment 8333523 [details] [diff] [review]
> > > patch v0.1
> > > 
> > > Review of attachment 8333523 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Changes to editor.js should be an Editor addon. (See
> > > sourceeditor/debugger.js for reference.)
> > 
> > I thought of doing so, but the editor.js contains just the framework for
> > autocopmletion, which will be used by both css and js autocompletion (or
> > maybe htmlmixed too). Since it looked like something deeply integrated with
> > the editor, rather than any specific tool, I kept it in editor.js only.
> 
> What about cssAutoCompleter? That doesn't sound very generic to me. In any
> case, when in future we will have some other auto-completion feature we can
> always move it into editor.js and make it generic. But for now it should be
> an addon.

I agree the variable name does not sound generic, but any kind of completer, when introduced in source editor will sound out of place :)

> This happened with debugger.js, there were more methods in it but when
> shader editor needed them we simply moved generic ones over.

Sounds good. We can move common functions back to editor.js once js completer starts building up to.

Comment 17

[Anton Kovalyov (:anton)]

Comment on attachment 8333523 [details] [diff] [review]
patch v0.1

Clearing the review? for now.

Comment 18

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

Addresses Anton's request of separating the autocompelting bits into a separate file.

Adds proper comments to the methods.

The state machine is now ready for review.

I will write tests and attach a separate patch containing just tests to speed up the review of this patch.

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 8339468 [details] [diff] [review]
patch v0.2

Review of attachment 8339468 [details] [diff] [review]:
-----------------------------------------------------------------

General comments:

- this is really great stuff. awesome! thank you for your work!
- the selectors suggestions are very useful when working with layouts you haven't seen before.
- inspector is broken if I try to open it after the style editor.
- style editor is broken if I try to open it after the inspector.
- the autocomplete popup is never in the correct location for me, I use the toolbox with zoom. If I reset the toolbox zoom level, the popup shows up at the correct location.

- editing medium sized files is slow, when at the end of the file. For example on ubuntu.com try to edit ubuntu-styles.css which is only ~3600 lines.

- popup text is sometimes misaligned. At the top of ubuntu-styles.css, with default toolbox zoom level, start to type #box-search and see how the suggestion starts to be misaligned. screenshot: http://imgur.com/1a5M9W3

- when I type |.| in the list of classes suggested I see |.| which is surprising.

- on ubuntu.com if I type |.box| I get suggested a number of classes which start with |box| including |.box| itself, which is fine because it exists. However, if I press tab it autocompletes |.box| (nothing really changes), then I have to press tab again to get the second class name I want. Maybe you should check if the thing the user typed and the suggestion are the same - not much point having to skip |.box| with tab to get the second suggestion when I already have typed |.box|.

- in a rule I can type |color: #| and I get suggested color names. I just got |color: #pink|. :)

- related to above, I also got |color: rgba(aliceblue)|

- if I type something partially and I come back later at it I don't have a way to retrigger autocomplete. For example start writing a selector |#|. Move around, go back to it. I should be able to press Tab or Ctrl-Space to get suggestions on demand, without starting to type other characters.

- if I type |a:| I only get ::selection as a suggestion, and the same for other things like |.foo:|. If I start typing |a:f| then I get other pseudo-elements suggested. Why does it show only selection? I expected at least :hover.

As requested, I reviewed StyleSheetEditor.jsm and autocomplete-popup.js changes. These look good. I only have one concern, see below.

::: browser/devtools/shared/autocomplete-popup.js
@@ +393,5 @@
>      }
>    },
>  
> +  get height() {
> +    return this._lineHeight * this.itemCount + this._extraVerticalSpace;

Why not this._panel.clientHeight ?

Comment 20

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #19)
> Comment on attachment 8339468 [details] [diff] [review]
> patch v0.2
> 
> Review of attachment 8339468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General comments:
> 
> - this is really great stuff. awesome! thank you for your work!
> - the selectors suggestions are very useful when working with layouts you
> haven't seen before.

Thanks.

> - inspector is broken if I try to open it after the style editor.
> - style editor is broken if I try to open it after the inspector.

This is because in style editor, i create a new instance of inspector front, whose corresponding actor should also be new, but actor's pool is a set thus this actor is lost. So any request the front makes goes to the first instance of the inspector actor, and then is returned to the first instance of the front, which has no pending requests and thus it ignores/throws it.

This will be solved once I base my patch on top of bug 916443, which brings the walker to toolbox level, so I reuse the same instance.


> - the autocomplete popup is never in the correct location for me, I use the
> toolbox with zoom. If I reset the toolbox zoom level, the popup shows up at
> the correct location.

Will look into this.
 
> - editing medium sized files is slow, when at the end of the file. For
> example on ubuntu.com try to edit ubuntu-styles.css which is only ~3600
> lines.

Filed bug 944499 

> - popup text is sometimes misaligned. At the top of ubuntu-styles.css, with
> default toolbox zoom level, start to type #box-search and see how the
> suggestion starts to be misaligned. screenshot: http://imgur.com/1a5M9W3

This is very weird. Ideally this should never happen. If you try to manage a STR, that would be very helpful. I will also keep an eye out for this.
 
> - when I type |.| in the list of classes suggested I see |.| which is
> surprising.

Will fix.
 
> - on ubuntu.com if I type |.box| I get suggested a number of classes which
> start with |box| including |.box| itself, which is fine because it exists.
> However, if I press tab it autocompletes |.box| (nothing really changes),
> then I have to press tab again to get the second class name I want. Maybe
> you should check if the thing the user typed and the suggestion are the same
> - not much point having to skip |.box| with tab to get the second suggestion
> when I already have typed |.box|.

Nice suggestion. Will do.
 
> - in a rule I can type |color: #| and I get suggested color names. I just
> got |color: #pink|. :)

This should not happen until you try to type "#p" . In your case, did you get the suggestions of color names as soon as you typed "#" ?

Anyways, I will try to see if this can be fixed, because in some css values, different types of values are possible.

> - related to above, I also got |color: rgba(aliceblue)|

What I said above + 

here, I cannot even check for a compulsory space before the token, as the user can do "rgba( a|". I thin I need to have finer states for css values like I have for selectors.


> - if I type something partially and I come back later at it I don't have a
> way to retrigger autocomplete. For example start writing a selector |#|.
> Move around, go back to it. I should be able to press Tab or Ctrl-Space to
> get suggestions on demand, without starting to type other characters.

Yeah, It should be possible, I suggest Ctrl Space to trigger that kind of suggestions. Will add.
 
> - if I type |a:| I only get ::selection as a suggestion, and the same for
> other things like |.foo:|. If I start typing |a:f| then I get other
> pseudo-elements suggested. Why does it show only selection? I expected at
> least :hover.

Will fix.
 
> As requested, I reviewed StyleSheetEditor.jsm and autocomplete-popup.js
> changes. These look good. I only have one concern, see below.
> 
> ::: browser/devtools/shared/autocomplete-popup.js
> @@ +393,5 @@
> >      }
> >    },
> >  
> > +  get height() {
> > +    return this._lineHeight * this.itemCount + this._extraVerticalSpace;
> 
> Why not this._panel.clientHeight ?

Because I do not want to trigger DOM codes and in turn reflows or repaints on every peypress just to know if I need to place the popup above or below.

Comment 21

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Addresses Mihai's review comments : 
 - Proper position of popup when in zoomed in state. The popup position logic is much much simpler now. (For one, I do not have to manually handle the "not enough screen space below the line" issue)
 - Inspector and style editor work in unison. Thanks to Patrick's remote highlighter work.
 - removed empty entries from the list.
 - You can trigger the auto complete popup by pressing "Ctrl + Space" now from any location.
 - Pseudo selectors are properly autocompleted
 - If the first suggestion is the same as the already entered text, pressing TAB goes to second selection instead now.

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 8340641 [details] [diff] [review]
patch v0.3

Review of attachment 8340641 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch updates and bug fixes!

General comments:

- Suggestions for tag names are upper case. We should suggest lower case tag names.

- I cannot click the suggestion item in the popup. I can do this in the web console.

- When the popup is open if I press Escape to close it, the split web console opens. Please make sure the console does not open in such cases.

- On ubuntu.com type |.foo| - you will see some .footer classes being suggested. Now press Backspace. Why does the popup close? I expected to see the suggestions for |.fo|. This is a common usage scenario: user types too many chars (or he might even have a typo), then he presses backspace to get back to the suggestions.

- I still get the color: #pink suggestion.

- I still see the #booxx-search issue. http://imgur.com/1a5M9W3

The only STR I can provide is: open the style editor on ubuntu.com, add a few lines at the top of the first css file. On the second line type char-by-char #box-search slowly. Notice the chars becoming misaligned.

Unfortunately, this bug might be very much related to the overall system config I have.

The StyleSheetEditor.jsm is unchanged - no additional comments. Also, I see you no longer touch the autocomplete popup code. At this point, I'm not sure how relevant my review is for the patch - beyond testing and user experience feedback. I believe Anton and Dave should have much more valuable input for you, in their areas of expertise in the codebase.

Comment 23

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #22)
> Comment on attachment 8340641 [details] [diff] [review]
> patch v0.3
> 
> Review of attachment 8340641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the patch updates and bug fixes!
> 
> General comments:
> 
> - Suggestions for tag names are upper case. We should suggest lower case tag
> names.

Yeah, I can do that.
 
> - I cannot click the suggestion item in the popup. I can do this in the web
> console.

I have not implemented that part yet. (Maybe in followup)
 
> - When the popup is open if I press Escape to close it, the split web
> console opens. Please make sure the console does not open in such cases.

This is a case in many other places too. (Netmonitor for once)
I think I can add stopPropagation in the escape handling code, but I am not sure since Codemirror might want to handle the escape key (?)
 
> - On ubuntu.com type |.foo| - you will see some .footer classes being
> suggested. Now press Backspace. Why does the popup close? I expected to see
> the suggestions for |.fo|. This is a common usage scenario: user types too
> many chars (or he might even have a typo), then he presses backspace to get
> back to the suggestions.

I do not show completions on backspace, delete, up, left, right, down, enter, return keypreses. These generally hamper normal code writing process. Getting the popup on every backspace press might become annoying soon. I think if the user wants the suggestions again, he can press "Ctrl + Space".

> - I still get the color: #pink suggestion.

Do you get the pink suggestion after you type "#p" or just "#". Because I get nothing when I just type '#', but when I type '#p' I get suggestions to the "p" part. Fixing this will require more precise knowledge of all types of css values in the state machine, which is a bit difficult to do correctly.

> - I still see the #booxx-search issue. http://imgur.com/1a5M9W3
> 
> The only STR I can provide is: open the style editor on ubuntu.com, add a
> few lines at the top of the first css file. On the second line type
> char-by-char #box-search slowly. Notice the chars becoming misaligned.
> 
> Unfortunately, this bug might be very much related to the overall system
> config I have.
>

I am sure my testing also includes the same steps. I will retry again.
 
> The StyleSheetEditor.jsm is unchanged - no additional comments. Also, I see
> you no longer touch the autocomplete popup code. At this point, I'm not sure
> how relevant my review is for the patch - beyond testing and user experience
> feedback. I believe Anton and Dave should have much more valuable input for
> you, in their areas of expertise in the codebase.

Well, removing the dependency of openAtScreen from the popup removed anything that was needed to change in the code and that was your suggestions only :) .

Thanks for the testing and the feedback.

Comment 24

[Dave Camp (:dcamp)]

Comment on attachment 8340641 [details] [diff] [review]
patch v0.3

Review of attachment 8340641 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on selector-search and inspector.js.  Existing tests cover those changes, right?

::: toolkit/devtools/server/actors/inspector.js
@@ +1607,5 @@
> +        sugs.classes.delete("");
> +        // since editing the style editor makes the stylesheet have errors and
> +        // thus the page's elements' styles start changing with a transition.
> +        // That transition comes from the `moz-styleeditor-transitioning` class.
> +        sugs.classes.delete("moz-styleeditor-transitioning");

There are a few magic classes we use, maybe we should have a list somewhere.  But maybe we should also delete HIDDEN_CLASS here too.

Comment 25

[Girish Sharma [:Optimizer]]

(In reply to Dave Camp (:dcamp) from comment #24)
> Comment on attachment 8340641 [details] [diff] [review]
> patch v0.3
> 
> Review of attachment 8340641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ on selector-search and inspector.js.  Existing tests cover those changes,
> right?

tests patch is different which fixes 2 broken inspector tests and adds more for autocompletion in style editor.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1607,5 @@
> > +        sugs.classes.delete("");
> > +        // since editing the style editor makes the stylesheet have errors and
> > +        // thus the page's elements' styles start changing with a transition.
> > +        // That transition comes from the `moz-styleeditor-transitioning` class.
> > +        sugs.classes.delete("moz-styleeditor-transitioning");
> 
> There are a few magic classes we use, maybe we should have a list somewhere.
> But maybe we should also delete HIDDEN_CLASS here too.

yes. will do.

Comment 26

[Dave Camp (:dcamp)]

Comment on attachment 8340641 [details] [diff] [review]
patch v0.3

Review of attachment 8340641 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/sourceeditor/css-autocompleter.js
@@ +151,5 @@
> +                let popped = scopeStack.pop();
> +                _state = popped == "f" ? CSS_STATES.frame
> +                                       : (selector = "",
> +                                          selectorState = SELECTOR_STATES.null,
> +                                          CSS_STATES.selector);

This is already dense code, I think executing assignments in a ternary expression is getting a bit much.  If statement maybe? :)

@@ +170,5 @@
> +              break;
> +
> +            case "}":
> +              if (scopeStack.slice(-1)[0] == ":") {
> +                scopeStack.pop();

I think this would be ever so slightly easier to follow if you added a peek() helper function:

function peek(stack) { return stack.slice(-1)[0]; }

Then it would look more like "if (peek(stack) == ":") { stack.pop(); }

@@ +571,5 @@
> +    return _state;
> +  },
> +
> +  /**
> +   * Queris the DOM Walker actor for suggestions regarding the selector being

Queries

@@ +616,5 @@
> +    // Set the values that this request was supposed to suggest to.
> +    this._currentQuery = query;
> +    walker.getSuggestionsForQuery(query, this.completing, this.selectorState)
> +          .then(result => deferred.resolve(this.prepareSelectorResults(result)));
> +    return deferred.promise;

You can just return walker.getSuggestionsForQuery(...).then(result => { return this.prepareSelectorResults(result); }) and not deal with the deferred here.

@@ +689,5 @@
> +        // We have crossed all possible matches alphabetically.
> +        break;
> +      }
> +    }
> +    deferred.resolve(finalList);

You can just return Promise.resolve(finalList) rather than creating a deferred here.

@@ +721,5 @@
> +        break;
> +      }
> +    }
> +    deferred.resolve(finalList);
> +    return deferred.promise;

Same here.

Comment 27

[Anton Kovalyov (:anton)]

Comment on attachment 8340641 [details] [diff] [review]
patch v0.3

Review of attachment 8340641 [details] [diff] [review]:
-----------------------------------------------------------------

I have some architectural questions along with a few nits.

::: browser/devtools/sourceeditor/autocomplete.js
@@ +1,1 @@
> +/* vim:set ts=2 sw=2 sts=2 et tw=80:

You should write a longer comment describing the overall architecture of how auto-completion work. I'm thinking something like "on load we tokenize the source, when you press [tab] we do such and such". This way a newcomer won't need to dig deep into your code to understand overall architecture.

@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const cssAutoCompleter  = require("devtools/sourceeditor/css-autocompleter");

Nit: you have two spaces here but it is still not aligned with the line below.

@@ +5,5 @@
> +
> +const cssAutoCompleter  = require("devtools/sourceeditor/css-autocompleter");
> +const { AutocompletePopup } = require("devtools/shared/autocomplete-popup");
> +
> +const Editor = {

Why is this here? We should use the one in editor.js.

@@ +38,5 @@
> +        return void cycleSuggestions(ed, true);
> +
> +      return win.CodeMirror.Pass;
> +    },
> +    "Ctrl-Space": cm => autoComplete(ctx)

Is this ctrl-space everywhere? (Not cmd-space on macs?) Do we need to localize these shortcuts? Editor object has static methods for that now.

@@ +40,5 @@
> +      return win.CodeMirror.Pass;
> +    },
> +    "Ctrl-Space": cm => autoComplete(ctx)
> +  });
> +  switch (ed.config.mode.name) {

This switch should be an 'if'.

@@ +95,5 @@
> +    if (reverse) {
> +      firstItem = ed.popup.getItemAtIndex(ed.popup.itemCount - 1);
> +      ed.popup.selectPreviousItem();
> +    }
> +    else {

The sourceeditor component code doesn't use }\nelse anywhere else.

@@ +135,5 @@
> +    case event.DOM_VK_DELETE:
> +    case event.DOM_VK_ENTER:
> +    case event.DOM_VK_RETURN:
> +    case event.DOM_VK_ESCAPE:
> +      ed._doNotAutocomplete = true;

Is this a setting? You should use either Editor.config or a local WeakMap (like dbginfo in debugger.js). Alternatively, we should add a more proper way to have private data within the Editor instance.

::: browser/devtools/sourceeditor/editor.js
@@ +170,5 @@
>      var num = cm.getOption("indentUnit");
>      if (cm.getCursor().ch !== 0) num -= 1;
>      cm.replaceSelection(" ".repeat(num), "end", "+input");
>    };
> +  this.config.extraKeys["Shift-Tab"] = cm => cm.indentSelection("subtract");

How is this related to this patch?

Comment 28

[Girish Sharma [:Optimizer]]

(In reply to Anton Kovalyov (:anton, @valueof) from comment #27)
> Comment on attachment 8340641 [details] [diff] [review]
> patch v0.3
> 
> Review of attachment 8340641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some architectural questions along with a few nits.
> 
> ::: browser/devtools/sourceeditor/autocomplete.js
> @@ +1,1 @@
> > +/* vim:set ts=2 sw=2 sts=2 et tw=80:
> 
> You should write a longer comment describing the overall architecture of how
> auto-completion work. I'm thinking something like "on load we tokenize the
> source, when you press [tab] we do such and such". This way a newcomer won't
> need to dig deep into your code to understand overall architecture.

Sure.

> @@ +2,5 @@
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +const cssAutoCompleter  = require("devtools/sourceeditor/css-autocompleter");
> 
> Nit: you have two spaces here but it is still not aligned with the line
> below.

2 space is by mistake. its one space only.

> @@ +5,5 @@
> > +
> > +const cssAutoCompleter  = require("devtools/sourceeditor/css-autocompleter");
> > +const { AutocompletePopup } = require("devtools/shared/autocomplete-popup");
> > +
> > +const Editor = {
> 
> Why is this here? We should use the one in editor.js.

How can I reuse it ? This file does not imports editor.js to get that variable.

> @@ +38,5 @@
> > +        return void cycleSuggestions(ed, true);
> > +
> > +      return win.CodeMirror.Pass;
> > +    },
> > +    "Ctrl-Space": cm => autoComplete(ctx)
> 
> Is this ctrl-space everywhere? (Not cmd-space on macs?) Do we need to
> localize these shortcuts? Editor object has static methods for that now.

will do.
 
> @@ +40,5 @@
> > +      return win.CodeMirror.Pass;
> > +    },
> > +    "Ctrl-Space": cm => autoComplete(ctx)
> > +  });
> > +  switch (ed.config.mode.name) {
> 
> This switch should be an 'if'.

Okay. (You have switch-es, don't you :) )
 
> @@ +95,5 @@
> > +    if (reverse) {
> > +      firstItem = ed.popup.getItemAtIndex(ed.popup.itemCount - 1);
> > +      ed.popup.selectPreviousItem();
> > +    }
> > +    else {
> 
> The sourceeditor component code doesn't use }\nelse anywhere else.

 
> @@ +135,5 @@
> > +    case event.DOM_VK_DELETE:
> > +    case event.DOM_VK_ENTER:
> > +    case event.DOM_VK_RETURN:
> > +    case event.DOM_VK_ESCAPE:
> > +      ed._doNotAutocomplete = true;
> 
> Is this a setting? You should use either Editor.config or a local WeakMap
> (like dbginfo in debugger.js). Alternatively, we should add a more proper
> way to have private data within the Editor instance.

Its not a setting. Just a variable to help out the ux flow.

> ::: browser/devtools/sourceeditor/editor.js
> @@ +170,5 @@
> >      var num = cm.getOption("indentUnit");
> >      if (cm.getCursor().ch !== 0) num -= 1;
> >      cm.replaceSelection(" ".repeat(num), "end", "+input");
> >    };
> > +  this.config.extraKeys["Shift-Tab"] = cm => cm.indentSelection("subtract");
> 
> How is this related to this patch?

Initially, it was, when I had the suggestions cycling methods in this file itself. But now its just fixing a bug that Shift+Tab does not do what it was doing before CM.

Comment 29

[Girish Sharma [:Optimizer]]

Attachment: part-1-no-tests-v0.5

Sorry for the long delay, but I had pretty bad test failures which took me a while to sort out as many of them were not reproducible locally.

Here are the changes in this patch as compared to the previous one:

- Chained some of the promises in Inspector selector search and Style editor initialization bits as otherwise, race conditions were happening.
- Addressed Anton's review comments (except for the big explanation comment in the autocompeltion file, explaining the code a bit)
- Fixed the issues listed by Mihai that I was previously not able to reproduce.

Asking for re-review from Dave and Anton, and an extra review from Heather as I refactored the stylesheeteditor in terms of promises a bit.

Comment 30

[Girish Sharma [:Optimizer]]

Attachment: part-2-tests-v0.1

Added 2 unit tests for the auto completion api and the state machine.

try build at : https://tbpl.mozilla.org/?tree=Try&rev=3a60c505c190

almost green try without the tests patch at : https://tbpl.mozilla.org/?tree=Try&rev=111417bb1c8a

functional test for the style editor part in progress.

Comment 31

[Anton Kovalyov (:anton)]

Comment on attachment 8358053 [details] [diff] [review]
part-1-no-tests-v0.5

Review of attachment 8358053 [details] [diff] [review]:
-----------------------------------------------------------------

Reviewed sourceeditor/* changes. Most are nits, but not having popup and completer as public properties on the Editor instance is a big deal.

::: browser/devtools/sourceeditor/autocomplete.js
@@ +8,5 @@
> +
> +const privates = new WeakMap();
> +
> +/**
> + * Prepares an editor instance for autocopmletion, setting up the popup and the

Typo in 'autocompletion'.

@@ +21,5 @@
> +  let win = ed.container.contentWindow.wrappedJSObject;
> +  let keyMap = {
> +    "Tab": cm => {
> +      if (ed.popup && ed.popup.isOpen)
> +        return void cycleSuggestions(ed);

I guess 'return void <expr>' is no good because of SpiderMonkey's strict mode warning system that nobody uses.

@@ +37,5 @@
> +  cm.addKeyMap(keyMap);
> +  if (ed.config.mode == Editor.modes.css)
> +    ed.completer = new cssAutoCompleter({walker: walker});
> +
> +  ed.popup = new AutocompletePopup(win.parent.document, {

Why is this a public property on the Editor instance? (Same goes for the completer.) I don't see these properties being accesses elsewhere so I think they should be part of the 'privates' WeakMap.

@@ +60,5 @@
> + * Provides suggestions to autocomplete the current token/word being typed.
> + */
> +function autoComplete({ ed, cm }) {
> +  let private = privates.get(ed);
> +  if (!ed.completer || private.insertingSuggestion || private.doNotAutocomplete) {

To re-iterate my point from the above: I don't understand why insertionSuggestion and doNotAutocomplete come from 'private' but compelter comes from 'ed' variable.

@@ +66,5 @@
> +    return;
> +  }
> +  let cur = ed.getCursor();
> +  ed.completer.complete(cm.getRange({line: 0, ch: 0}, cur), cur)
> +    .then(suggestions => {

This looks super weird to me because the next line is inside the function but it's not indented as such.

@@ +75,5 @@
> +    }
> +    let left = suggestions[0].preLabel.length * cm.defaultCharWidth() + 4;
> +    ed.popup.hidePopup();
> +    ed.popup.setItems(suggestions);
> +    ed.popup.openPopup(cm.display.cursor, -1 * left, 0);

What are these magic numbers?

@@ +84,5 @@
> +/**
> + * Cycles through provided suggestions by the popup in a top to bottom manner
> + * when `reverse` is not true. Opposite otherwise.
> + */
> +function cycleSuggestions(ed, reverse) {

I can't tell if it this function just goes to the next (or previous) suggestion or really cycles through them (i.e. if you're on the last item and hit 'down' it goes to the first one).

::: browser/devtools/sourceeditor/css-autocompleter.js
@@ +13,5 @@
> +  property: "property",    // foo { bar|: … }
> +  value: "value",          // foo {bar: baz|}
> +  selector: "selector",    // f| {bar: baz}
> +  media: "media",          // @med| , or , @media scr| { }
> +  keyframes: "keyframes",    // @keyf|

Misaligned.

@@ +37,5 @@
> + *        - walker {Object} The object used for query selecting from the current
> + *                 target's DOM.
> + *        - maxEntries {Number} Maximum selectors suggestions to display.
> + */
> +function CSSCompleter(options = {}) {

Again, unless I spend more time reading through this code or go line-by-line in the debugger, I will have no idea how it works. Overall algorithm description would be *really* helpful here for other people on the team, new contributors and yourself three months later.

::: browser/devtools/sourceeditor/css-tokenizer.js
@@ +1,1 @@
> +/**

Ask Mark if you need secreview for this. Also I think you need to email our licensing team.

Are we going to import any unit tests for this file? We should.

::: browser/devtools/sourceeditor/editor.js
@@ +761,5 @@
>     */
>    extend: function (funcs) {
>      Object.keys(funcs).forEach((name) => {
>        let cm  = editors.get(this);
> +      let ctx = { ed: this, cm: cm , Editor: Editor};

Nit: space between cm and a comma.

Comment 32

[Anton Kovalyov (:anton)]

Comment on attachment 8358054 [details] [diff] [review]
part-2-tests-v0.1

Review of attachment 8358054 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with my comments addressed.

::: browser/devtools/sourceeditor/test/browser_css_statemachine.js
@@ +12,5 @@
> +const TESTS_URI = "http://mochi.test:8888/browser/browser/devtools/sourceeditor" +
> +                  "/test/css_statemachine_tests.json";
> +
> +const source = read(CSS_URI);
> +const tests = eval(read(TESTS_URI));

JSON.parse instead of eval? You'll have to change single quotes to double quotes though.

::: browser/devtools/sourceeditor/test/head.js
@@ +4,5 @@
>  
>  "use strict";
>  
> +const devtools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> +const require = devtools.require;

This seems unnecessary, we have 80 < lines < 100 everywhere in the code.

@@ +45,5 @@
>    win.close();
>    finish();
> +}
> +
> +function limit(source, [line, ch]) {

Write a comment explaining that limit returns a portion of a source up to the [line, ch] position. Also [line, ch] could be {line, ch}.

Comment 33

[Girish Sharma [:Optimizer]]

(In reply to Anton Kovalyov (:anton, @valueof) from comment #31)
> Comment on attachment 8358053 [details] [diff] [review]
> part-1-no-tests-v0.5
> 
> Review of attachment 8358053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Reviewed sourceeditor/* changes. Most are nits, but not having popup and
> completer as public properties on the Editor instance is a big deal.

> @@ +21,5 @@
> > +  let win = ed.container.contentWindow.wrappedJSObject;
> > +  let keyMap = {
> > +    "Tab": cm => {
> > +      if (ed.popup && ed.popup.isOpen)
> > +        return void cycleSuggestions(ed);
> 
> I guess 'return void <expr>' is no good because of SpiderMonkey's strict
> mode warning system that nobody uses.

Okay.

> @@ +37,5 @@
> > +  cm.addKeyMap(keyMap);
> > +  if (ed.config.mode == Editor.modes.css)
> > +    ed.completer = new cssAutoCompleter({walker: walker});
> > +
> > +  ed.popup = new AutocompletePopup(win.parent.document, {
> 
> Why is this a public property on the Editor instance? (Same goes for the
> completer.) I don't see these properties being accesses elsewhere so I think
> they should be part of the 'privates' WeakMap.

The other properties which I have already moved to private weakmap were meaningless to the outside world and were present for the proper functioning of the feature. Popup and completer on the other hand might be useful to other editor addons (or actual firefox addons) who want to modify the behavior, or change something in the popup, etc.

The popup, specially, can also be used by other things (which I cannot right now think of).

If you still think that they should be private, I can make them private.

> @@ +66,5 @@
> > +    return;
> > +  }
> > +  let cur = ed.getCursor();
> > +  ed.completer.complete(cm.getRange({line: 0, ch: 0}, cur), cur)
> > +    .then(suggestions => {
> 
> This looks super weird to me because the next line is inside the function
> but it's not indented as such.

The .then part is not inside the complete method, its like 

ed.completer.complete(args).then(suggestions => {

Thus, I am indenting with the above dot.


> @@ +75,5 @@
> > +    }
> > +    let left = suggestions[0].preLabel.length * cm.defaultCharWidth() + 4;
> > +    ed.popup.hidePopup();
> > +    ed.popup.setItems(suggestions);
> > +    ed.popup.openPopup(cm.display.cursor, -1 * left, 0);
> 
> What are these magic numbers?

The cursor is at the end of the currently entered part of the token, like "backgr"

but we need to open the popup at the beginning of the character "b". Thus the calculation.

4 comes from the popup's padding.
 
> @@ +84,5 @@
> > +/**
> > + * Cycles through provided suggestions by the popup in a top to bottom manner
> > + * when `reverse` is not true. Opposite otherwise.
> > + */
> > +function cycleSuggestions(ed, reverse) {
> 
> I can't tell if it this function just goes to the next (or previous)
> suggestion or really cycles through them (i.e. if you're on the last item
> and hit 'down' it goes to the first one).

I dont't get your query. This method cycles suggestions, if you are on last one, pressing down moves to first one. That is what cycling means :)


> @@ +37,5 @@
> > + *        - walker {Object} The object used for query selecting from the current
> > + *                 target's DOM.
> > + *        - maxEntries {Number} Maximum selectors suggestions to display.
> > + */
> > +function CSSCompleter(options = {}) {
> 
> Again, unless I spend more time reading through this code or go line-by-line
> in the debugger, I will have no idea how it works. Overall algorithm
> description would be *really* helpful here for other people on the team, new
> contributors and yourself three months later.

That is what I said in the comment, this patch is without that description. Will add in the final one.

> ::: browser/devtools/sourceeditor/css-tokenizer.js
> @@ +1,1 @@
> > +/**
> 
> Ask Mark if you need secreview for this. Also I think you need to email our
> licensing team.
> 
> Are we going to import any unit tests for this file? We should.

Licensing team part is done. See comment 8 . What is the reason for a sec review ?

Also, I asked Tab, he says he has no unit tests. That is why I wrote thorough unit tests for the state machine, the actual consumer of the tokenizer.

Comment 34

[Girish Sharma [:Optimizer]]

(In reply to Anton Kovalyov (:anton, @valueof) from comment #32)
> Comment on attachment 8358054 [details] [diff] [review]
> part-2-tests-v0.1
> 
> Review of attachment 8358054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with my comments addressed.
> 
> ::: browser/devtools/sourceeditor/test/browser_css_statemachine.js
> @@ +12,5 @@
> > +const TESTS_URI = "http://mochi.test:8888/browser/browser/devtools/sourceeditor" +
> > +                  "/test/css_statemachine_tests.json";
> > +
> > +const source = read(CSS_URI);
> > +const tests = eval(read(TESTS_URI));
> 
> JSON.parse instead of eval? You'll have to change single quotes to double
> quotes though.

And remove all the comments from the file. I wanted to add comments to the file explaining the format of the test cases and thus shifted to eval.

> ::: browser/devtools/sourceeditor/test/head.js
> @@ +4,5 @@
> >  
> >  "use strict";
> >  
> > +const devtools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> > +const require = devtools.require;
> 
> This seems unnecessary, we have 80 < lines < 100 everywhere in the code.

Okay.
 
> @@ +45,5 @@
> >    win.close();
> >    finish();
> > +}
> > +
> > +function limit(source, [line, ch]) {
> 
> Write a comment explaining that limit returns a portion of a source up to
> the [line, ch] position. Also [line, ch] could be {line, ch}.

It could be, but then I would have to do extra work at the calling place :

{line: test[i][0], ch: test[i][1]}

instead of just 'test' :(

Comment 35

[Anton Kovalyov (:anton)]

> If you still think that they should be private, I can make them private.

Yes, you should make them private. It is always easier to make expose private property when its needed then to hide a public property.

> The cursor is at the end of the currently entered part of the token, like "backgr"

You should write that as a comment there.

> That is what I said in the comment, this patch is without that description. Will add in the final one.

Ah ok.

> Licensing team part is done. See comment 8 . What is the reason for a sec review?

I remember someone telling me that we always need to ask sec team about potential review when we add external source into the tree.

> Also, I asked Tab, he says he has no unit tests. That is why I wrote thorough unit tests for the state machine, the actual consumer of the tokenizer.

No unit tests. :( Okay, I guess.

Comment 36

[Dave Camp (:dcamp)]

Comment on attachment 8358053 [details] [diff] [review]
part-1-no-tests-v0.5

Review of attachment 8358053 [details] [diff] [review]:
-----------------------------------------------------------------

Followup to my nits are fine.

Comment 37

[Heather Arthur [:harth]]

Comment on attachment 8358053 [details] [diff] [review]
part-1-no-tests-v0.5

Review of attachment 8358053 [details] [diff] [review]:
-----------------------------------------------------------------

Just these two things:

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +95,5 @@
>    /**
> +   * Initiates the style editor ui creation and the inspector front to get
> +   * reference to the walker.
> +   */
> +  initiate: function() {

Kinda silly, but it's usually "initialize". Initiate means something slightly different, like starting off a process of some kind.

@@ +420,5 @@
>      let sheet = this._styleSheetToSelect;
>  
>      for each (let editor in this.editors) {
>        if (editor.styleSheet.href == sheet.href) {
> +        this._styleSheetBoundToSelect = this._styleSheetToSelect;

Can you explain the need for `styleSheetBoundToSelect`? It might need a better name, as I can't guess what it's doing.

Comment 38

[Girish Sharma [:Optimizer]]

Attachment: part-1-no-tests-v0.6

Anton, final review request.

Asking for sec review for an external file being added into the source tree

File name : "css-tokenizer.js"
Purpose: To take in CSS source and spit out tokens with meaningful tokens.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I don't see any place for an exploit due to the file css-tokenizer.js

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Nopes

Which older supported branches are affected by this flaw? Nothing like that.

If not all supported branches, which bug introduced the flaw? No flaw

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? no risk. no backport.

How likely is this patch to cause regressions; how much testing does it need? Its a new feature. no regressions possible until existing tests are passing

Comment 39

[Girish Sharma [:Optimizer]]

Attachment: part-2-tests-v0.2

Addressed review comments.

Comment 40

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8361762 [details] [diff] [review]
part-1-no-tests-v0.6

Clearing sec-approval. Sec-approval is used for clearance to check in security sensitive bugs into trunk. It doesn't denote a request for a security review. Since this isn't a hidden security bug, it isn't the right flag.

I'm needinfo'ing Curtis to discuss how to get a security review.

Comment 41

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

this is the draft for the current process
https://wiki.mozilla.org/Security/Process/Secreview_Bug_Process

That said I'll clear my name from the flag and this will get triaged for a proper owner at our meeting next wed.

Comment 42

[Girish Sharma [:Optimizer]]

(needinfo'ing for sec-review for newly added external file)

Comment 43

[Girish Sharma [:Optimizer]]

Attachment: part-3-styleeditor-test-v0.1

Added a functionality test for style editor.

Asking Anton for review too as I had to make some changes in files in aourceeditor folder for the tests to work properly. For ex. adding a method to return the private popup element.

Final try with all the patches : https://tbpl.mozilla.org/?tree=Try&rev=3fdd215204d3

Comment 44

[Anton Kovalyov (:anton)]

Comment on attachment 8362367 [details] [diff] [review]
part-3-styleeditor-test-v0.1

Review of attachment 8362367 [details] [diff] [review]:
-----------------------------------------------------------------

Not too crazy about event names (after-suggest etc.) but I don't have better alternatives.

Comment 45

[Girish Sharma [:Optimizer]]

Comment on attachment 8362367 [details] [diff] [review]
part-3-styleeditor-test-v0.1

(for faster review)

Comment 46

[Girish Sharma [:Optimizer]]

Comment on attachment 8362367 [details] [diff] [review]
part-3-styleeditor-test-v0.1

(my bad for not pinging Heather first to confirm if she would be able to review or not)

Thanks again everyone for sticking around and reviewing the patches

Comment 47

[Girish Sharma [:Optimizer]]

gentle ping for sec review.

or can I land this, while sec review still goes on ?

Comment 48

[Rob Campbell [:rc] (:robcee)]

hey Optimizer,

Can you put a killswitch pref on this so we can turn it off if we find any issues with it? Sorry for the late addition, but it'd make landing ahead of sec-review easier.

thank you! <3

Comment 49

[Girish Sharma [:Optimizer]]

Attachment: part-1-no-tests-v0.7

Added a kill switch.

Only disturbing Rob this time for review ;)

Comment 50

[Girish Sharma [:Optimizer]]

Attachment: part-3-styleeditor-test-v0.2

Added a check that the popup element is not at all there when completion is disabled.

Comment 51

[Girish Sharma [:Optimizer]]

Attachment: part-3-styleeditor-test-v0.3

(ughh, removed a file by accident)

Comment 52

[Girish Sharma [:Optimizer]]

Attachment: part-3-styleeditor-test-v0.4

Comment 53

[Mark Goodwin [:mgoodwin]]

Looks good.

Comment 54

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8366203 [details] [diff] [review]
part-1-no-tests-v0.7

Review of attachment 8366203 [details] [diff] [review]:
-----------------------------------------------------------------

alright, looks good. (just reviewed the checkbox and pref check in detail. relying on previous reviews for contents).

Comment 55

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8366210 [details] [diff] [review]
part-3-styleeditor-test-v0.4

Review of attachment 8366210 [details] [diff] [review]:
-----------------------------------------------------------------

nothing too scary in here.

Should be good to go with a successful try run.

Comment 56

[Girish Sharma [:Optimizer]]

Thanks everyone.

latest try is also green : https://tbpl.mozilla.org/?tree=Try&rev=85c8aeb1197e

Will land this in couple of hours.

Comment 57

[Girish Sharma [:Optimizer]]

landed in fx-team :

https://hg.mozilla.org/integration/fx-team/rev/fcbf8c877de9
https://hg.mozilla.org/integration/fx-team/rev/025d4a4b4223
https://hg.mozilla.org/integration/fx-team/rev/59cc8ecd78d3

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fcbf8c877de9
https://hg.mozilla.org/mozilla-central/rev/025d4a4b4223
https://hg.mozilla.org/mozilla-central/rev/59cc8ecd78d3

Comment 59

[Jason Barnabe (np)]

(In reply to Girish Sharma [:Optimizer] from comment #14)
> (In reply to Jason Barnabe (np) from comment #12)
> > I would like to include this feature in my extension Stylish, which also
> > uses Orion/CodeMirror to edit CSS. As this feature is being written, can you
> > keep in mind reuse of the code by extensions such as mine?
> 
> Yes, it is reusable by extensions.

After trying with nightly, this doesn't seem to be automatically enabled for Stylish's editor. What do I have to do to make it work?

Comment 60

[Girish Sharma [:Optimizer]]

After you append the codemirror editor into the dom node (basically calling appendTo method), do something like :

editor.extend(AutoCompleter);
editor.setupAutoCompletion(walker);

You can pass mull instead of `walker` part if you do not want selector completion, otherwise you will have to get the walker from the toolbox like :

let toolbox = gDevTools.getToolbox(target);
return toolbox.initInspector().then(() => {
 walker = toolbox.walker;
})

So assuming you make the proper imports in your file, your final code might look like :

let { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
let target = devtools.TargetFactory.forTab(gBrowser.selectedTab);
target.makeRemote().then(() => {
  editor.appendTo(inputElement).then(() => {
    let toolbox = gDevTools.getToolbox(target);
    return toolbox.initInspector().then(() => {
      walker = toolbox.walker;
      editor.extend(AutoCompleter);
      editor.setupAutoCompletion(walker);
    })
 });
});

This is assuming that the toolbox is open for the tab.
Unfortunately, there is no good way to get hold of the walker.

Comment 61

[Jason Barnabe (np)]

Stylish doesn't work with a toolbox nor with a specific document. After exchanging some e-mails back and forth with Girish, here's what I did to get it working in Stylish - https://github.com/JasonBarnabe/stylish/commit/6159874530dfaba1e01499b5f0c8587c64131acb