1243851 - Treat enter as shift+enter if input is valid but incomplete.

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Morgan Phillips [:mrrrgn]]

We already do this in the JS shell, it's a super nice feature for developers to have. That is:

JS shell:

js> function f(x) {
js>     return x * x;
js> }

Web Console:

function f(x) {
SyntaxError: missing } after function body

Comment 1

[Brian Grinstead [:bgrins]]

We have the capability to do this right now with shift+enter (although it's not great for lots of reasons).  There are a couple of different paths here, are one of these what you are wanting?

1) We have bug 1133849 on file to add a general multiline editor mode (which is going through some design work).
2) I've also thought about being smarter about not executing script when JS fits certain patterns like `function f() {` - there isn't a separate bug for it right now but webkit nightly builds have this feature, which I also mention here: https://bugzilla.mozilla.org/show_bug.cgi?id=1133849#c5

Comment 2

[Morgan Phillips [:mrrrgn]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> We have the capability to do this right now with shift+enter (although it's
> not great for lots of reasons).  There are a couple of different paths here,
> are one of these what you are wanting?
> 
> 1) We have bug 1133849 on file to add a general multiline editor mode (which
> is going through some design work).
> 2) I've also thought about being smarter about not executing script when JS
> fits certain patterns like `function f() {` - there isn't a separate bug for
> it right now but webkit nightly builds have this feature, which I also
> mention here: https://bugzilla.mozilla.org/show_bug.cgi?id=1133849#c5

I like #2, I'm pumped about multi-line mode but as a short term fix I think this would be nice to have. In JSAPI we have JS_BufferIsCompilablUnit which is used to implement this behavior in the JS Shell. I'm going to crack at a patch for the heck of it, will maybe add you as reviewer?

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Morgan Phillips [:mrrrgn] from comment #2)
> (In reply to Brian Grinstead [:bgrins] from comment #1)
> > We have the capability to do this right now with shift+enter (although it's
> > not great for lots of reasons).  There are a couple of different paths here,
> > are one of these what you are wanting?
> > 
> > 1) We have bug 1133849 on file to add a general multiline editor mode (which
> > is going through some design work).
> > 2) I've also thought about being smarter about not executing script when JS
> > fits certain patterns like `function f() {` - there isn't a separate bug for
> > it right now but webkit nightly builds have this feature, which I also
> > mention here: https://bugzilla.mozilla.org/show_bug.cgi?id=1133849#c5
> 
> I like #2, I'm pumped about multi-line mode but as a short term fix I think
> this would be nice to have. In JSAPI we have JS_BufferIsCompilablUnit which
> is used to implement this behavior in the JS Shell. I'm going to crack at a
> patch for the heck of it, will maybe add you as reviewer?

Sure!  By the way I have a WIP patch here that attempted to do this with Parser.jsm: https://github.com/bgrins/devtools-patches/blob/master/multiline-entry.  Probably your approach is better, but there's also an integration test case in there that might be useful.

Comment 4

[Morgan Phillips [:mrrrgn]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Morgan Phillips [:mrrrgn] from comment #2)
> > (In reply to Brian Grinstead [:bgrins] from comment #1)
> > > We have the capability to do this right now with shift+enter (although it's
> > > not great for lots of reasons).  There are a couple of different paths here,
> > > are one of these what you are wanting?
> > > 
> > > 1) We have bug 1133849 on file to add a general multiline editor mode (which
> > > is going through some design work).
> > > 2) I've also thought about being smarter about not executing script when JS
> > > fits certain patterns like `function f() {` - there isn't a separate bug for
> > > it right now but webkit nightly builds have this feature, which I also
> > > mention here: https://bugzilla.mozilla.org/show_bug.cgi?id=1133849#c5
> > 
> > I like #2, I'm pumped about multi-line mode but as a short term fix I think
> > this would be nice to have. In JSAPI we have JS_BufferIsCompilablUnit which
> > is used to implement this behavior in the JS Shell. I'm going to crack at a
> > patch for the heck of it, will maybe add you as reviewer?
> 
> Sure!  By the way I have a WIP patch here that attempted to do this with
> Parser.jsm:
> https://github.com/bgrins/devtools-patches/blob/master/multiline-entry. 
> Probably your approach is better, but there's also an integration test case
> in there that might be useful.

Oh awesome, thank you!

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: shiftenter.diff

This adds a new static method to the Debugger object which mirrors the functionality of JS_BufferIsCompilableUnit. This allows us to behave like the js shell insofar as multi-line statements go.

Thank you so much for the WIP you sent! This is my first time looking at DevTools stuff so that saved me like... half a day of poking around or more. ^.^

Comment 6

[Morgan Phillips [:mrrrgn]]

Attachment: shiftenter.diff

Like a true smarty-pants I forgot to run `hg addremove`

Comment 7

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8713486 [details] [diff] [review]
shiftenter.diff

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

::: js/src/vm/Debugger.cpp
@@ +4523,5 @@
>  bool
> +Debugger::isCompilableUnit(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);

Maybe this is not necessary on the Debugger object? I have to poke around some more to understand.

Comment 8

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8713486 [details] [diff] [review]
shiftenter.diff

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

::: devtools/client/webconsole/test/browser_webconsole_multiline_input.js
@@ +28,5 @@
> +  {input: "function foo() { }" },
> +  {input: "var a = 1;" },
> +  {input: "function foo() { var a = 1; }" },
> +  {input: '"asdf"' },
> +  {input: '99 + 3' },

I goofed and did not add syntax errors here. My current copy has some cases like this, i.e. function f(x) { let y = 1, } will not go to a new line because it's an error

Comment 9

[Morgan Phillips [:mrrrgn]]

Attachment: shiftenter.diff

Addressed my own nits. Sorry about the bug mail. https://www.youtube.com/watch?v=UO7HY7Nz398

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8713492 [details] [diff] [review]
shiftenter.diff

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

This looks great!  And I think it's handling more cases than the prior attempt I did with Parser.jsm and error codes.  I'm not the right person to review the Debugger changes, I'll forward that to Nick.

Also Helen and Nick: when you have a chance can you please check out the interaction with this patch applied?  My main concern is that it might be weird that pressing Enter sometimes completes and sometimes does a new line, although playing with it briefly it has felt natural and been doing pretty much what I want.  Also want to make sure this fits into the bigger plan with multiline input and the console.

::: devtools/client/webconsole/webconsole.js
@@ +10,5 @@
>  
>  const {Utils: WebConsoleUtils, CONSOLE_WORKER_IDS} = require("devtools/shared/webconsole/utils");
>  const promise = require("promise");
>  
> +const Dbg = Cu.import("resource://gre/modules/jsdebugger.jsm");

It seems a little strange that we have to add a debugger to get access to this function call (that's usually something that only happens in the server).  I guess that's a side effect of attaching this function onto the Debugger object?  Maybe it should live somewhere else - again, curious what Nick thinks.

Comment 11

[Morgan Phillips [:mrrrgn]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8713492 [details] [diff] [review]
> shiftenter.diff
> 
> Review of attachment 8713492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great!  And I think it's handling more cases than the prior
> attempt I did with Parser.jsm and error codes.  I'm not the right person to
> review the Debugger changes, I'll forward that to Nick.
> 
> Also Helen and Nick: when you have a chance can you please check out the
> interaction with this patch applied?  My main concern is that it might be
> weird that pressing Enter sometimes completes and sometimes does a new line,
> although playing with it briefly it has felt natural and been doing pretty
> much what I want.  Also want to make sure this fits into the bigger plan
> with multiline input and the console.
> 
> ::: devtools/client/webconsole/webconsole.js
> @@ +10,5 @@
> >  
> >  const {Utils: WebConsoleUtils, CONSOLE_WORKER_IDS} = require("devtools/shared/webconsole/utils");
> >  const promise = require("promise");
> >  
> > +const Dbg = Cu.import("resource://gre/modules/jsdebugger.jsm");
> 
> It seems a little strange that we have to add a debugger to get access to
> this function call (that's usually something that only happens in the
> server).  I guess that's a side effect of attaching this function onto the
> Debugger object?  Maybe it should live somewhere else - again, curious what
> Nick thinks.

Sweet thanks for the quick look. :) Agreed about the Debugger object, I'd discussed putting it onto Parser instead with jorendorff however in the end we decided that it may not really fit there either with the Debugger being the lesser of two evils. Interested to hear Nick's thoughts.

Comment 12

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

Comment on attachment 8713492 [details] [diff] [review]
shiftenter.diff

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

The Debugger API seems like a fine place for this function to me.

Would like to take a quick glance again on the next iteration of the patch.

::: devtools/client/webconsole/webconsole.js
@@ +10,5 @@
>  
>  const {Utils: WebConsoleUtils, CONSOLE_WORKER_IDS} = require("devtools/shared/webconsole/utils");
>  const promise = require("promise");
>  
> +const Dbg = Cu.import("resource://gre/modules/jsdebugger.jsm");

Cu.import is horrible and returns the module's whole global object, letting you accidentally access private internals. The way to avoid this is relatively verbose and ugly:

  const Dbg = {};
  Cu.import("...", Dbg);
  Dbg.addDebuggerToGlobal(Dbg);
  // Use Dbg.Debugger.isCompilableUnit() below...

Luckily, we can use the devtools' common js loader here and things are a bit less wordy:

  const Debugger = require("Debugger");
  // Use Debugger.isCompilableUnit() below...

::: js/src/vm/Debugger.cpp
@@ +4520,5 @@
>      return true;
>  }
>  
>  bool
> +Debugger::isCompilableUnit(JSContext* cx, unsigned argc, Value* vp)

This should be documented in `js/src/doc/Debugger/Debugger.md`.

@@ +4523,5 @@
>  bool
> +Debugger::isCompilableUnit(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args[0].isString());

I don't think it is good practice to let any old JS code trigger assertions. Instead, we should throw an error here.

@@ +4525,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args[0].isString());
> +
> +    JSString* str = ToString<CanGC>(cx, args[0]);

If you know `args[0].isString()` then you can do `args[0].toString()`. `ToString` is more heavyweight and does all of this "is it an object with a toString method? then invoke that" stuff.

@@ +4535,5 @@
> +    if (!chars.initTwoByte(cx, str))
> +        return false;
> +
> +    // Return true on any out-of-memory error or non-EOF-related syntax error, so our
> +    // caller doesn't try to collect more buffered source.

I think OOM should be reported in the usual way:

  if (!returnsFalseOnOOM()) {
      ReportOutOfMemory(cx);
      return false;
  }

@@ +4553,5 @@
> +            result = false;
> +
> +        cx->clearPendingException();
> +    }
> +    JS_SetErrorReporter(cx->runtime(), older);

Do we have an AutoSetErrorReporter RAII class? If not, maybe you could make one and use it here. The combination of our usual style of OOM/error handling as early returns combined with needing to explicitly clean up global state mutations seems like it would be easy to make mistakes.

Comment 13

[Morgan Phillips [:mrrrgn]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #12)
> Comment on attachment 8713492 [details] [diff] [review]
> shiftenter.diff
> 
> Review of attachment 8713492 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +4553,5 @@
> > +            result = false;
> > +
> > +        cx->clearPendingException();
> > +    }
> > +    JS_SetErrorReporter(cx->runtime(), older);
> 
> Do we have an AutoSetErrorReporter RAII class? If not, maybe you could make
> one and use it here. The combination of our usual style of OOM/error
> handling as early returns combined with needing to explicitly clean up
> global state mutations seems like it would be easy to make mistakes.

I'm inclined to agree with you, for instance, when I went to report OOM I nearly forgot to reset the error reporter >.<. I searched for examples of this sort of error reporting sidestep in other parts of the codebase and it seems rare (I've only found it in the BufferedIsCompilableUnit and isCompilableUnit) but the structure of JS_SetErrorReporter seems to encourage this pattern (it returns the previous error reporter).

My feeling is that this should be filed as a separate issue. That way we can survey dangerous uses of JS_SetErrorReporter, replace them, and any fallout from it will be isolated. If you're not down with this I understand and will try adding it here.

Comment 14

[Morgan Phillips [:mrrrgn]]

Attachment: shiftenter.diff

Hopefully this is moving closer to the mark.

Comment 15

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8714462 [details] [diff] [review]
shiftenter.diff

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

::: js/src/vm/Debugger.cpp
@@ +4534,5 @@
> +        return false;
> +    }
> +
> +    JSString* str = args[0].toString();
> +    if (!str)

Unnecessary check. Removed on my end.

Comment 16

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

Comment on attachment 8714462 [details] [diff] [review]
shiftenter.diff

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

Thanks!

::: js/src/vm/Debugger.cpp
@@ +4556,5 @@
> +        // We ran into an error. If it was because we ran out of memory we report
> +        // it in the usual way.
> +        if (cx->isThrowingOutOfMemory()) {
> +            JS_SetErrorReporter(cx->runtime(), older);
> +            ReportOutOfMemory(cx);

I think in the case where isThrowingOutOfMemory() is already true, we do not need to ReportOutOfMemory, as some callee code already did that.

Comment 17

[Brian Grinstead [:bgrins]]

I think we should start with this as step towards a multiline mode, so making this block bug 1133849

Comment 18

[Morgan Phillips [:mrrrgn]]

Attachment: shiftenter.diff

Pref + additional unit tests in place. Super pumped about this.

Comment 19

[Morgan Phillips [:mrrrgn]]

s/additional//

Comment 20

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8714567 [details] [diff] [review]
shiftenter.diff

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

::: devtools/client/webconsole/webconsole.js
@@ +1,2 @@
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +

Accident. Removing in my local copy.

Comment 21

[Helen V. Holmes (:helenvholmes) (:✨)]

Locally, I haven't been able to get anything to execute any single-line functions. So, for example:

var x = 5; // hits enter
// creates new line

Same with when I create a multi-line function. Is there something I'm missing, or is something just messed up locally for me?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #21)
> Locally, I haven't been able to get anything to execute any single-line
> functions. So, for example:
> 
> var x = 5; // hits enter
> // creates new line
> 
> Same with when I create a multi-line function. Is there something I'm
> missing, or is something just messed up locally for me?

I should have clarified this earlier when asking for feedback - you'll need to run `./mach build binaries` since there are c++ changes.  Or you could run a full `./mach build` which would cover all the changes.  Let me know if you are still seeing issues after that.

Comment 23

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Also Helen and Nick: when you have a chance can you please check out the
> interaction with this patch applied?  My main concern is that it might be
> weird that pressing Enter sometimes completes and sometimes does a new line,
> although playing with it briefly it has felt natural and been doing pretty
> much what I want.  Also want to make sure this fits into the bigger plan
> with multiline input and the console.

This feels really natural to me! There are probably gotchas I'm unaware of but this is awesome. Thumbs up from me!

Comment 24

[Morgan Phillips [:mrrrgn]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > Also Helen and Nick: when you have a chance can you please check out the
> > interaction with this patch applied?  My main concern is that it might be
> > weird that pressing Enter sometimes completes and sometimes does a new line,
> > although playing with it briefly it has felt natural and been doing pretty
> > much what I want.  Also want to make sure this fits into the bigger plan
> > with multiline input and the console.
> 
> This feels really natural to me! There are probably gotchas I'm unaware of
> but this is awesome. Thumbs up from me!

Yay ! FWIW we've been using this in the JS Shell for years, it's fairly battle tested. Thanks so much for taking a look. :)

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8714567 [details] [diff] [review]
shiftenter.diff

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

r=me, thanks!

::: devtools/client/webconsole/test/browser_webconsole_multiline_input.js
@@ +48,5 @@
> +
> +  for (let test of SHOULD_ENTER_MULTILINE) {
> +    hud.jsterm.setInputValue(test.input);
> +    EventUtils.synthesizeKey("VK_RETURN", { shiftKey: test.shiftKey });
> +    let inputValue = inputNode.value;

Nit: please change this to hud.jsterm.getInputValue (as of Bug 1240196)

@@ +62,5 @@
> +    hud.jsterm.setInputValue(test.input);
> +    EventUtils.synthesizeKey("VK_RETURN", { shiftKey: test.shiftKey });
> +    is(inputNode.selectionStart, 0, "selection starts/ends at 0");
> +    is(inputNode.selectionEnd, 0, "selection starts/ends at 0");
> +    is(inputNode.value, "", "Input value is cleared");

Nit: please change this to hud.jsterm.getInputValue (as of Bug 1240196)

::: devtools/client/webconsole/webconsole.js
@@ +3850,5 @@
>          default:
>            break;
>        }
>        return;
> +    } else if ((event.shiftKey || (!Debugger.isCompilableUnit(inputNode.value) &&

I'd prefer something like this:

  else if (event.keyCode == Ci.nsIDOMKeyEvent.DOM_VK_RETURN) {
    let autoMultiline = Services.prefs.getBoolPref(PREF_AUTO_MULTILINE);
    if (event.shiftKey ||
        (autoMultiline && !Debugger.isCompilableUnit(inputValue))) {
      // shift return or incomplete statement
      return;
    }
  }

This way the pref is only read when enter is pressed instead of every key.  It's also re-using the inputValue variable.  And lastly, I removed that outdated TODO comment (which isn't true in this case since the inputNode does already get taller once the newline is entered).

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80bf314a61f6

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80bf314a61f6

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Allows entering multi-line statements in the console naturally without having to declare your intent to do so ahead of time (before this change you would have had to press shift+enter instead of enter at the end of each line).
[Suggested wording]: Smart multi-line input in the Web Console
[Links (documentation, blog post, etc)]: TBD, should be docs on MDN and in eventual Hacks post for 47.

Comment 30

[Will Bamberg [:wbamberg]]

I've added a note on this in the docs: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/The_command_line_interpreter#Entering_expressions. Does this cover it? It's a really nice feature.

Comment 31

[Morgan Phillips [:mrrrgn]]

(In reply to Will Bamberg [:wbamberg] from comment #30)
> I've added a note on this in the docs:
> https://developer.mozilla.org/en-US/docs/Tools/Web_Console/
> The_command_line_interpreter#Entering_expressions. Does this cover it? It's
> a really nice feature.

That covers it, thank you ! :)

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx 47 (Aurora) release notes