704295 - Autocomplete with a variable name that is equal to a prefix of a global variable makes it impossible to use the variable

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
1. In the web console, type the following:
var d = 5;
2. Hit enter
3. Type the following (note how there is no semicolon):
var a = d
4. Hit enter

Expected results:
The line |var a = d| is executed.

Actual results:
The line |var a = defaultStatus| is executed.

Comment 1

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

This is a frustrating bug when you encounter it.

Comment 2

[Mihai Sucan [:msucan]]

Should be easy to fix.

Comment 3

[Brijesh Patel]

Attachment: My patch for this particular bug

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 578639 [details] [diff] [review]
My patch for this particular bug

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

Thank you very much for your contribution!

This works fine but there's a regression.

STR:
1. open the web console.
2. type dispa<Tab> to autocomplete to dispatchEvent.
3. backspace until you have only "d".
4. type oc<Tab> to autocomplete to document.

Expected result: "document" shows up.

Actual result: "docpatchEvent" is shown.

Please look into this. The problem is related to acceptProposedCompletion() and to complete(). I expect some properties are not cleared when needed.

Looking forward for the updated patch! Please let me know if you need further help.

Comment 5

[Brijesh Patel]

Attachment: patch_2

Please check if any regression's there now or not.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 578812 [details] [diff] [review]
patch_2

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

Brijesh, thanks for your update!

This is indeed an improvement, but I still see a regression.

STR:

1. open the web console.
2. type "dispatch" and you will see dispatchEvent autocompleted.
3. backspace until you have only "d".

Expected result: the input only shows "d".

Actual result: the input shows "d   atchEvent".

Please also fix this case. Thank you!

Comment 7

[Brijesh Patel]

Attachment: patch_3

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 578917 [details] [diff] [review]
patch_3

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

Thanks for your patch. I am still having problems with this patch.

STR:

1. open the Web Console and type "dis".
2. press the Down arrow key until you get to "dispatchEvent", so you have "patchEvent" in gray in the completion node.
3. press backspace once.

Expected result: completion node is cleared.

Actual result: I see "di patchEvent".

I took some time to investigate the bug. It seems to also be a problem in AutocompletePopup.jsm - there's a bug in clearItems(). You need to add at the start of the method this.selectedIndex = -1 such that the currently selectedIndex/Item is cleared. This confuses the Web Console code... From there, with this fix, we can move on to fixing the remaining issues in the Web Console.


Thanks a lot for your contribution! Looking forward for the updated patch!

::: browser/devtools/webconsole/HUDService.jsm
@@ +5438,4 @@
>                               matchProp: properties.matchProp};
>  
>        if (items.length > 1 && !popup.isOpen) {
> +        this.updateCompleteNode("");

This doesn't seem needed. The completion node should be automatically cleared in other places. If we need to clear it here, it's a bug. (see the calls to clearCompletion()).

@@ +5447,1 @@
>          popup.hidePopup();

Can you please explain why you do this?

Comment 10

[Brijesh Patel]

Attachment: patch_4

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 579315 [details] [diff] [review]
patch_4

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

Patch works as expected. Thank you very much!

Next step: writing a test. Please look into browser/devtools/webconsole/test/browser to see a good number of tests. Copy one of them and make it test the STR in comment 0.

Please ping me on IRC if you need help with writing the test!

Awesome first-contribution! Thanks!

::: browser/devtools/webconsole/AutocompletePopup.jsm
@@ +229,3 @@
>      // Reset the panel and list dimensions. New dimensions are calculated when a
>      // new set of items is added to the autocomplete popup.
> +    this.selectedIndex = -1;

Please move this before the while loop - before the list is cleared.

::: browser/devtools/webconsole/HUDService.jsm
@@ +5452,3 @@
>          if (items.length == 1) {
>            // onSelect is not fired when the popup is not open.
>            this.onAutocompleteSelect();

I investigated the code a bit more now.

Your patch does exactly what we need, but I would suggest:

- make this.onAutocompleteSelect() to be always called - so move it out of the ifs.

- do if (items.length == 1) { popup.selectedIndex = 0; }. This is the same as what you do, with popup.selectNextItem() - hopefully the proposed change is clearer when we come back to read the code.

- you can remove updateCompleteNode("") because the call to onAutocompleteSelect() will do that, when needed.

What do you think? Do these changes make sense to you?

Comment 12

[Brijesh Patel]

Attachment: patch_5

yes, this changes are sensible to me though the work done by this patch and previous one is same, but this patch looks better as far as code readability is concerned...

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 579594 [details] [diff] [review]
patch_5

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

Thanks for your update, Brijesh!

Looking forward to see the test!

::: browser/devtools/webconsole/AutocompletePopup.jsm
@@ +222,5 @@
>    clearItems: function AP_clearItems()
>    {
> +    // Reset the selectedIndex to -1 before clearing the list
> +    this.selectedIndex = -1;
> +        

Nit: this line has trailing whitespace. Please remove it.

::: browser/devtools/webconsole/HUDService.jsm
@@ +5448,1 @@
>          if (items.length == 1) {

You no longer need the outer if items.length > 0.

@@ +5448,2 @@
>          if (items.length == 1) {
> +          popup.selectedIndex = 0;          

Nit: this line has trailing whitespace. Please remove it.

Comment 14

[Brijesh Patel]

Attachment: patch_6

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 580425 [details] [diff] [review]
patch_6

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

This is great Brijesh! Your first test passes without any errors and it works nicely! I only have a comment on how you can simulate the steps-to-reproduce even more closely.

Still, there's one problem with this patch. I get test failures when I run all the Web Console tests. See this copy/paste:
http://pastebin.mozilla.org/1398729

Out of about a thousand of tests, 6 of them fail. Please look into the tests that fail and fix them. This happens usually when existing tests expect behavior that has changed. Please let me know if you need help with anything.

Thank you very much for your work and patience. We are very close to be ready to land this fix!

::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_704295.js
@@ +64,5 @@
> +  
> +  // Test typing 'var a = d' and press RETURN
> +  input.value = "var a = d";
> +  input.setSelectionRange(9, 9);
> +  jsterm.complete(jsterm.COMPLETE_HINT_ONLY);

I suggest you do jsterm.setInputValue("vad a = ");

Then instead of setSelectionRange() and instead of calling jsterm.complete() manually, generate synthetic keyboard events:

EventUtils.synthesizeKey("d", {});

Then:
EventUtils.synthesizeKey("VK_ENTER", {})

Then you check what is the jsterm.completeNode.value and the input.value. This should be all.

What do you think?

Comment 16

[Brijesh Patel]

Attachment: patch_7

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

I have assigned this to Brijesh as it appears that Brijesh is working on this.

I'm trying to keep the status of our mentored bugs up to date. Please unassign if this was done in error.

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 580647 [details] [diff] [review]
patch_7

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

This is great Brijesh! Your patch works as expected and all the tests pass.

This is ready to land. Thank you!

Comment 19

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

https://hg.mozilla.org/integration/fx-team/rev/9366658ba029

Comment 20

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

Comment on attachment 580647 [details] [diff] [review]
patch_7

this is a really annoying bug when you hit it. I'd love to get this into aurora.

Comment 21

[Johnathan Nightingale [:johnath]]

Comment on attachment 580647 [details] [diff] [review]
patch_7

Awesome to see this fixed, but we discussed this in channel triage and don't think it needs to be rushed into aurora. The problem is not new to FF10, and has straightforward workarounds. I'm really happy to have a fix in the tree and working its way through, though.

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/9366658ba029