Closed Bug 900448 Opened 6 years ago Closed 6 years ago

Auto-complete popup stays open over tab switches

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 28

People

(Reporter: gfritzsche, Assigned: dj)

Details

(Whiteboard: [good first bug][mentor=msucan][lang=js])

Attachments

(2 files, 3 obsolete files)

* open the "web console" on some tab
* trigger an auto-completion popup
* CMD-T for new tab

Results:
* auto-completion popup stays around until i click somewhere

Expected:
* auto-completion popup should have closed on tab change
Priority: -- → P2
Whiteboard: [good first bug][mentor=msucan@mozilla.com][lang=js]
Priority: P2 → P3
Assignee: nobody → athiralek
Can anyone give more4 light into this bug?
Flags: needinfo?(mihai.sucan)
Thanks for your interest Athira. You need to listen to the jsterm input "blur" event and close the autocomplete popup.

See browser/devtools/webconsole/webconsole.js. Search for the JSTerm object. You have inputNode there and autocompletePopup. In JST_init() add the event listener I mentioned, then remove it in JST_destroy().
Flags: needinfo?(mihai.sucan)
Any activity here or can we unassign this so someone can work on it?
Iam working on it.
unassigning this. Athira, feel free to paste a patch and we'll put you back in.
Assignee: athiralek → nobody
Whiteboard: [good first bug][mentor=msucan@mozilla.com][lang=js] → [good first bug][mentor=msucan][lang=js]
Hi,

I am seeing this bug and is not clear to me at all (sorry if it is too obious). The  Mihai Sucan comment means that in browser/devtools/webconsole/webconsole.js we need to add in JST_init 'this.inputNode.addEventListener("blur", this._inputEventHandler, false);' and in JST_destroy 'this.inputNode.removeEventListener("blur", this._inputEventHandler, false);'?

Regards,
Gio
Hello Giovanny!

In JST_init() add:

this.inputNode.addEventListener("blur", this._onBlurInput, false);

In JST_destroy() add:

this.inputNode.removeEventListener("blur", this._onBlurInput, false);

... then add the _onBlurInput method to the JSTerm object. This function needs to invoke this.clearCompletion().
One more question, the changes needs to be in mozilla-central or fx-team? A question to know before generate a patch.
(In reply to Giovanny Andres Gongora Granada from comment #8)
> One more question, the changes needs to be in mozilla-central or fx-team? A
> question to know before generate a patch.

fx-team please
Attached patch rev1 (obsolete) — Splinter Review
Attachment #8337892 - Flags: review?(rcampbell)
Comment on attachment 8337892 [details] [diff] [review]
rev1

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

looks good to me.
Attachment #8337892 - Flags: review?(rcampbell) → review+
Comment on attachment 8337892 [details] [diff] [review]
rev1

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

anything else we need here, Mihai?
Attachment #8337892 - Flags: review?(mihai.sucan)
Comment on attachment 8337892 [details] [diff] [review]
rev1

Thank you DJ! This patch is looking fine.

DJ, do you want to write a test for this bug? See:

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

In the browser/devtools/webconsole/test folder you will find the console tests. Copy one of the tests and changing such that it tests that this bug is fixed. I suggest you look at the existing autocomplete popup tests.

Add the name of the new file to browser.ini.

The new test should add a new tab, open the web console, type something like |document.| and when the popup is open, switch to the previous tab, wait for the popup to close, then call finishTest() which will close the tab you added, and it will also close the console.

If you have any questions, please let us know. Thanks!

(r+, we can land the patch as is. the test would be nice to have.)
Attachment #8337892 - Flags: review?(mihai.sucan) → review+
Been working on some other patch, while I had this one applied. It seems click on suggestions in the popup no longer works. I checked if the blur event could tell us the blur reason, but I found nothing useful. It looks like this.clearCompletion() should be called in a short setTimeout(). DJ, can you do that? Update the _blurEventHandler() function to do this? You can use this.hud.window.setTimeout().
Assignee: nobody → dj
Status: NEW → ASSIGNED
Yep, I can work on this and adding the test.
Attached patch mochitest-browser test rev1 (obsolete) — Splinter Review
This patch adds a mochitest for this bug.
Attached patch rev2 (obsolete) — Splinter Review
Attachment #8337892 - Attachment is obsolete: true
Attachment #8341929 - Flags: review?(mihai.sucan)
Comment on attachment 8341929 [details] [diff] [review]
rev2

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

Thanks for the updated patch. This working fine and you're very close to being done. More comments below.

::: browser/devtools/webconsole/webconsole.js
@@ +3031,5 @@
>     * Initialize the JSTerminal UI.
>     */
>    init: function JST_init()
>    {
> +    let chromeDocument = this.hud.document;

This change is fine, but given chromeDocument is only used once in JST_init() please remove this variable. Update the call to the AutocompletePopup() constructor.

@@ +3740,5 @@
>      }
>    },
>  
>    /**
> +   * The inputNode "blur" event handler.

This is now the window blur event handler.

@@ +3745,5 @@
> +   * @private
> +   */
> +  _blurEventHandler: function JST__blurEventHandler()
> +  {
> +    this.clearCompletion();

before calling clearCompletion() please do a check, if (this.autocompletePopup) { this.clearCompletion() }. The blur event handler can fire during the webconsole destroy and you'd get an exception.

@@ +4379,5 @@
>      this.inputNode.removeEventListener("keypress", this._keyPress, false);
>      this.inputNode.removeEventListener("input", this._inputEventHandler, false);
>      this.inputNode.removeEventListener("keyup", this._inputEventHandler, false);
>      this.inputNode.removeEventListener("focus", this._focusEventHandler, false);
> +    this.inputNode.removeEventListener("blur", this._blurEventHandler, false);

This needs to be updated now.
Attachment #8341929 - Flags: review?(mihai.sucan) → feedback+
Comment on attachment 8341926 [details] [diff] [review]
mochitest-browser test rev1

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

Thanks for the patch! Test looks good. Just one concern about the timeout. See below.

::: browser/devtools/webconsole/test/browser.ini
@@ +242,5 @@
>  [browser_webconsole_reflow.js]
>  [browser_webconsole_log_file_filter.js]
>  [browser_webconsole_expandable_timestamps.js]
>  [browser_webconsole_autocomplete_in_debugger_stackframe.js]
> +[browser_webconsole_bug_900448_autocomplete_popup_close.js]

Please rename to: browser_webconsole_autocomplete_popup_close_on_tab_switch.js.

::: browser/devtools/webconsole/test/browser_webconsole_bug_900448_autocomplete_popup_close.js
@@ +25,5 @@
> +  win = HUD.target.tab.linkedBrowser.contentWindow;
> +  popup = HUD.jsterm.autocompletePopup;
> +
> +  popup._panel.addEventListener("popupshown", function() {
> +    popup._panel.removeEventListener("popupshown", arguments.callee, false);

arguments.callee is deprecated.

@@ +45,5 @@
> +}
> +
> +function tab2Loaded (aEvent) {
> +  tab2.linkedBrowser.removeEventListener(aEvent.type, tab2Loaded, true);
> +  tID = win.setTimeout(completeTest, 2000, false, "Popup did not close within 2 seconds.");

Timeouts are generally not allowed in tests. Let the test timeout if the popup fails to close.
Attachment #8341926 - Flags: feedback+
Attached patch test rev2Splinter Review
Attachment #8341926 - Attachment is obsolete: true
Attachment #8343072 - Flags: review?(mihai.sucan)
Attached patch rev3Splinter Review
Attachment #8341929 - Attachment is obsolete: true
Attachment #8343073 - Flags: review?(mihai.sucan)
Attachment #8343072 - Flags: review?(mihai.sucan) → review+
Comment on attachment 8343073 [details] [diff] [review]
rev3

Both patches look good. Good work! Thank you!

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e29c8d6d4ac1

Once the try push is green I will land the patches.
Attachment #8343073 - Flags: review?(mihai.sucan) → review+
Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/c3f23ed49f48
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug][mentor=msucan][lang=js] → [good first bug][mentor=msucan][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c3f23ed49f48
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=msucan][lang=js][fixed-in-fx-team] → [good first bug][mentor=msucan][lang=js]
Target Milestone: --- → Firefox 28
Keywords: verifyme
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0

Verified as fixed on latest Aurora 28.0a2 (buildID: 20140131004003): the pop-up disappears when a new tab is opened.

However, the same issue reproduces on Inspector tab, I filed bug 966306 for that.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.