Closed Bug 624847 Opened 9 years ago Closed 9 years ago

"Undo Close Tab" closes current group when only one blank tab is left

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 4.0b12

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file, 2 obsolete files)

(This happens only when TabCandy was activated before)

1. Create a group with 2 tabs (one of them blank)
2. Close the non-blank tab
3. Press Shift+Ctrl+T

Actual Result:

TabCandy is shown because the active group was deleted in background.

Expected Result:

Stay in chrome window with the closed tab restored.
(In reply to comment #0)
> Actual Result:
> TabCandy is shown because the active group was deleted in background.

At this point, if I press Ctrl+E to try to un-invoke TabCandy, it *starts* to zoom up my non-blank tab, but then fails and snaps back to the TabCandy view.  The next Ctrl+E after that succeeds, though.
(In reply to comment #1)
> At this point, if I press Ctrl+E to try to un-invoke TabCandy, it *starts* to
> zoom up my non-blank tab, but then fails and snaps back to the TabCandy view. 
> The next Ctrl+E after that succeeds, though.

Yeah you're right. I already fixed this issue in bug 624265 but we decided to split the patch. So there'll be patch later today.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #502964 - Flags: review?(ian)
Passed try.
Comment on attachment 502964 [details] [diff] [review]
patch v1

>+  prepareUndoCloseTab: function(blankTabToRemove) {
>+    if (this._window) {
>       this._window.UI.restoredClosedTab = true;
>+      
>+      if (blankTabToRemove)
>+        blankTabToRemove.tabItem.isRemovedAfterRestore = true;

It is possible to get into this situation with a blank pinned tab. This patch should take that into account. Perhaps instead of tacking something onto .tabItem, you should attach it directly to the xul:tab (i.e. blankTabToRemove in this case), like ._tabViewTabIsRemovedAfterRestore.

Flagging Dao for feedback on this suggestion.
Attachment #502964 - Flags: review?(ian)
Attachment #502964 - Flags: review-
Attachment #502964 - Flags: feedback?(dao)
bugspam. Moving b10 to b11
Blocks: 627096
bugspam. Removing b10
No longer blocks: 608028
Priority: -- → P3
Blocks: 629079
Duplicate of this bug: 629079
Attached patch patch v2 (obsolete) — Splinter Review
Pushed to try.
Attachment #502964 - Attachment is obsolete: true
Attachment #508069 - Flags: review?(ian)
Attachment #502964 - Flags: feedback?(dao)
Passed try.
Comment on attachment 508069 [details] [diff] [review]
patch v2

Looks good
Attachment #508069 - Flags: review?(ian) → review+
Attachment #508069 - Flags: approval2.0?
Comment on attachment 508069 [details] [diff] [review]
patch v2

This technically needs a browser peer to look at it, but I don't think the one line change actually matters enough to worry about it.
Attachment #508069 - Flags: approval2.0? → approval2.0+
Attachment #508069 - Attachment is obsolete: true
Keywords: checkin-needed
Target Milestone: Firefox 4.0b10 → Firefox 4.0b12
http://hg.mozilla.org/mozilla-central/rev/3c87074d5f50
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.