597200 - Web Console close button always closes the HUD from the focused window

Status: RESOLVED DUPLICATE of bug 577721

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

STR:

1. open two windows and a tab in each.
2. open the web console in each of the two tabs.
3. place each window so you see the close buttons of the two Web Consoles.
4. Focus window A and shift+click the close button from window B.

Expected result: Web Console from window B closes.
Actual result: Web Console from window A closes.

Also, shift+click the close button again from window B (while window A is still focused). The Web Console from window A opens back.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test code

Proposed fix and test code.

Comment 2

[Patrick Walton (:pcwalton)]

Comment on attachment 478548 [details] [diff] [review]
proposed fix and test code

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -4549,21 +4549,32 @@ JSTermFirefoxMixin.prototype = {
> 
>     let inputNode = this.xulElementFactory("textbox");
>     inputNode.setAttribute("class", "jsterm-input-node");
>     inputNode.setAttribute("flex", "1");
>     inputNode.setAttribute("multiline", "true");
>     inputNode.setAttribute("rows", "1");
>     inputContainer.appendChild(inputNode);
> 
>+    let self = this;
>     let closeButton = this.xulElementFactory("button");
>     closeButton.setAttribute("class", "jsterm-close-button");
>     inputContainer.appendChild(closeButton);
>-    closeButton.addEventListener("command", HeadsUpDisplayUICommands.toggleHUD,
>-                                 false);
>+    closeButton.addEventListener("command", function() {

How about factoring this out and reusing the HeadsUpDisplay.toggleHUD() method, adding an optional HUD ID parameter? The createHUDNodes() function is too large already.

>+      let displayNode = self.parentNode.parentNode.parentNode;

Seems a bit fragile. How about using a loop looking for well-known class names like the one in bug 587615?

Looks good otherwise. f=me with changes.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch. Thanks for your feedback+!

Asking again for feedback because you may have further comments on the code changes. This patch now depends on bug 597103.

Comment 4

[Patrick Walton (:pcwalton)]

Comment on attachment 479746 [details] [diff] [review]
updated patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -4555,18 +4555,18 @@ JSTermFirefoxMixin.prototype = {
>+    closeButton.addEventListener("command", this.onCloseButtonClick.bind(this),
>+      false);

Nit: Indent "false" under "command".

>+  onCloseButtonClick: function JSTF_onCloseButtonClick()
>+  {
>+    let contentWindow = this.context.get();
>+    let gBrowser = this.parentNode.ownerDocument.defaultView.gBrowser;
>+    let tab = gBrowser._getTabForContentWindow(contentWindow);

Hmm. In most JavaScript styles, underscore-prefixed methods are private and not supposed to be called from the outside. I don't know whether (a) this applies to gBrowser and (b) there's another way to do what you want. I'd ask somebody (gavin?) if there's a preferred way to grab the tab that corresponds to a content window.

>+    HUDService.deactivateHUDForContext(tab);

On a side note, it's odd that "context" refers to the content window within the JSTermFirefoxMixin but refers to the tab in the HUDService. Another cleanup bug to file, I think.

Looks good in general though. f+.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to comment #4)
> Comment on attachment 479746 [details] [diff] [review]
> updated patch
> 
> >+  onCloseButtonClick: function JSTF_onCloseButtonClick()
> >+  {
> >+    let contentWindow = this.context.get();
> >+    let gBrowser = this.parentNode.ownerDocument.defaultView.gBrowser;
> >+    let tab = gBrowser._getTabForContentWindow(contentWindow);
> 
> Hmm. In most JavaScript styles, underscore-prefixed methods are private and not
> supposed to be called from the outside. I don't know whether (a) this applies
> to gBrowser and (b) there's another way to do what you want. I'd ask somebody
> (gavin?) if there's a preferred way to grab the tab that corresponds to a
> content window.

Yeah, I know... I was given the choice to either copy the source code of that method or ... just call it. If you know (or someone else?) another API I can use for this purpose, please let me know.

> >+    HUDService.deactivateHUDForContext(tab);
> 
> On a side note, it's odd that "context" refers to the content window within the
> JSTermFirefoxMixin but refers to the tab in the HUDService. Another cleanup bug
> to file, I think.

Hehe, true.

> Looks good in general though. f+.

Thanks!

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 479746 [details] [diff] [review]
updated patch

Asking for review from Gavin.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 479746 [details] [diff] [review]
updated patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  onCloseButtonClick: function JSTF_onCloseButtonClick()
>+  {
>+    let contentWindow = this.context.get();
>+    let gBrowser = this.parentNode.ownerDocument.defaultView.gBrowser;
>+    let tab = gBrowser._getTabForContentWindow(contentWindow);
>+    HUDService.deactivateHUDForContext(tab);

Sorry but this is really crazy... this methods gets a tabbrowser from a content window, a tab from that tabbrowser, which then gets passed to deactivateHUDForContext, which gets the contentWindow from the tab, passes it to the active window's gBrowser to try to find a <browser>, which gets the notification box to get the hudId to close. Even ignoring the fact that deactivateHUDForContext uses currentContext() (won't that cause the button to fail when clicked in a non-current window?), the multiple steps you need to go through is rather ridiculous. This method should look like |HUDService.close(this.hudID)|, or something similarly concise. I'm not sure what re-factoring is necessary to make that possible, but it seems like it should be possible for the JSTermFirefoxMixin to know it's hudID...

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> Comment on attachment 479746 [details] [diff] [review]
> updated patch
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+  onCloseButtonClick: function JSTF_onCloseButtonClick()
> >+  {
> >+    let contentWindow = this.context.get();
> >+    let gBrowser = this.parentNode.ownerDocument.defaultView.gBrowser;
> >+    let tab = gBrowser._getTabForContentWindow(contentWindow);
> >+    HUDService.deactivateHUDForContext(tab);
> 
> Sorry but this is really crazy... this methods gets a tabbrowser from a content
> window, a tab from that tabbrowser, which then gets passed to
> deactivateHUDForContext, which gets the contentWindow from the tab, passes it
> to the active window's gBrowser to try to find a <browser>, which gets the
> notification box to get the hudId to close. Even ignoring the fact that
> deactivateHUDForContext uses currentContext() (won't that cause the button to
> fail when clicked in a non-current window?), the multiple steps you need to go
> through is rather ridiculous. This method should look like
> |HUDService.close(this.hudID)|, or something similarly concise. I'm not sure
> what re-factoring is necessary to make that possible, but it seems like it
> should be possible for the JSTermFirefoxMixin to know it's hudID...

This amount of craziness made me in the first place to submit a patch that does not call deactivateHUDForContext(tab). :) I simply included the following code:

+    closeButton.addEventListener("command", function() {
+      let displayNode = self.parentNode.parentNode.parentNode;
+      let hudId = displayNode.getAttribute("id");
+      let window = self.context.get();
+
+      HUDService.unregisterActiveContext(hudId);
+      HUDService.unregisterDisplay(displayNode);
+
+      if (window) {
+        window.focus();
+      }
+    }, false);

(please see attachment 478548 [details] [diff] [review])

Still, I do agree with Patrick that we'd need a central method for closing the HUD ... this is why I updated the patch to call deactivateHUDForContext(tab).

HUDService.close(hudId) is simply the same as the above code - yet another way to close a HUD.

Also, please note that the currentContext() bug is fixed in bug 597103 - which is marked as a dependency for this bug.

Now I am unsure what to do. Please tell me what is the preferred way of going forward with this patch. Thanks!

Comment 9

[Mihai Sucan [:msucan]]

This bug is going to be fixed by the patch from bug 577721, due to the addition of the HeadsUpDisplay.tab property and the use of HUDService.deactivateHUDForContext(tab).