663443 - No indication to which tab a Web Console window belongs (esp. an empty one)

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[[:Aleksej]]

There is no indication of which tab a Web Console window is specific to.

Steps to reproduce:
1. Open Web Console in a tab.
(optional) 2. Cause activity in the active tab.
3. Open a new tab.
(optional) 4. Cause activity in the active tab.
5. Press "Clear" in the Web Console.
(optional) 6. Cause activity in the active tab.

At steps 3-4, it is not immediately clear why the Web Console does not react.
At steps 5-6, it is not clear how to find out which tab the Web Console window belongs to without trying each tab.

See also bug 634423 (hide Web Console on switching) and bug 604214 (rename "Web Console" into "Tab Console").

Comment 1

[Kevin Dangoor]

To clarify, because I missed it when just going through the STR, you need to set the Web Console to position in a window.

My initial thought here is that we should hide the Web Console when you leave the tab for which you've opened it. Another option would be to have the title of the web console window be the URL of the tab that it's tied to.

Comment 2

[Johnathan Nightingale [:johnath]]

Speculatively tracking-firefox-6+ on this because it looks like a regression as a result of firefox-6-specific changes - we need to understand pretty quickly whether this is trivially fixable, or whether we should back out the change that caused it.

Rob,Panos,Mihai,Kevin  - can we get some analysis here?

Comment 3

[Mihai Sucan [:msucan]]

(In reply to comment #1)
> To clarify, because I missed it when just going through the STR, you need to
> set the Web Console to position in a window.
> 
> My initial thought here is that we should hide the Web Console when you
> leave the tab for which you've opened it. Another option would be to have
> the title of the web console window be the URL of the tab that it's tied to.

Any of the two approaches is fine.

(In reply to comment #2)
> Speculatively tracking-firefox-6+ on this because it looks like a regression
> as a result of firefox-6-specific changes - we need to understand pretty
> quickly whether this is trivially fixable, or whether we should back out the
> change that caused it.
> 
> Rob,Panos,Mihai,Kevin  - can we get some analysis here?

This is trivially fixable by a patch that displays the URL of the tab (and possibly the page title) in the Web Console window title.

Comment 4

[Johnathan Nightingale [:johnath]]

Can we get a patch that does one of the two in a safe way for Aurora? That seems preferable to backing out the change that gave us detachable consoles, but we'd need it quite soon since the next migration is in less than 3 weeks

Comment 5

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed fix for this bug.

I also pushed the patch to the try server:

http://tbpl.mozilla.org/?tree=Try&rev=49014a35340e

Looking forward to your review. Thank you!

Comment 6

[David Dahl :ddahl]

Comment on attachment 539764 [details] [diff] [review]
proposed patch

># HG changeset patch
># Date 1308220471 -10800
># User Mihai Sucan <[e-mail]>
># Parent 6da2c7c5c170e779421c34670d8cc9606f410e22
>Bug 663443 - No indication to which tab a Web Console window belongs
>
>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
>@@ -2818,16 +2818,20 @@ HUD_SERVICE.prototype =
>     let _browser = gBrowser.
>       getBrowserForDocument(aContentWindow.top.document);
>     let nBox = gBrowser.getNotificationBox(_browser);
>     let nBoxId = nBox.getAttribute("id");
>     let hudId = "hud_" + nBoxId;
>     let windowUI = nBox.ownerDocument.getElementById("console_window_" + hudId);
>     if (windowUI) {
>       // The Web Console popup is already open, no need to continue.
>+      if (aContentWindow == aContentWindow.top) {
>+        let hud = this.hudReferences[hudId];
>+        hud.reattachConsole(aContentWindow);
>+      }

Is this something we forgot to do in the original patch? or are you updating the title by calling reattachConsole?

Looks good!

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> Comment on attachment 539764 [details] [diff] [review] [review]
> proposed patch
> 
> ># HG changeset patch
> ># Date 1308220471 -10800
> ># User Mihai Sucan <[e-mail]>
> ># Parent 6da2c7c5c170e779421c34670d8cc9606f410e22
> >Bug 663443 - No indication to which tab a Web Console window belongs
> >
> >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
> >@@ -2818,16 +2818,20 @@ HUD_SERVICE.prototype =
> >     let _browser = gBrowser.
> >       getBrowserForDocument(aContentWindow.top.document);
> >     let nBox = gBrowser.getNotificationBox(_browser);
> >     let nBoxId = nBox.getAttribute("id");
> >     let hudId = "hud_" + nBoxId;
> >     let windowUI = nBox.ownerDocument.getElementById("console_window_" + hudId);
> >     if (windowUI) {
> >       // The Web Console popup is already open, no need to continue.
> >+      if (aContentWindow == aContentWindow.top) {
> >+        let hud = this.hudReferences[hudId];
> >+        hud.reattachConsole(aContentWindow);
> >+      }
> 
> Is this something we forgot to do in the original patch? or are you updating
> the title by calling reattachConsole?

Bits of both. This is something we forgot to do in the original patch, and it needs to be called to update the title now.


> Looks good!

Thank you for the review+!

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 539764 [details] [diff] [review]
proposed patch

Asking for a toolkit peer review as well.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 539764 [details] [diff] [review]
proposed patch

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

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +3230,5 @@
>      catch (ex) {}
>  
>      let panel = this.chromeDocument.createElementNS(XUL_NS, "panel");
>  
> +    let label = this.getStr("webConsoleOwnWindowTitle") + " - " + this.uriSpec;

* you've got redundant code in two places for generating this, should collapse into a single function

* this breaks in RTL locales

* the separator should probably be localize-able

* what do extra long URLs look like? ellipse-ified? cut off?

Comment 10

[Mihai Sucan [:msucan]]

(In reply to comment #9)
> * this breaks in RTL locales
> 
> * the separator should probably be localize-able

Can this patch change locales/strings? I wasn't sure if I am allowed to change strings in Aurora.

The idea I have is to change the string to allow passing the URL to the string (formatStringFromName), so the separator is included in the string, and it's localizable, and it should deal well with RTL languages. (since everything in that case is up to the localizer)

Does that sound fine?


> * what do extra long URLs look like? ellipse-ified? cut off?

Extra long URLs are up to the window manager. Should I limit them to a certain length?


Thanks for your review!

Comment 11

[Mihai Sucan [:msucan]]

Attachment: updated patch

Changed the string such that the separator is localizable, which should also address the RTL languages problem.

Please let me know if you want other changes. I am not sure if title length is a problem per se - should be the problem of the window manager / Gecko platform code to cut it off as needed.

If you want, I can cut the URL until the first "?" (so we include only the path, without parameters). Or I can cut it until the last "/" before "?", so we include only the path.

Thank you!

Comment 12

[Dietrich Ayala (:dietrich)]

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

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

r=me with these addressed.

::: toolkit/components/console/hudservice/HUDService.jsm
@@ +3364,5 @@
> +  /**
> +   * Retrieve the Web Console panel title.
> +   *
> +   * @return string
> +   *         The Web Console pane title.

nit: s/pane/panel/

::: toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
@@ +136,5 @@
>  webConsolePositionWindow=Window
>  
>  # LOCALIZATION NOTE (webConsoleOwnWindowTitle): The Web Console floating panel
>  # title.
> +webConsoleOwnWindowTitle=Web Console - %S

you need to rename the entity. this triggers notifications for localizer groups that action is required.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: [in-devtools] patch update 2

Updated the patch according to the previous review comment.

Thank you Dietrich!

What is the process to get this patch into Aurora?

Comment 14

[Mihai Sucan [:msucan]]

Please note that this patch has passed a round of runs on the tryserver.

Comment 15

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

Comment on attachment 540239 [details] [diff] [review]
[in-devtools] patch update 2

We can't make string changes on aurora, so this patch is unsuitable.

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 540239 [details] [diff] [review]
[in-devtools] patch update 2

Pushed to devtools:

http://hg.mozilla.org/projects/devtools/rev/30f88cca1ac2

Comment 17

[Mihai Sucan [:msucan]]

(In reply to comment #15)
> Comment on attachment 540239 [details] [diff] [review] [review]
> [in-devtools] patch update 2
> 
> We can't make string changes on aurora, so this patch is unsuitable.

What shall I do then?

Also implement the second proposed solution? Hide Web Console on tab switch. That can become a complex patch, may not be easy to keep it simple.

Comment 18

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

Mihai, I'd suggest either adding the URI and/or title to the window title without changing or adding strings.

If that's not possible, I'd say this is going to be WONTFIX for Fx6 and we'll get a proper fix in for Fx7.

Comment 19

[Mihai Sucan [:msucan]]

(In reply to comment #18)
> Mihai, I'd suggest either adding the URI and/or title to the window title
> without changing or adding strings.

I did that in the first patch, by appending " - " and the URI, but that was a no-go during review (see the above comments). Having the separator and the direction hard-coded is not appropriate for localization.

Comment 20

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

(In reply to comment #9)
> * this breaks in RTL locales

Is this really true? e.g. titlePrivateBrowsingSuffix seems to not pose a problem for ar or he (though it's possible it's currently suboptimal?).

> * the separator should probably be localize-able

I'm not sure this is needed - we don't localize the separator in the private browsing title suffix case (but again this may just be an existing issue). But either way, this point alone wouldn't be a blocker to landing this on Aurora.

Comment 21

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

ok, I'm confused now. I wanted to mark this WONTFIX-for-firefox6 and I'm getting a bit of a mixed signal.

Can we do this without introducing strings or not? Is it an l10n or rtl issue? If not, let's fix it! If it is, we can wait until Fx7.

Comment 22

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

If you guys don't think this needs to be fixed for Firefox 6, that's fine. I was just suggesting that the initial patch (attachment 539764 [details] [diff] [review]) would probably be suitable for Aurora, assuming I'm right in comment 20.

Comment 23

[Mihai Sucan [:msucan]]

Can attachment 539764 [details] [diff] [review] go in Fx6 and the one I landed in devtools ... go in Fx7?

I mean, for Fx6 we will have the l10n problem, but at least the fix is in.

For Fx7 we get the proper fix.

Does that sound good enough?

Comment 24

[Johnathan Nightingale [:johnath]]

Cc'ng l10n to answer Mihai's question in comment 23 (which pulls off of gavin's comment 20)

Comment 25

[Axel Hecht [:Pike]]

Landing conflicting patches on aurora and central seems like a lot of hassle, can't figure out how big of a problem this bug is. My gut-feeling would be that this can wait for 7.

CCing :rtl, too.

Comment 26

[(no longer active)]

(In reply to comment #20)
> (In reply to comment #9)
> > * this breaks in RTL locales
> 
> Is this really true? e.g. titlePrivateBrowsingSuffix seems to not pose a
> problem for ar or he (though it's possible it's currently suboptimal?).

I'm not sure if this what Dietrich was suggesting, but yes, this can be a problem for RTL locales.  Here is an example (upper case letters are RTL):

/http://mozilla.org - ELOSNOC BEW
^
|--- problem

This will happen on Windows where the title bar is RTL.

Fortunately, it's easy to fix: you should just append a LRM character to the URL.

> > * the separator should probably be localize-able
> 
> I'm not sure this is needed - we don't localize the separator in the private
> browsing title suffix case (but again this may just be an existing issue).
> But either way, this point alone wouldn't be a blocker to landing this on
> Aurora.

I agree that the separator doesn't necessarily need to be localizable (if I had to make the call, I'd have said let's not make it localizable).



While the above may suggest that I'm advocating to get this landed on Aurora, I'm actually very much against taking these sorts of fixes on Aurora.  I think we should first establish why this is needed on Aurora (it's not a critical bug at all) and then decide if the patch is good enough for Aurora or not.

Comment 27

[Johnathan Nightingale [:johnath]]

Yeah - nice to know that we *could* land this - but personally I'm fine with status-firefox6:wontfix

Comment 28

[Axel Hecht [:Pike]]

Ehsan's comment says that the (obsolete) patch we have is not good enough for Aurora. There *could* be a patch that may only hurt locales that are not happy about spaces as separators.

The patch landed in devtools has the complete string in it, and thus the localizer can set the LRM in the string to make the url direction right. So on devtools, we should be RTL-fine.

Comment 29

[(no longer active)]

(In reply to comment #28)
> The patch landed in devtools has the complete string in it, and thus the
> localizer can set the LRM in the string to make the url direction right. So on
> devtools, we should be RTL-fine.

Yes, that is correct.

Comment 30

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

ok then. I'm going to make the call based on comments 27, 22 and my original #21.

We'll wontfix this for 6 and get a proper fix landed for Fx7.

thanks for all your helpful comments!

Comment 31

[(no longer active)]

(In reply to comment #29)
> (In reply to comment #28)
> > The patch landed in devtools has the complete string in it, and thus the
> > localizer can set the LRM in the string to make the url direction right. So on
> > devtools, we should be RTL-fine.
> 
> Yes, that is correct.

Although, we should definitely add a localization note about this.

Comment 32

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

http://hg.mozilla.org/mozilla-central/rev/30f88cca1ac2

Comment 33

[(no longer active)]

(In reply to comment #31)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > The patch landed in devtools has the complete string in it, and thus the
> > > localizer can set the LRM in the string to make the url direction right. So on
> > > devtools, we should be RTL-fine.
> > 
> > Yes, that is correct.
> 
> Although, we should definitely add a localization note about this.

Mihai, would you mind filing a followup bug for this, please?

Comment 34

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110627 Firefox/7.0a1

Verified issue on the latest trunk on Ubuntu 11.04, Mac OS X 10.6, Win 7 and WinXP.

The url is displayed within the webconsole's title-bar.