625195 - "Move to New Window" is disabled if a tab is orphaned or the only tab in the active group

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Andreas Jung]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110112 Firefox/4.0b10pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110112 Firefox/4.0b10pre

Assume there are the following tab groups in TabCandy:
Group A: [Tab 1]
Group B: [Tab 2] [Tab 3]

If you now switch to Group A and right click on Tab 1 in the tab bar the context menu entry "Move to New Window" is disabled. It should be enable though since it isn't the only tab in the window.

Note that dragging the tab away to detach it to an new window works as expected.

Reproducible: Always

Comment 1

[Alex Lakatos[:AlexLakatos]]

Can reproduce on: Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110116 Firefox/4.0b10pre

Comment 2

[Raymond Lee [:raymondlee]]

The "close tab" should also be enabled if the total number of tabs (visible + hidden tabs) is bigger than 1.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 4

[Raymond Lee [:raymondlee]]

Comment on attachment 531573 [details] [diff] [review]
v1

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=7ecc37475b7e

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 531573 [details] [diff] [review]
v1

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

The patch and the test look good but nonetheless I think it would be better to provide a more generic solution like described below:

::: browser/base/content/browser.js
@@ +8497,5 @@
>  
> +    // Disable the "Move to New Window" when there is only one tab.
> +    document.getElementById("context_openTabInWindow").disabled = disabled;
> +
> +    disabled = gBrowser.visibleTabs.length == 1;

While this is a solution, could we rather extend the current scheme that uses tbattr="tabbrowser-multiple"? Like:

* (1) tabbrowser-multiple - these are enabled when having multiple tabs (just like our openTabInWindow).

* (2) tabbrowser-multiple-visible - these are only enabled when having multiple visible tabs. this is in fact what tabbrowser-multiple means at the moment.

We'd need to ask browser peers if they're fine with that change, but I think that's a cleaner solution and also nice for add-on developers. Though maybe in terms of backwards compliance we shouldn't touch "tabbrowser-multiple" and find a new value for (1).

::: browser/base/content/browser.xul
@@ -134,5 @@
>          </menupopup>
>        </menu>
>        <menuitem id="context_openTabInWindow" label="&moveToNewWindow.label;"
>                  accesskey="&moveToNewWindow.accesskey;"
> -                tbattr="tabbrowser-multiple"

See above.

::: browser/base/content/tabview/groupitems.js
@@ +1912,5 @@
>    },
>  
>    // ----------
>    // Function: uninit
> +  uninit : function GroupItems_uninit() {

Nit: space ("uninit:")

::: browser/base/content/test/tabview/browser_tabview_bug625195.js
@@ +17,5 @@
> +
> +  showTabView(function() {
> +    registerCleanupFunction(function () {
> +      while (gBrowser.tabs.length > 1)
> +        gBrowser.removeTab(gBrowser.tabs[gBrowser.tabs.length - 1]);

Nit: this is a bit easier and does the same:

gBrowser.removeTab(gBrowser.tabs[1]);

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v2

Updated the patch based on comment 5

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 532007 [details] [diff] [review]
v2

>   updateContextMenu: function updateContextMenu(aPopupMenu) {
>+    let disabled = gBrowser.tabs.length == 1;
>+
>     this.contextTab = document.popupNode.localName == "tab" ?
>                       document.popupNode : gBrowser.selectedTab;
>-    let disabled = gBrowser.visibleTabs.length == 1;

I'm not sure why you're moving this up...

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

(In reply to comment #7)
> Comment on attachment 532007 [details] [diff] [review] [review]
> v2
> 
> >   updateContextMenu: function updateContextMenu(aPopupMenu) {
> >+    let disabled = gBrowser.tabs.length == 1;
> >+
> >     this.contextTab = document.popupNode.localName == "tab" ?
> >                       document.popupNode : gBrowser.selectedTab;
> >-    let disabled = gBrowser.visibleTabs.length == 1;
> 
> I'm not sure why you're moving this up...
Moved it back to the original place.

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=2c0189c29db7

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/62b546e80604

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 532138 [details] [diff] [review]
Patch for check-in

Sorry for commenting after the bug was pushed to m-c but I didn't notice that any sooner:

>+  showTabView(function() {
>+    registerCleanupFunction(function () {
>+      if (gBrowser.tabs[1])
>+        gBrowser.removeTab(gBrowser.tabs[1]);
>+      if (gBrowser.tabs[2])
>+        gBrowser.removeTab(gBrowser.tabs[2]);
>+      TabView.hide();
>+    });

That's not gonna work for the second tab. So in case we're timing out there will still be two tabs. Much easier is this:

>+  showTabView(function() {
>+    registerCleanupFunction(function () {
>+      while (gBrowser.tabs.length > 1)
>+        gBrowser.removeTab(gBrowser.tabs[1]);
>+      TabView.hide();
>+    });

I don't know if we need to file an extra bug for this little fix. Maybe we could just add a follow-up patch?

Comment 11

[Raymond Lee [:raymondlee]]

(In reply to comment #10)
> Comment on attachment 532138 [details] [diff] [review] [review]
> Patch for check-in
> 
> Sorry for commenting after the bug was pushed to m-c but I didn't notice
> that any sooner:
> 
> >+  showTabView(function() {
> >+    registerCleanupFunction(function () {
> >+      if (gBrowser.tabs[1])
> >+        gBrowser.removeTab(gBrowser.tabs[1]);
> >+      if (gBrowser.tabs[2])
> >+        gBrowser.removeTab(gBrowser.tabs[2]);
> >+      TabView.hide();
> >+    });
> 
> That's not gonna work for the second tab. So in case we're timing out there
> will still be two tabs. Much easier is this:
> 
> >+  showTabView(function() {
> >+    registerCleanupFunction(function () {
> >+      while (gBrowser.tabs.length > 1)
> >+        gBrowser.removeTab(gBrowser.tabs[1]);
> >+      TabView.hide();
> >+    });
> 
> I don't know if we need to file an extra bug for this little fix. Maybe we
> could just add a follow-up patch?

Filed bug 657331

Comment 12

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110519 Firefox/6.0a1

Verified issue using steps from Comment 0 on Ubuntu 10.10, WinXP, Mac OS X 10.6, Win 7. Bug is no longer present.