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)
Tracking
()
RESOLVED
FIXED
Firefox 2 beta2
People
(Reporter: ria.klaassen, Assigned: zeniko)
Details
(Keywords: fixed1.8.1)
Attachments
(1 file, 1 obsolete file)
1.32 KB,
patch
|
dietrich
:
review+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
- 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?
Reporter | ||
Comment 2•18 years ago
|
||
(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?
Updated•18 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Assignee | ||
Comment 3•18 years ago
|
||
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)
Comment 4•18 years ago
|
||
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 5•18 years ago
|
||
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-
Assignee | ||
Comment 6•18 years ago
|
||
(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?
Comment 7•18 years ago
|
||
(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.
Updated•18 years ago
|
Whiteboard: [at risk]
Assignee | ||
Comment 8•18 years ago
|
||
This patch looks uglier by the minute - but it does the job.
Attachment #228558 -
Attachment is obsolete: true
Attachment #229900 -
Flags: review?(dietrich)
Comment 9•18 years ago
|
||
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+
Comment 10•18 years ago
|
||
Checked in to trunk, confirmed in tinderbuild.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [at risk]
Comment 11•18 years ago
|
||
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?
Updated•18 years ago
|
Target Milestone: --- → Firefox 2 beta2
Comment 12•18 years ago
|
||
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+
Updated•18 years ago
|
Assignee: nobody → zeniko
Updated•18 years ago
|
Whiteboard: [checkin needed (1.8 branch)]
Updated•18 years ago
|
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.
Description
•