Closed Bug 625195 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 6

People

(Reporter: andreasjunghw, Assigned: raymondlee)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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
The "close tab" should also be enabled if the total number of tabs (visible + hidden tabs) is bigger than 1.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #531573 - Flags: feedback?(tim.taubert)
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+
Attached patch v2 (obsolete) — Splinter Review
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+
(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
Keywords: checkin-needed
Attachment #532138 - Attachment description: v3 → Patch for check-in
http://hg.mozilla.org/mozilla-central/rev/62b546e80604
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed4xp
Resolution: --- → FIXED
Target Milestone: Future → Firefox 6
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?
(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
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.