Closed Bug 612470 Opened 14 years ago Closed 13 years ago

Closing the last tab of a group switches to another group even if an app tab is selected

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Mardak, Assigned: pcwalton)

References

Details

(Whiteboard: [softblocker][ux][watch out for patch interaction with 627736][fx4-fixed-bugday])

Attachments

(1 file, 4 obsolete files)

The current group probably shouldn't be discarded if the last tab is closed but the user still has a tab open as an app tab. This can happen in a couple situations:

- Select a non-app tab then close all non-app tabs. Tabs from another group appear immediately.

- Select an app tab and click the close button of all the non-app tabs. Tab strip will now only contain app tabs but opening a new tab will cause another group's tabs to appear.
At minimum, the 2 different initial behaviors is confusing (other group's tabs appear immediately vs appearing only after creating a new tab).

What I was actually trying to do was for my "restaurants" group, I was getting rid of all the tabs with cmd-w then hit cmd-t to open yelp. But at that point, I was in some other random group.
blocking2.0: --- → ?
Blocks: 598154
Priority: -- → P3
blocking2.0: ? → betaN+
I can't reproduce this on trunk. Does this still happen for you?
Still happens for me. With both STRs.
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
Whiteboard: [softblocker]
Depends on: 624939
softblocker = critical
Severity: normal → critical
Severity: critical → normal
(In reply to comment #0)
> The current group probably shouldn't be discarded if the last tab is closed but
> the user still has a tab open as an app tab. This can happen in a couple
> situations:
> 
> - Select a non-app tab then close all non-app tabs. Tabs from another group
> appear immediately

This makes sense to me. 

> 
> - Select an app tab and click the close button of all the non-app tabs. Tab
> strip will now only contain app tabs but opening a new tab will cause another
> group's tabs to appear.

Even there are app tabs, I think when the last non-app tab is closed in a group, we should show other group's tabs to make things consistent.

Another suggestions?
Bug 624939 is a dupe of this bug, with a patch but no blocking status. So, is it better to move the patch to the blocker, or move the blocking status to the bug with the patch?
(In reply to comment #8)
> Bug 624939 is a dupe of this bug, with a patch but no blocking status. So, is
> it better to move the patch to the blocker, or move the blocking status to the
> bug with the patch?

I think it's better to move the patch to this blocker.

Assigning to pwalton@mozilla.com
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
No longer depends on: 624939
Attached patch Proposed patch. (obsolete) — Splinter Review
Here's a new version of the patch that rebases to tip, adds a test, and addresses Ian's review comments.
Attachment #505321 - Flags: review?
Attachment #505321 - Flags: review? → review?(ian)
(In reply to comment #7)
> (In reply to comment #0)
> Even there are app tabs, I think when the last non-app tab is closed in a
> group, we should show other group's tabs to make things consistent.
> 
> Another suggestions?

It isnt't nice! I dont want my group to close. It could have an "confirm" alert, at least...

But I noticed that if the group has a name, it won't close even if all the non-app tabs are closed! Its good :)
Comment on attachment 505321 [details] [diff] [review]
Proposed patch.

>-        if (!GroupItems.getUnclosableGroupItemId()) {

We do still need this; it makes sure we never close the last group if there are app tabs. It probably belongs in closeIfEmpty().

Adding Raymond, who wrote that code, to make sure I've got it right.

>+++ b/browser/base/content/test/tabview/browser_tabview_bug612470.js
>@@ -0,0 +1,76 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */

Please use the standard license block we've been using in the rest of the tests.

Otherwise looks great!
Attachment #505321 - Flags: review?(ian)
Attachment #505321 - Flags: review-
Attachment #505321 - Flags: feedback?(raymond)
Comment on attachment 505321 [details] [diff] [review]
Proposed patch.

Agreed with Ian, we need should close the last group if it has one or more app tabs.
Attachment #505321 - Flags: feedback?(raymond) → feedback-
Attached patch Proposed patch, version 2. (obsolete) — Splinter Review
Patch version 2 addresses review comments.
Attachment #505321 - Attachment is obsolete: true
Attachment #505651 - Flags: review?(ian)
Comment on attachment 505651 [details] [diff] [review]
Proposed patch, version 2.

Thanks for making the changes! One thing: 

> >-        if (!GroupItems.getUnclosableGroupItemId()) {
> 
> We do still need this; it makes sure we never close the last group if there are
> app tabs. It probably belongs in closeIfEmpty().

On further reflection, it definitely needs to go in closeIfEmpty(); otherwise there'll be a loophole that will allow you to end up with no groups if you have app tabs. 

R+ with that fixed.
Attachment #505651 - Flags: review?(ian) → review+
i am not sure if this is a separate bug or not:
after closing the last non-app tab as described in this bug, focus moves to the app-tab in the switched-to group, instead of to the last focused tab in that group.
is the tab focus after closing a group an issue, or should the view always go back to tabcandy anyway?
(In reply to comment #17)
> i am not sure if this is a separate bug or not:
> after closing the last non-app tab as described in this bug, focus moves to the
> app-tab in the switched-to group, instead of to the last focused tab in that
> group.
> is the tab focus after closing a group an issue, or should the view always go
> back to tabcandy anyway?

This patch should fix that, as you'll remain the same group rather than switching.
bugspam. Moving b10 to b11
Blocks: 627096
bugspam. Removing b10
No longer blocks: 608028
Attached patch Proposed patch, version 3. (obsolete) — Splinter Review
Patch version 3 fixes test bustage.
Attachment #505651 - Attachment is obsolete: true
Attachment #506032 - Flags: review?(ian)
Comment on attachment 506032 [details] [diff] [review]
Proposed patch, version 3.

Looks great! Verified that this passes try for me locally... have you pushed to try?
Attachment #506032 - Flags: feedback+
(In reply to comment #22)
> Comment on attachment 506032 [details] [diff] [review]
> Proposed patch, version 3.
> 
> Looks great! Verified that this passes try for me locally... have you pushed to
> try?

Yup, it works.
Whiteboard: [softblocker] → [softblocker][ux][watch out for patch interaction with 627736]
Blocks: 623440
Comment on attachment 506032 [details] [diff] [review]
Proposed patch, version 3.

>+          let dontClose = !item.closedManually && gBrowser._numPinnedTabs > 0;
>+          self.remove(item, { dontClose: dontClose });

I imagine you don't need that _numPinnedTabs check now that closeIfEmpty is calling getUnclosableGroupItemId, yes?

Anyway, r+
Attachment #506032 - Flags: review?(ian) → review+
(In reply to comment #24)
> Comment on attachment 506032 [details] [diff] [review]
> Proposed patch, version 3.
> 
> >+          let dontClose = !item.closedManually && gBrowser._numPinnedTabs > 0;
> >+          self.remove(item, { dontClose: dontClose });
> 
> I imagine you don't need that _numPinnedTabs check now that closeIfEmpty is
> calling getUnclosableGroupItemId, yes?
> 
> Anyway, r+

We definitely need it; I just checked and removing that check reintroduces the bug.
http://hg.mozilla.org/mozilla-central/rev/ea8bf490e66d
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Causing oranges on Windows only:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug625424.js | there are 2 tabs in this groupItem - Got 1, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug625424.js | Found an unexpected tab at the end of test run: about:blank

Backing out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You weren't actually at fault, we just failed to notice that the push right before you landed that test, but the hateful buildbot decided not to bother running Windows Moth on that push, so it looked like it started from your push because yours was the first that actually ran the test. It kept failing after your backout, wound up fixed by http://hg.mozilla.org/mozilla-central/rev/5a0f46c26d12
http://hg.mozilla.org/mozilla-central/rev/63adefe33a92
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Backed out again:

http://hg.mozilla.org/mozilla-central/rev/14c9cceb7649
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 629009
Attached patch patch v4 (obsolete) — Splinter Review
The test for private browsing passes. I made some little changes, so requesting review again:

1.) groupItem.close() can now be called with an options object. The only supported option is {immediately: true} so that the groupItem is not closed animatedly. This was the timing problem with the private browsing test. The only part where this argument is given is in GroupItems.reconstitute() where unknown groups are deleted when the tabview state is restored from session.

2.) Another test started failing (that one restored a tab). So I merged the tabview part from bug 628270 to fix this.

Pushed to try. http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=38a71e2896d0
Attachment #506032 - Attachment is obsolete: true
Attachment #507374 - Flags: review?(ian)
Passed try.
Comment on attachment 507374 [details] [diff] [review]
patch v4

Looks great
Attachment #507374 - Flags: review?(ian) → review+
Attachment #507374 - Attachment is obsolete: true
This patch badly fails to apply.
Already checked in:

http://hg.mozilla.org/mozilla-central/rev/ea8bf490e66d

Sorry for neglecting to update this.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verfieid with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker][ux][watch out for patch interaction with 627736] → [softblocker][ux][watch out for patch interaction with 627736][fx4-fixed-bugday]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.