Console's input should be a source editor / CodeMirror instance

RESOLVED FIXED in Firefox 62

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: fitzgen, Assigned: nchevobbe)

Tracking

(Blocks 3 bugs)

unspecified
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [boogaloo-mvp] [polish-backlog])

Attachments

(1 attachment)

That way we would get syntax highlighting and paren matching when writing snippets of code.

Would be especially useful for folks who use shift+return to get multiline inputs.
I believe Joe's (and now Mike's!) JSTerm rewrite does include syntax highlighting? Is that correct?

I also remember Joe saying that using the source editor component wasn't a good idea, but I don't remember the specifics.
I've been thinking of this idea as well. Actually shortly discussed it with Rob.

The idea of using a source editor for the whole JSTerm output is not ideal, as we saw with Paul's JSTerm addon.

However, using the source editor strictly for the JS input field does make sense. You get syntax highlighting and it's quite nice for multiline editing.

This is easily doable [1] with the web console. We can switch to the source editor. An added benefit is that we can also take a snapshot of the syntax highlighted input and put it in the output as static markup, so we get the user-typed JS input with highlighting in the output as well, not just in the input field.

However, Panos does point out JSTerm will come soon. So I am not sure if we should do this in the console now. I discussed with Mike and the plan is for JSTerm to reuse code as much as possible from the web console. That's actually good - recent output rewrite work will be of benefit to JSTerm.

[1] famous last words
(In reply to Mihai Sucan [:msucan] from comment #2)
> However, using the source editor strictly for the JS input field does make
> sense.

Yes, this is exactly what I meant. Didn't mean to imply the whole console should be a source editor.
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Whiteboard: [polish-backlog]
Summary: Console's input should be a source editor instance → Console's input should be a source editor / code mirror instance
Summary: Console's input should be a source editor / code mirror instance → Console's input should be a source editor / CodeMirror instance
There's an old and bitrotted patch here that shows approximately what needs to be done: https://github.com/bgrins/devtools-patches/blob/master/use-cm-in-console
Depends on: 1463409
Blocks: 1463669
This bug should only be about having the input as a CodeMirror editor. It's safe to do so since the feature will be behind a flag. We'll deal with history and autocompletion in follow-ups bug
Blocks: 1463672
Blocks: 1463674
Priority: P3 → P2
Whiteboard: [polish-backlog] → [boogaloo-mvp] [polish-backlog]
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8980648 [details]
Bug 983473 - Put a CodeMirror instance in JsTerm; .

https://reviewboard.mozilla.org/r/246802/#review252954

A few notes, but r+ since it looks good overall and is off by default

::: commit-message-bf476:10
(Diff revision 1)
> +The styles should stay the same, except that now we don't have
> +to do the computation for the input height, since they're already
> +done in CodeMirror. In-line style, history navigation and
> +autocompletion will be handled in separate bugs.
> +The creation of the editor might be done outside of the JsTerm in
> +the future so we can re-use it to syntax highligh Evaluation input

Typo: 'highlight'

::: devtools/client/webconsole/components/JSTerm.js:293
(Diff revision 1)
>    }
>  
>    focus() {
> -    if (this.inputNode && !this.inputNode.getAttribute("focused")) {
> +    if (this.props.codeMirrorEnabled && this.editor) {
> +      try {
> +        // Putting the focus in a try/catch as this might get called when the editor is

Why is this.editor set in the constructor, when we can't append it until componentDidMount? If we were able wait to initialize it until componentDidMount then checking `this.editor` may avoid unpredictable state and this try/catch

::: devtools/client/webconsole/components/JSTerm.js:606
(Diff revision 1)
>     *        The new value to set.
>     * @returns void
>     */
>    setInputValue(newValue) {
> +    if (this.props.codeMirrorEnabled) {
> +      if (this.editor) {

Relatedly: in what case would `this.editor` not exist?

::: devtools/client/webconsole/components/JSTerm.js:1370
(Diff revision 1)
> +    if (this.props.codeMirrorEnabled) {
> +      return dom.div({
> +        className: "jsterm-input-container devtools-monospace",
> +        key: "jsterm-container",
> +        style: {direction: "ltr"},
> +        "aria-live": "off",

I've been talking with Marco Zehe about CodeMirror  accessibility - including trying out `inputStyle: "contenteditable"` to see if it has better screenreader support. Let's revisit https://bugzilla.mozilla.org/show_bug.cgi?id=919711 to make sure we have an OK accessibility story for the console input before we flip the pref by default.

::: devtools/client/webconsole/new-console-output-wrapper.js:59
(Diff revision 1)
>  
> -        // Do not focus if a link was clicked
>          let target = event.originalTarget || event.target;
> +
> +        // If the target is a code mirror frame, let it handle the focus.
> +        if (target.URL ===

This code isn't necessary since we are using appendToLocalElement, right?
Attachment #8980648 - Flags: review?(bgrinstead) → review+
Comment on attachment 8980648 [details]
Bug 983473 - Put a CodeMirror instance in JsTerm; .

https://reviewboard.mozilla.org/r/246802/#review252954

Thanks for the review Brian. I fixed the things you pointed out and will push this to autoland.

> Why is this.editor set in the constructor, when we can't append it until componentDidMount? If we were able wait to initialize it until componentDidMount then checking `this.editor` may avoid unpredictable state and this try/catch

Having it in the constructor was done in order to have access to the instance even before appending to the DOM. But yeah, I don't think we make use of it, so let's move it

> Relatedly: in what case would `this.editor` not exist?

When there's no input in the console (jsterm without chrome debugging iirc), the editor shouldn't be created (missing in this patch :) ).

> I've been talking with Marco Zehe about CodeMirror  accessibility - including trying out `inputStyle: "contenteditable"` to see if it has better screenreader support. Let's revisit https://bugzilla.mozilla.org/show_bug.cgi?id=919711 to make sure we have an OK accessibility story for the console input before we flip the pref by default.

Yes, that was on my list too. I created Bug 1465146 to make sure of this and blocked the activation of codeMirror on it.

> This code isn't necessary since we are using appendToLocalElement, right?

Yes, good catch.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2037d91d464
Put a CodeMirror instance in JsTerm; r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/e2037d91d464
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
Depends on: 1470922
Depends on: 1484989
Blocks: 1177738
Blocks: 1527898
You need to log in before you can comment on or make changes to this bug.