Closed Bug 456002 Opened 14 years ago Closed 14 years ago

Dragging and dropping a tab from a window with 1 tab is broken

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: zurtex, Assigned: dao)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file)

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?
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
Blocks: 392870
OS: Windows Server 2003 → All
Hardware: PC → All
Keywords: regression
Seems to work for me. Are you using an extension related to the tab bar?
(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
Oh yeah, I was using a modified build.
Attached patch patchSplinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #339788 - Flags: review?(gavin.sharp)
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."
Blocks: 456425
Duplicate of this bug: 457354
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)?
(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
Duplicate of this bug: 457990
Gavin: ping?
Duplicate of this bug: 460478
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?
(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 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+
http://hg.mozilla.org/mozilla-central/rev/1e91efd774f4

filed bug 461013
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
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).
Depends on: 462673
Flags: blocking-firefox3.1? → blocking-firefox3.1+
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.
(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?
Oh, I guess that's what bug 406216 is about...
Depends on: 406216
verified

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
No longer depends on: 462673
Depends on: 481560
You need to log in before you can comment on or make changes to this bug.