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

VERIFIED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
7 years ago
a year ago

People

(Reporter: Andreas Jung, Assigned: raymondlee)

Tracking

Trunk
Firefox 6
x86
Windows XP

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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
Component: Tabbed Browser → TabCandy
QA Contact: tabbed.browser → tabcandy

Updated

6 years ago
Version: unspecified → Trunk
Can reproduce on: Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110116 Firefox/4.0b10pre
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Future
(Assignee)

Comment 2

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

Comment 3

6 years ago
Created attachment 531573 [details] [diff] [review]
v1
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #531573 - Flags: feedback?(tim.taubert)
(Assignee)

Comment 4

6 years ago
Comment on attachment 531573 [details] [diff] [review]
v1

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=7ecc37475b7e
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]);
Attachment #531573 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 6

6 years ago
Created attachment 532007 [details] [diff] [review]
v2

Updated the patch based on comment 5
Attachment #531573 - Attachment is obsolete: true
Attachment #532007 - Flags: review?(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...
Attachment #532007 - Flags: review?(dao) → review+
(Assignee)

Comment 8

6 years ago
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
Attachment #532007 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #532138 - Attachment description: v3 → Patch for check-in
http://hg.mozilla.org/mozilla-central/rev/62b546e80604
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed → 4xp
Resolution: --- → FIXED
Target Milestone: Future → Firefox 6

Updated

6 years ago
Keywords: 4xp
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?
(Assignee)

Comment 11

6 years ago
(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
Blocks: 657331

Comment 12

6 years ago
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.