900448 - Auto-complete popup stays open over tab switches

Status: VERIFIED FIXED

(DevTools :: Console, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

* 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

Comment 1

[Athira]

Can anyone give more4 light into this bug?

Comment 2

[Mihai Sucan [:msucan]]

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().

Comment 3

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

Any activity here or can we unassign this so someone can work on it?

Comment 4

[Athira]

Iam working on it.

Comment 5

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

unassigning this. Athira, feel free to paste a patch and we'll put you back in.

Comment 6

[Giovanny Gongora [:gioyik]]

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

Comment 7

[Mihai Sucan [:msucan]]

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().

Comment 8

[Giovanny Gongora [:gioyik]]

One more question, the changes needs to be in mozilla-central or fx-team? A question to know before generate a patch.

Comment 9

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

(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

Comment 10

[DJ Edmonson]

Attachment: rev1

Comment 11

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

Comment on attachment 8337892 [details] [diff] [review]
rev1

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

looks good to me.

Comment 12

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

Comment on attachment 8337892 [details] [diff] [review]
rev1

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

anything else we need here, Mihai?

Comment 13

[Mihai Sucan [:msucan]]

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.)

Comment 14

[Mihai Sucan [:msucan]]

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().

Comment 15

[DJ Edmonson]

Yep, I can work on this and adding the test.

Comment 16

[DJ Edmonson]

Attachment: mochitest-browser test rev1

This patch adds a mochitest for this bug.

Comment 17

[DJ Edmonson]

Attachment: rev2

Comment 18

[Mihai Sucan [:msucan]]

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.

Comment 19

[Mihai Sucan [:msucan]]

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.

Comment 20

[DJ Edmonson]

Attachment: test rev2

Comment 21

[DJ Edmonson]

Attachment: rev3

Comment 22

[Mihai Sucan [:msucan]]

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.

Comment 23

[Mihai Sucan [:msucan]]

Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/c3f23ed49f48

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3f23ed49f48

Comment 25

[Petruta Horea [:phorea], Desktop QA]

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.