Add autocompletion popup support in CodeMirror-JsTerm

RESOLVED FIXED in Firefox 63

Status

enhancement
P1
normal
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

(Blocks 1 bug)

Trunk
Firefox 63
Dependency tree / graph

Firefox Tracking Flags

(firefox62 wontfix, firefox63 fixed)

Details

(Whiteboard: [boogaloo-mvp])

Attachments

(7 attachments)

Bug 983473 will land without the autocompletion popup, so let's deal with it here.
CodeMirror already has ways to deal with autocompletion that would simplify the JsTerm code.
Assignee

Updated

Last year
No longer depends on: 1463672
Whiteboard: [boogaloo-mvp]
Assignee

Updated

Last year
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1

Updated

Last year
Product: Firefox → DevTools
Assignee

Updated

Last year
Depends on: 1473305
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Some tests won't pass yet because we are waiting on other bugs to land.
Depends on: 1472161, 1472606
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Could you rebase this when you have a chance? Specifically, part 4 isn't applying cleanly.
(In reply to Brian Grinstead [:bgrins] from comment #16)
> Could you rebase this when you have a chance? Specifically, part 4 isn't
> applying cleanly.

And even more so when applied on top of Bug 1472161.

Comment 18

Last year
mozreview-review
Comment on attachment 8990016 [details]
Bug 1463674 - Add shadow autocompletion text capability to the source editor; .

https://reviewboard.mozilla.org/r/255040/#review262730

When I enter:

```
function foo() {
  window
}
```

Then move to after `window` and press `.` I don't see the text appearing. Can you see that as well?

::: commit-message-2050f:8
(Diff revision 2)
> +This functionnality is used in the webconsole to display to the user
> +what will be inserted if they hit Tab.
> +Since CodeMirror does not provide such feature, we take advantage of
> +markText to put the autocompletion text in a title attribute and
> +then display it using a CSS after pseudo element.
> +This way, we don't have to run any complex computation for positionning

Nit: 'positioning'

::: devtools/client/themes/webconsole.css:385
(Diff revision 2)
>  /* Unset the bottom right radius on the jsterm inputs when the sidebar is visible */
>  :root[platform="mac"]  .jsterm-cm .sidebar ~ .jsterm-input-container > .CodeMirror {
>    border-bottom-right-radius: 0;
>  }
>  
> +.jsterm-cm [data-complete]::after {

Where does the [data-complete] node come from? I don't see it in this patch, at least
Attachment #8990016 - Flags: review?(bgrinstead)

Comment 19

Last year
mozreview-review
Comment on attachment 8990017 [details]
Bug 1463674 - Refactor char and chevron width measurement; .

https://reviewboard.mozilla.org/r/255042/#review262732

::: devtools/client/webconsole/components/JSTerm.js:288
(Diff revision 2)
> +      ? this.inputNode.getBoundingClientRect().height - this.inputNode.clientHeight
> +      : 0;
> +
> +    // Update the character and chevron width needed for the popup offset calculations.
> +    this._inputCharWidth = this._getInputCharWidth();
> +    this._chevronWidth = this.editor ? null : this._getChevronWidth();

We have a condition here so that we never call `_getChevronWidth` if this.editor exists, but `_getChevronWidth` appears to have handling for the codeMirror case (via `this.node`). Is there a meaningful distinction between when `this.editor` is set vs when `this.node` is set?

::: devtools/client/webconsole/components/JSTerm.js:1322
(Diff revision 2)
> -    if (this.props.codeMirrorEnabled || !this.inputNode) {
> -      return;
> +    if (!this.inputNode && !this.node) {
> +      return null;
> +    }
> +
> +    if (this.editor) {
> +      return this.editor.codeMirror.defaultCharWidth();

We would ideally not use the codeMirror object directly from the source editor instance (or even expose it), in order make it less hard to switch the editor used behind the scenes if needed.

We could instead add `defaultCharWidth` to `CM_MAPPING` so that it will be exposed on the editor object direction rather than digging into `codeMirror`: https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/devtools/client/sourceeditor/editor.js#1427-1435.
Attachment #8990017 - Flags: review?(bgrinstead)
Assignee

Updated

Last year
Blocks: 1136299
> Where does the [data-complete] node come from? I don't see it in this patch, at least

Yeah, this is a leftover from a previous iteration.
> We have a condition here so that we never call `_getChevronWidth` if this.editor exists, but `_getChevronWidth` appears to have handling for the codeMirror case (via `this.node`). Is there a meaningful distinction between when `this.editor` is set vs when `this.node` is set?

this.editor is created in componentDidMount, and this.node is available also after mounting so there shouldn't be differences. That being said, the reason we were testing this.node is because in a previous iteration we were using it to compute some variable if this.inputNode didn't exist. Since we don't call it if codeMirror is enabled, I removed the reference in getChevronWidth.

> We would ideally not use the codeMirror object directly from the source editor instance (or even expose it), in order make it less hard to switch the editor used behind the scenes if needed.

Good point, I'll look into this also for the following patches, where I think we do use the code mirror instance.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

Last year
mozreview-review
Comment on attachment 8990017 [details]
Bug 1463674 - Refactor char and chevron width measurement; .

https://reviewboard.mozilla.org/r/255042/#review262918
Attachment #8990017 - Flags: review?(bgrinstead) → review+

Comment 30

Last year
mozreview-review
Comment on attachment 8990015 [details]
Bug 1463674 - Allow sourceeditor consumer to have <kbd>Tab</kbd> handling; .

https://reviewboard.mozilla.org/r/255038/#review262930

::: devtools/client/sourceeditor/editor.js:184
(Diff revision 3)
>    // indenting with tabs, insert one tab. Otherwise insert N
>    // whitespaces where N == indentUnit option.
>    this.config.extraKeys.Tab = cm => {
> +    if (config.extraKeys && config.extraKeys.Tab) {
> +      const res = config.extraKeys.Tab(cm);
> +      if (res !== "CodeMirror.Pass") {

I'm a bit confused by this. If the extraKeys returns CodeMirror.Pass then shouldn't we early return here with it as well instead of continuing on? CodeMirror.Pass is meant to be passed on to CM as indication that we aren't handling the key - and AFAICT I don't think we actually want to tell CM that in any case.

I would have expected either (1) something like this where we just override the Tab handler when the caller defines it:

```
this.config.extraKeys.Tab = (config.extraKeys && config.extraKeys.Tab) || cm => {
// original function
};
```

Or (2) the inverse of what we are doing (where we return if and only if CodeMirror.Pass is returned):

```
const res = config.extraKeys.Tab(cm);
if (res === "CodeMirror.Pass") {
  return res;
}
```

Or (3) don't ever invoke CodeMirror.Pass and instead allow the extraKeys function to return false (or something else) as a way to prevent the normal 'extra' tab behavior from running:

```
const res = config.extraKeys.Tab(cm);
if (res === false) {
  return;
}
```

Looking at the code in the later patch, I *think* what we want is (3), so that we could rely on this better Tab behavior (which is space-aware, for instance) as opposed to having our console extraKey handler do everything:

```
  if (this.complete(this.COMPLETE_HINT_ONLY) &&
    this.lastCompletion &&
    this.acceptProposedCompletion()
  ) {
    return false;
  }

  if (this.hasEmptyInput()) {
    return false;
  }

  // Let the normal editor tab handling run by not returning false
```
Attachment #8990015 - Flags: review?(bgrinstead)

Comment 31

Last year
mozreview-review
Comment on attachment 8990016 [details]
Bug 1463674 - Add shadow autocompletion text capability to the source editor; .

https://reviewboard.mozilla.org/r/255040/#review262940

The code change looks good, although I'd like to confirm if you are seeing the issue I mentioned in Comment 18 with the shadow text not appearing when happening in a multiline statement.
Attachment #8990016 - Flags: review?(bgrinstead) → review+
Assignee

Updated

Last year
Blocks: 1474817
(In reply to Brian Grinstead [:bgrins] from comment #31)
> Comment on attachment 8990016 [details]
> Bug 1463674 - Add shadow autocompletion text capability to the source
> editor; .
> 
> https://reviewboard.mozilla.org/r/255040/#review262940
> 
> The code change looks good, although I'd like to confirm if you are seeing
> the issue I mentioned in Comment 18 with the shadow text not appearing when
> happening in a multiline statement.

I thought I replied to this, sorry about that. 
So yes, I do see this issue, and I also see it in the old jsterm. I filed Bug 1474817 to fix this.
Assignee

Updated

Last year
Depends on: 1473923

Comment 33

Last year
mozreview-review
Comment on attachment 8990019 [details]
Bug 1463674 - Add new test helpers for JsTerm; .

https://reviewboard.mozilla.org/r/255046/#review263604

::: devtools/client/webconsole/test/mochitest/head.js:344
(Diff revision 3)
>  }
>  
>  /**
>   * Returns true if the give node is currently focused.
>   */
>  function hasFocus(node) {

Here's an existing hasFocus function that seems to get called with the inputNode in various places: https://searchfox.org/mozilla-central/search?q=hasFocus&path=webconsole. Should those callers be updated to use isJstermFocused?

::: devtools/client/webconsole/test/mochitest/head.js:380
(Diff revision 3)
> +    }
> +
> +    if (jsterm.editor) {
> +      const command = caretIndexOffset < 0 ? "goColumnLeft" : "goColumnRight";
> +      for (let i = 0; i < Math.abs(caretIndexOffset); i++) {
> +        jsterm.editor.codeMirror.execCommand(command);

Would it be possible to accomplish this with the existing Editor API methods so we don't need to touch codeMirror directly (like with getPosition and setCursor)?

Untested locally but I think that would work - if caretIndexOffset is non-negative then you would call `setCursor(getPosition(caretIndexOffset))` otherwise you would call `setCursor(getPosition(value+caretIndexOffset))`. Or something like that.

::: devtools/client/webconsole/test/mochitest/head.js:493
(Diff revision 3)
> +  if (jsterm.completeNode) {
> +    return jsterm.completeNode.value;
> +  }
> +
> +  if (jsterm.editor) {
> +    const autoCompletionMark = jsterm.editor.codeMirror.getAllMarks()

I'd prefer to avoid using codeMirror directly here. Does Editor.getMarker or some other existing function work for our case? If not, could we provide a new one that does?

::: devtools/client/webconsole/test/mochitest/head.js:511
(Diff revision 3)
> + * enabled or not.
> + *
> + * @param {JsTerm} jsterm
> + * @returns {Boolean}
> + */
> +function isJstermFocused(jsterm) {

I almost wonder if this should just be implemented on JSTerm component - I was surprised we don't ever use it in the frontend. May as well keep it here until we need it though.

::: devtools/client/webconsole/test/mochitest/head.js:520
(Diff revision 3)
> +  if (jsterm.inputNode) {
> +    return document.activeElement == jsterm.inputNode && documentIsFocused;
> +  }
> +
> +  if (jsterm.editor) {
> +    return documentIsFocused && jsterm.editor.codeMirror.hasFocus();

We already expose hasFocus on the Editor object - no need to dig into CodeMirror
Attachment #8990019 - Flags: review?(bgrinstead)
> Here's an existing hasFocus function that seems to get called with the inputNode in various places: https://searchfox.org/mozilla-central/search?q=hasFocus&path=webconsole. Should those callers be updated to use isJstermFocused?

Yes, this is done in the next patch.

> Would it be possible to accomplish this with the existing Editor API methods so we don't need to touch codeMirror directly (like with getPosition and setCursor)?

let me try that

> I'd prefer to avoid using codeMirror directly here. Does Editor.getMarker or some other existing function work for our case? If not, could we provide a new one that does?

I did not find anything existing so I created a dedicated function to get its value on the editor.

> I almost wonder if this should just be implemented on JSTerm component - I was surprised we don't ever use it in the frontend. May as well keep it here until we need it though.

Yeah, i'd keep it here for now. If at some point we need it in the code itself, we'd remove the helper function.

> We already expose hasFocus on the Editor object - no need to dig into CodeMirror

okay
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks for the review so far Brian !
I made the changes you suggested (and some others in some cases), and I asked honza for review on one big patch since I figured out this is a pretty big list of changes.

Comment 43

Last year
mozreview-review
Comment on attachment 8990015 [details]
Bug 1463674 - Allow sourceeditor consumer to have <kbd>Tab</kbd> handling; .

https://reviewboard.mozilla.org/r/255038/#review263680

::: devtools/client/sourceeditor/editor.js:182
(Diff revision 4)
>    // Overwrite default tab behavior. If something is selected,
>    // indent those lines. If nothing is selected and we're
>    // indenting with tabs, insert one tab. Otherwise insert N
>    // whitespaces where N == indentUnit option.
>    this.config.extraKeys.Tab = cm => {
> +    if (config.extraKeys && config.extraKeys.Tab) {

Please leave a comment here explaining this case
Attachment #8990015 - Flags: review?(bgrinstead) → review+

Comment 44

Last year
mozreview-review
Comment on attachment 8990019 [details]
Bug 1463674 - Add new test helpers for JsTerm; .

https://reviewboard.mozilla.org/r/255046/#review263682
Attachment #8990019 - Flags: review?(bgrinstead) → review+

Comment 45

Last year
mozreview-review
Comment on attachment 8990021 [details]
Bug 1463674 - Run all jsterm test with both the old and the new version; .

https://reviewboard.mozilla.org/r/255050/#review263684

Nice work! I expect we'll start to see some intermittent timeouts pop up on these tests since we're now doing twice the work. Please do a try run with a bunch of re-runs on the webconsole tests to see if there's any requestLongerTimeout that we should go ahead and add now.
Attachment #8990021 - Flags: review?(bgrinstead) → review+
> Nice work! I expect we'll start to see some intermittent timeouts pop up on these tests since we're now doing twice the work. Please do a try run with a bunch of re-runs on the webconsole tests to see if there's any requestLongerTimeout that we should go ahead and add now.

Yes, will do

Comment 47

Last year
mozreview-review
Comment on attachment 8990018 [details]
Bug 1463674 - Enable autocompletion popup in codeMirror JsTerm; .

https://reviewboard.mozilla.org/r/255044/#review263722

::: devtools/client/webconsole/components/JSTerm.js:221
(Diff revision 4)
> +              if (this.hasEmptyInput()) {
> +                this.editor.codeMirror.getInputField().blur();
> +                return false;
> +              }
> +
> +              if (!this.hasEmptyInput() && !this.editor.somethingSelected()) {

We don't need to check `!this.hasEmptyInput()` since we already early returned in this case.

Also, do we actually need to handle the `this.editor.somethingSelected` case - will Editor not do what we want?
Attachment #8990018 - Flags: review?(bgrinstead) → review+
> We don't need to check `!this.hasEmptyInput()` since we already early returned in this case.

right

> Also, do we actually need to handle the `this.editor.somethingSelected` case - will Editor not do what we want?

In this block, we return false, meaning the editor extraKeys.Tab won't do anything (which is what we want).

No, if we weren't going to test if something is selected, we would only add a tab at the beginning of the line, which is the current behaviour.
But for a multiline input, it's better if the whole selected block is indented, which is what is handled by editors' extraKeys.Tab.
Comment on attachment 8990020 [details]
Bug 1463674 - Add a missing test for hitting arrow keys when autocomplete popup is displayed; .

https://reviewboard.mozilla.org/r/255048/#review264058

Looks good to me, just couple of minor comments.

R+ assuming try is green.

Honza

::: devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_arrow_keys.js:39
(Diff revision 4)
> +  jsterm.setInputValue("window.foo");
> +  EventUtils.sendString(".");
> +  await onPopUpOpen;
> +
> +  info("Trigger autocomplete popup opening");
> +  checkInput("window.foo.|");

The helper for testing cursor position would deserver an explanatory comment.

::: devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_arrow_keys.js:43
(Diff revision 4)
> +  info("Trigger autocomplete popup opening");
> +  checkInput("window.foo.|");
> +  is(popup.isOpen, true, "popup is open");
> +  checkJsTermCompletionValue(jsterm, "           aa", "completeNode has expected value");
> +
> +  info("Test that arrow left closes the popup and clear complete node");

nit: `and clear` => `and clears`
Attachment #8990020 - Flags: review?(odvarko) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 57

Last year
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/abb8a2696754
Allow sourceeditor consumer to have <kbd>Tab</kbd> handling; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/7e2fa737f80e
Add shadow autocompletion text capability to the source editor; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/bc02b75bf576
Refactor char and chevron width measurement; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/b31613e5658a
Enable autocompletion popup in codeMirror JsTerm; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/d9c8064b6c29
Add new test helpers for JsTerm; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/44bf63c34c0a
Add a missing test for hitting arrow keys when autocomplete popup is displayed; r=Honza.
https://hg.mozilla.org/integration/autoland/rev/28d0ba8fa12c
Run all jsterm test with both the old and the new version; r=bgrins.
Depends on: 1479521
Assignee

Updated

11 months ago
Blocks: 1480709
You need to log in before you can comment on or make changes to this bug.