Closed Bug 343895 Opened 18 years ago Closed 18 years ago

Restoring first tab with Undo Close Tab creates an extra blank one

Categories

(Firefox :: Tabbed Browser, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 2 beta2

People

(Reporter: ria.klaassen, Assigned: zeniko)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

- Set browser.tabs.autoHide to false
- Open Firefox with one tab and load a page in it
- Middle-click on the tab and see the contents disappear
- Try to restore it with Undo Close Tab in the context-menu
- Result: two tabs, one with the restored site and one new blank tab
- Expected result: one tab with the restored site
Isn't this same as bug 320989?
(In reply to comment #1)
> 
The Undo Close Tab function was not yet implemented those days.
Maybe the fix of bug 320989 will also fix this bug, or it has some influence on it, I don't know. Any way, it is a trunk bug with low priority and this newly arisen problem looks pretty malfunctioning.

Flags: blocking-firefox2?
Flags: blocking-firefox2? → blocking-firefox2+
Attached patch handle this edge case (obsolete) — Splinter Review
That's about how I solved it for my Session Manager extension: if we've only got one empty tab and the tab bar isn't hidden, the empty tab is removed after the closed tab has been restored.

This could also be moved into nsSessionStore.js, although there might be an edge case where an extension wants to reopen a closed tab without actually overwriting the existing blank tab.
Attachment #228558 - Flags: review?(dietrich)
This doesn't play well with dom-generated windows, does it? (For example, a page which opens an "about:blank" window with toolbars=yes feature, and starts inserting dom content into its document).
Comment on attachment 228558 [details] [diff] [review]
handle this edge case

>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.657
>diff -u -8 -d -p -r1.657 browser.js
>--- browser/base/content/browser.js	6 Jul 2006 00:57:36 -0000	1.657
>+++ browser/base/content/browser.js	8 Jul 2006 22:30:41 -0000
>@@ -6804,12 +6804,22 @@ HistoryMenu.populateUndoSubmenu = functi
> }
> 
> /**
>  * Re-open a closed tab.
>  * @param aIndex
>  *        The index of the tab (via nsSessionStore.getClosedTabData)
>  */
> function undoCloseTab(aIndex) {
>+  var blankTabToRemove = null;
>+  if (tabbrowser.tabContainer.childNodes.length == 1 &&
>+      !gPrefService.getBoolPref("browser.tabs.autoHide") &&
>+      gBrowser.selectedBrowser.sessionHistory.count < 2 &&
>+      gBrowser.selectedBrowser.currentURI.spec == "about:blank")
>+    blankTabToRemove = gBrowser.selectedTab;
>+

This broke undo-close-tab, with this error:

Error: tabbrowser is not defined
Source File: chrome://browser/content/browser.js
Line: 6932

Maybe just missing a getBrowser() call in there?

>   var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>            getService(Ci.nsISessionStore);
>   ss.undoCloseTab(window, aIndex);
>+
>+  if (blankTabToRemove)
>+    gBrowser.removeTab(blankTabToRemove);
> }


Also, per Mano's comment, we need a check to see if the page has content prior to removing it. Or can we make the determination for his example by seeing if it's .opener is null?
Attachment #228558 - Flags: review?(dietrich) → review-
(In reply to comment #5)
> Maybe just missing a getBrowser() call in there?

tabbrowser should actually have read gBrowser. Maybe it'd be nicer to replace all those instances with browser = getBrowser() though.

> Also, per Mano's comment, we need a check to see if the page has content prior
> to removing it.

What about making sure that gBrowser.selectedBrowser.contentDocument.body.childNodes == 0?
(In reply to comment #6)
> (In reply to comment #5)
> > Maybe just missing a getBrowser() call in there?
> 
> tabbrowser should actually have read gBrowser. Maybe it'd be nicer to replace
> all those instances with browser = getBrowser() though.

Yeah, getBrowser() seems to be the standard in browser.js.

> > Also, per Mano's comment, we need a check to see if the page has content prior
> > to removing it.
> 
> What about making sure that
> gBrowser.selectedBrowser.contentDocument.body.childNodes == 0?
> 

IIRC, childNodes will be true even if there's no children, so should probably either use hasChildNodes() or childNodes.length == 0.
Whiteboard: [at risk]
Attached patch nits fixedSplinter Review
This patch looks uglier by the minute - but it does the job.
Attachment #228558 - Attachment is obsolete: true
Attachment #229900 - Flags: review?(dietrich)
Comment on attachment 229900 [details] [diff] [review]
nits fixed

Thanks, confirmed fixes the bug described, as well as mano's "about:blank with content" usurpation scenario.
Attachment #229900 - Flags: review?(dietrich) → review+
Checked in to trunk, confirmed in tinderbuild.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [at risk]
Comment on attachment 229900 [details] [diff] [review]
nits fixed

Problem: An edge-case where restoring a closed tab creates an extra blank tab.

Solution: Handle the confluence of factors that must be present to cause the bug by removing the blank tab.

Risk: low
Attachment #229900 - Flags: approval1.8.1?
Target Milestone: --- → Firefox 2 beta2
Comment on attachment 229900 [details] [diff] [review]
nits fixed

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.
Attachment #229900 - Flags: approval1.8.1? → approval1.8.1+
Assignee: nobody → zeniko
Whiteboard: [checkin needed (1.8 branch)]
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: