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

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 2

[Mihai Sucan [:msucan]]

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

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 4

[Brian Grinstead [:bgrins]]

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

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

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

Comment 6

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug 983473 - Put a CodeMirror instance in JsTerm; .

This is only about adding an editor in the JsTerm and making
sure we can still execute input strings.
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
in the output; but not in this bug since it would need to move
jsterm.execute as well.

Review commit: https://reviewboard.mozilla.org/r/246802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246802/

Comment 7

[Brian Grinstead [:bgrins]]

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?

Comment 8

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

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.

Comment 9

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Comment on attachment 8980648 [details]
Bug 983473 - Put a CodeMirror instance in JsTerm; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246802/diff/1-2/

Comment 10

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Comment on attachment 8980648 [details]
Bug 983473 - Put a CodeMirror instance in JsTerm; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246802/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2037d91d464
Put a CodeMirror instance in JsTerm; r=bgrins.

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e2037d91d464