Closed
Bug 456002
Opened 16 years ago
Closed 16 years ago
Dragging and dropping a tab from a window with 1 tab is broken
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
VERIFIED
FIXED
Firefox 3.1b2
People
(Reporter: zurtex, Assigned: dao)
References
Details
(Keywords: regression, verified1.9.1)
Attachments
(1 file)
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce:
1. Create 2 windows of Firefox, 1 must only have 1 tab
2. Drag and drop that 1 tab to the other window of Firefox
3. Notice one tab that was originally in the 2nd window has now gone to whatever page the tab which drag and dropped had and a new blank unclosable tab has been created in the 2nd window.
Expected results:
A new tab from the 1st window should of been created.
Also the the error console says this:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.removeProgressListener]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://browser/content/tabbrowser.xml :: _beginRemoveTab :: line 1439" data: no]
Test on Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b1pre) Gecko/20080918073230 Minefield/3.1b1pre ID:20080918073230 and also confirmed on Windows Vista
Flags: blocking-firefox3.1?
Comment 1•16 years ago
|
||
Seen also with
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080918210621 Minefield/3.1b1pre
likely regression form bug 392870
Assignee | ||
Updated•16 years ago
|
Keywords: regression
Assignee | ||
Comment 2•16 years ago
|
||
Seems to work for me. Are you using an extension related to the tab bar?
Comment 3•16 years ago
|
||
(In reply to comment #2)
> Seems to work for me. Are you using an extension related to the tab bar?
Default profile.
STR
1. open a window (http://www.mozilla.org/projects/minefield/)
2. open second window, and go to whatever uri.
3. from window 1, drag tab to tab-bar in window 2
AR:
* a new tab is created 'untitled', the contents of tab1-window2 is replaced by the contents of tab1-window1
* the 'untitled tab' cannot be closed
Assignee | ||
Comment 4•16 years ago
|
||
Oh yeah, I was using a modified build.
Assignee | ||
Comment 5•16 years ago
|
||
Assignee | ||
Comment 6•16 years ago
|
||
Comment on attachment 339788 [details] [diff] [review]
patch
> // First, start teardown of the other browser. Make sure to not
>- // fire the beforeunload event in the process.
>+ // fire the beforeunload event in the process. Close the window
>+ // if the dragged tab was the last one.
Since this isn't limited to dragging tabs, this should read: "Close the other window if this was its last tab."
Comment 8•16 years ago
|
||
attachment 339788 [details] [diff] [review] makes on defaults closing the last document in a window a bit slower (and certainly makes it look noticeably slower) than before by loading a blank doc before closing the window. Could you avoid that temporary tab (/ whatever else is slowing it down)?
Assignee | ||
Comment 9•16 years ago
|
||
(In reply to comment #8)
> attachment 339788 [details] [diff] [review] makes on defaults closing the last document in a window a bit
> slower (and certainly makes it look noticeably slower) than before by loading a
> blank doc before closing the window. Could you avoid that temporary tab (/
> whatever else is slowing it down)?
--> bug 457096
Assignee | ||
Comment 11•16 years ago
|
||
Gavin: ping?
Comment 13•16 years ago
|
||
Comment on attachment 339788 [details] [diff] [review]
patch
>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> <method name="_beginRemoveTab">
> if (aTab.localName != "tab")
>- aTab = this.mCurrentTab;
>+ return null;
Why this behavior change?
>+ if (closeWindow)
>+ this.addTab("about:blank");
>+ else
>+ BrowserOpenTab();
Aren't these effectively the same thing? Why is the distinction important?
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #13)
> > <method name="_beginRemoveTab">
>
> > if (aTab.localName != "tab")
> >- aTab = this.mCurrentTab;
> >+ return null;
>
> Why this behavior change?
The current behavior doesn't seem to make sense to me and bugs me every time I look at it (and others, e.g. bz in bug 113934), but it never seemed important enough to file a bug. Do you want me to file a separate bug? Or can you explain why the code is doing what it's doing?
> >+ if (closeWindow)
> >+ this.addTab("about:blank");
> >+ else
> >+ BrowserOpenTab();
>
> Aren't these effectively the same thing? Why is the distinction important?
BrowserOpenTab focuses the location bar, which we don't need in the former case and could cause visible jitter (as far as I remember, it actually did when I tried it). I can add a comment regarding this.
Comment 15•16 years ago
|
||
Comment on attachment 339788 [details] [diff] [review]
patch
(In reply to comment #14)
> The current behavior doesn't seem to make sense to me and bugs me every time I
> look at it (and others, e.g. bz in bug 113934), but it never seemed important
> enough to file a bug. Do you want me to file a separate bug? Or can you explain
> why the code is doing what it's doing?
I actually misinterpreted it as a null check; I agree that it doesn't make much sense as it is. Bug 107848 comment 25 suggests that event listeners on non-tab elements used to pass |this| as the parameter, or something. I think we can probably get rid of it, but a new bug would be good.
> I can add a comment regarding this.
Sounds good.
Attachment #339788 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 16•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
Comment 17•16 years ago
|
||
If I use the STR in comment #3 At step 3 when I mouse down on the tab in order to drag it I immediately get the following in the error console:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]" nsresult: "0x80004002 (NS_NOINTERFACE)" location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0" data: no]
The D&D operation is however successful in a today's nightly build containing this patch (20081022).
Updated•16 years ago
|
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Comment 18•16 years ago
|
||
The patch causes a regression.
https://bugzilla.mozilla.org/show_bug.cgi?id=406216
> // Remove this tab as the owner of any other tabs, since it's going away.
>- for (var i = 0; i < this.mTabs.length; ++i) {
>- var tab = this.mTabContainer.childNodes[i];
>+ for (let i = 0; i < l; ++i) {
>+ let tab = this.mTabs[i];
This section does it. The number of tabs is possibly changed by an add-on who listen "TabClose" event. "l" indicates the old number of tabs, so, "this.mTabs[i]" possibly refers invalid item.
Assignee | ||
Comment 19•16 years ago
|
||
(In reply to comment #18)
> The patch causes a regression.
> https://bugzilla.mozilla.org/show_bug.cgi?id=406216
>
> > // Remove this tab as the owner of any other tabs, since it's going away.
> >- for (var i = 0; i < this.mTabs.length; ++i) {
> >- var tab = this.mTabContainer.childNodes[i];
> >+ for (let i = 0; i < l; ++i) {
> >+ let tab = this.mTabs[i];
>
> This section does it. The number of tabs is possibly changed by an add-on who
> listen "TabClose" event. "l" indicates the old number of tabs, so,
> "this.mTabs[i]" possibly refers invalid item.
Can you please file a new bug and make it blocking this one?
Assignee | ||
Comment 20•16 years ago
|
||
Oh, I guess that's what bug 406216 is about...
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 21•16 years ago
|
||
verified
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•