The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Developer Tools: Console
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: Brijesh Patel)

Tracking

Trunk
Firefox 11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good-first-bug][mentor=msucan][fixed-in-fx-team])

Attachments

(1 attachment, 6 obsolete attachments)

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.
This is a frustrating bug when you encounter it.
Should be easy to fix.
Whiteboard: [good-first-bug][mentor=msucan]
(Assignee)

Comment 3

5 years ago
Created attachment 578639 [details] [diff] [review]
My patch for this particular bug
Attachment #578639 - Flags: feedback?(mihai.sucan)
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.
Attachment #578639 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 5

5 years ago
Created attachment 578812 [details] [diff] [review]
patch_2

Please check if any regression's there now or not.
Attachment #578639 - Attachment is obsolete: true
Attachment #578812 - Flags: feedback?(mihai.sucan)
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!
Attachment #578812 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 7

5 years ago
Created attachment 578917 [details] [diff] [review]
patch_3
Attachment #578812 - Attachment is obsolete: true
Attachment #578917 - Flags: feedback?(mihai.sucan)
Duplicate of this bug: 705843
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?
Attachment #578917 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 10

5 years ago
Created attachment 579315 [details] [diff] [review]
patch_4
Attachment #578917 - Attachment is obsolete: true
Attachment #579315 - Flags: feedback?(mihai.sucan)
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?
Attachment #579315 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 12

5 years ago
Created attachment 579594 [details] [diff] [review]
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...
Attachment #579315 - Attachment is obsolete: true
Attachment #579594 - Flags: feedback?(mihai.sucan)
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.
Attachment #579594 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 14

5 years ago
Created attachment 580425 [details] [diff] [review]
patch_6
Attachment #579594 - Attachment is obsolete: true
Attachment #580425 - Flags: feedback?(mihai.sucan)
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?
Attachment #580425 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 16

5 years ago
Created attachment 580647 [details] [diff] [review]
patch_7
Attachment #580425 - Attachment is obsolete: true
Attachment #580647 - Flags: review?(mihai.sucan)
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.
Assignee: nobody → brijesh3105
Status: NEW → ASSIGNED
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!
Attachment #580647 - Flags: review?(mihai.sucan) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9366658ba029
Whiteboard: [good-first-bug][mentor=msucan] → [good-first-bug][mentor=msucan][fixed-in-fx-team]
tracking-firefox10: --- → ?
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.
Attachment #580647 - Flags: approval-mozilla-aurora?
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.
Attachment #580647 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

5 years ago
tracking-firefox10: ? → ---
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9366658ba029
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
You need to log in before you can comment on or make changes to this bug.