Last Comment Bug 625195 - "Move to New Window" is disabled if a tab is orphaned or the only tab in the active group
: "Move to New Window" is disabled if a tab is orphaned or the only tab in the ...
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: x86 Windows XP
: -- normal
: Firefox 6
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 657331
  Show dependency treegraph
 
Reported: 2011-01-12 14:50 PST by Andreas Jung
Modified: 2016-04-12 14:00 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (8.02 KB, patch)
2011-05-11 03:04 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v2 (8.23 KB, patch)
2011-05-12 12:25 PDT, Raymond Lee [:raymondlee]
dao+bmo: review+
Details | Diff | Splinter Review
Patch for check-in (8.42 KB, patch)
2011-05-13 01:23 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Andreas Jung 2011-01-12 14:50:21 PST
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] 2011-01-17 08:12:01 PST
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] 2011-01-17 10:50:21 PST
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] 2011-05-11 03:04:04 PDT
Created attachment 531573 [details] [diff] [review]
v1
Comment 4 Raymond Lee [:raymondlee] 2011-05-11 18:36:21 PDT
Comment on attachment 531573 [details] [diff] [review]
v1

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=7ecc37475b7e
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-12 07:12:50 PDT
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] 2011-05-12 12:25:09 PDT
Created attachment 532007 [details] [diff] [review]
v2

Updated the patch based on comment 5
Comment 7 Dão Gottwald [:dao] 2011-05-12 13:21:51 PDT
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] 2011-05-13 01:23:17 PDT
Created attachment 532138 [details] [diff] [review]
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 (⌚ UTC+1/+2) 2011-05-15 07:51:40 PDT
http://hg.mozilla.org/mozilla-central/rev/62b546e80604
Comment 10 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-15 13:11:53 PDT
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] 2011-05-16 05:48:28 PDT
(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 2011-05-20 04:40:57 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.