734439 - Add code folding support to the source editor

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P3)

Description

[Mihai Sucan [:msucan]]

Orion upstream has some support for code folding. We should look into that and see if it's ready to be enabled in the source editor.

We should start with JavaScript support, but if it's easy to do, then CSS as well.

Comment 1

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

Moving to Source Editor component.

Filter on CHELICERAE.

Comment 2

[Anton Kovalyov (:anton)]

Attachment: fold.patch

Added foldcode and foldgutter CodeMirror addons and enabled folding in Scratchpad. I will file bugs for other tools in their respective components once this thing is landed. Style editor should be simple but with the debugger there might be issues with breakpoints and folded code, I'm not sure.

Test only checks that the addons are there and activated. They don't test their behavior.

Reviews: Mihai for the editor and Benvie for Scratchpad.

Try: https://tbpl.mozilla.org/?tree=Try&rev=6a365b49608b

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 8360097 [details] [diff] [review]
fold.patch

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

Patch looks good. Only some questions below...

::: browser/devtools/sourceeditor/codemirror/fold/foldcode.js
@@ +1,2 @@
> +(function() {
> +  "use strict";

These new codemirror files come from the CM codebase without changes? Should we review these?

@@ +52,5 @@
> +    }
> +    return widget;
> +  }
> +
> +  // Clumsy backwards-compatible interface

Do we need any sort of backwards compat in our CM copy?

::: browser/devtools/sourceeditor/codemirror/mozilla.css
@@ +93,5 @@
> +
> +.CodeMirror-foldmarker {
> +  color: blue;
> +  text-shadow: #b9f 1px 1px 2px, #b9f -1px -1px 2px, #b9f 1px -1px 2px, #b9f -1px 1px 2px;
> +  font-family: arial;

Rather odd to see arial here. Why not a generic font family like sans-serif?

@@ +110,5 @@
> +}
> +
> +.CodeMirror-foldgutter-open:after {
> +  font-size: 120%;
> +  content: "\25BE";

Can we rely on these unicode characters to be available in the font?

::: browser/devtools/sourceeditor/test/browser_editor_addons.js
@@ +23,5 @@
>    });
> +}
> +
> +function testFold(doc, ed, win) {
> +  // Wait until folding arrow is there.

Isn't there any event you can listen for?

Comment 4

[Brandon Benvie [:benvie]]

Comment on attachment 8360097 [details] [diff] [review]
fold.patch

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

Aside from the comment below Scratchpad changes LGTM.

::: browser/devtools/scratchpad/scratchpad.js
@@ +1495,5 @@
> +
> +    if (Services.prefs.getBoolPref(ENABLE_CODE_FOLDING)) {
> +      config.foldGutter = true;
> +      config.gutters = ["CodeMirror-linenumbers", "CodeMirror-foldgutter"];
> +    }

Maybe in a followup/future bug: Is there a way we can abstract over this so that the Scratchpad doesn't need to know about CodeMirror classes? I think we'll want to have code folding in most places CM is used and I'm guessing the above few lines will be the same or very similar for all of them.

That brings up a second question, which is whether we want a pref for each place the editor is used to control folding, or a single pref that applies to all editors.

Comment 5

[Anton Kovalyov (:anton)]

(In reply to Mihai Sucan [:msucan] from comment #3)
> 
> ::: browser/devtools/sourceeditor/codemirror/fold/foldcode.js
> @@ +1,2 @@
> > +(function() {
> > +  "use strict";
> 
> These new codemirror files come from the CM codebase without changes? Should
> we review these?

They are without changes. I review them myself (mostly to make sure I'm familiar with how things work) and I also make sure they don't have innerHTML and other potentially insecure features. That said, I'm open to requesting another secreview for all the addons we've landed lately.

> @@ +52,5 @@
> > +    }
> > +    return widget;
> > +  }
> > +
> > +  // Clumsy backwards-compatible interface
> 
> Do we need any sort of backwards compat in our CM copy?

These files are bundled with the 3.20 distribution of CodeMirror. If, in future, CM removes these APIs all addons will be updated within the same release. So as long as we use addons from the same distribution as the main code, we're okay.

> ::: browser/devtools/sourceeditor/codemirror/mozilla.css
> @@ +93,5 @@
> > +
> > +.CodeMirror-foldmarker {
> > +  color: blue;
> > +  text-shadow: #b9f 1px 1px 2px, #b9f -1px -1px 2px, #b9f 1px -1px 2px, #b9f -1px 1px 2px;
> > +  font-family: arial;
> 
> Rather odd to see arial here. Why not a generic font family like sans-serif?

Good point, I'll change it to sans-serif.
 
> @@ +110,5 @@
> > +}
> > +
> > +.CodeMirror-foldgutter-open:after {
> > +  font-size: 120%;
> > +  content: "\25BE";
> 
> Can we rely on these unicode characters to be available in the font?

Yeah, I checked these chars (triangles) and they seem to be very well supported.

> 
> ::: browser/devtools/sourceeditor/test/browser_editor_addons.js
> @@ +23,5 @@
> >    });
> > +}
> > +
> > +function testFold(doc, ed, win) {
> > +  // Wait until folding arrow is there.
> 
> Isn't there any event you can listen for?

Unfortunately, not without making modification to the addon we're using.

Comment 6

[Anton Kovalyov (:anton)]

(In reply to Brandon Benvie [:benvie] from comment #4)
> Comment on attachment 8360097 [details] [diff] [review]
> fold.patch
> 
> Review of attachment 8360097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Aside from the comment below Scratchpad changes LGTM.
> 
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +1495,5 @@
> > +
> > +    if (Services.prefs.getBoolPref(ENABLE_CODE_FOLDING)) {
> > +      config.foldGutter = true;
> > +      config.gutters = ["CodeMirror-linenumbers", "CodeMirror-foldgutter"];
> > +    }
> 
> Maybe in a followup/future bug: Is there a way we can abstract over this so
> that the Scratchpad doesn't need to know about CodeMirror classes? I think
> we'll want to have code folding in most places CM is used and I'm guessing
> the above few lines will be the same or very similar for all of them.

Yes, definitely. Filed bug 960270.

> That brings up a second question, which is whether we want a pref for each
> place the editor is used to control folding, or a single pref that applies
> to all editors.

I don't think it should be a catch-all pref because addon developers who use Editor instance might not want code folding. We can make it a devtools-all pref but that's after we make sure it doesn't interfere with breakpoints in the debugger and so on.

Comment 7

[Anton Kovalyov (:anton)]

Attachment: WIP 2

Changed arial to sans-serif and rebased. Ready to merge once tree re-opens.

Comment 8

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/9e72e83797e2 (try was green)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e72e83797e2