113934 - [FIX]Drag & Drop tabs between browser windows back end

Status: VERIFIED FIXED

(Core :: DOM: Navigation, enhancement)

Description

[Troy Telford]

I have no idea about the difficulty of this; so I seriously doubt it'll be given
consideration until after Mozilla 1.0 is released.

But, here it is anyway:

With two Mozilla browsers open, it would be nice to be able to 'drag & drop' the
tabs used in tabbed browsing between browser windows.  (Also provide a way to
drag a 'hidden' tab (meaning there is no tab-list of open sites, since only one
is open) to become a tab in another window.

I'm well aware that the 'open new windows as a tab' would fix most of the reason
for this; but I can see the ability to do this as being useful...

Comment 1

[Boris Zbarsky [:bzbarsky]]

tabbed browser

Comment 2

[Boris Zbarsky [:bzbarsky]]

reassign for real

Comment 3

[John Morrison]

there is another bug suggesting a similar (the same?) concept, which would be 
cool, but way to cool in the near term :-]

-> Future

Comment 4

[sairuh (rarely reading bugmail)]

the following work:

a. d&d link window A -> window B
b. d&d link in tab A -> tab B [in same window]
c. d&d link in tab in window A -> window B which has no tabs
d. d&d link in window B which has no tabs -> a tab [content area or tab widget
itself] in window A
e. d&d link in tab in window A -> tab in window B [content area or tab widget
itself]

is this still a problem? or, am i misunderstanding this bug --are you referring
to actually dragging the tab widget itself into another window?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> are you referring to actually dragging the tab widget itself into another 
> window?

That's what I think the reporter meant... hence marking this as an enhancement.

Comment 6

[Peter Trudelle]

->future.  d&d to rearrange tab order would be a first step.

Comment 7

[Gilles Durys]

*** Bug 118842 has been marked as a duplicate of this bug. ***

Comment 8

[David Hyatt]

Reassigning to new component owner.

Comment 9

[Michael Wardle]

This would be excellent.

It's made more necessary because parts of Mozilla (such as mailnews) still can't
open links in new tabs, so the only viable alternative is to open the link in a
new window, and I end up with windows and tabs all over the place, when I just
wanted one window with lots of tabs. :-(

As the reported stated, it would still be useful even if everything *were*
working correctly.

Comment 10

[Peter Trudelle]

nsbeta1-

Comment 11

[jag (Peter Annema)]

So what you do is go to the window you want the link opened in, add a new tab
(make sure it's active), switch back to mail/news, and click the link. No need
to open it in a new window at all.

Comment 12

[jeff wilkinson]

Perhaps related is that it would be nice to have keyboard shortcuts to switch
between tabs.  Perhaps Ctl-Tab rotates one way and Ctrl-Shift-Tab goes back the
other way a click at a time.  

Comment 13

[Garth Wallace]

jeff: Not sure what you mean. There already are keyboard shortcuts for switching
between tabs.

Comment 14

[James Yopp]

ABSOLUTELY!
  Tabs should be enhanced to allow the following:
 1. Drag & Drop Tab Widgets between windows
 2. Drag & Drop Tab Widgets into a new order within the same window
 3. Drag & Drop Tab Widgets to desktop, folders, etc. to make links
 4. Hide the close button when only one tab is visible
 5. Allow text from one tab to be dragged & dropped to forms on another tab

Tabs rock!  They are the biggest single innovation to the web browsing UI since
Opera introduced gestures.

ALSO: When the 4th & 5th mouse buttons (eg, intellimouse, etc.) are supported
for page back & forth, please include an option for modifier+(back) and
modifier+(fwd) to cycle through tabs...

Comment 15

[guanxi]

James - You'll find many of these already in Bugzilla.  See bug 126299.

"Hide the close button when only one tab is visible"

Click Edit >> Preferences >> Navigator >> Tabbed Browsing >> "Hide the close
button when only one tab is visible"

Comment 16

[John]

Isn't this a duplicate of bug 105885? If so, make sure to move your votes, and
add yourself to their cc list if still interested.

Comment 17

[Boris Zbarsky [:bzbarsky]]

No, this is not a duplicate.  Bug 105885 doesn't require embedding a document in
a new DOMWindow, necessarily, while this one does.  This bug is _much_ more
difficult to fix than that one.

Comment 18

[John]

Boris,
I'm sorry, no one has mentioned DOM on either bug (I think) and the user
experience is identical (AFAIK), (I'm not sure but) shouldn't the implementation
be the same? (I'm sorry if I'm being thick-headed but I know nothing of mozilla
code or much beyond the user experience, please enlighten me).

Comment 19

[Boris Zbarsky [:bzbarsky]]

This bug involves transferring a document into a new window object. The other
bug involves changing the order of <iframes> in a XUL document.  The latter
shouldn't require any changes to the implementation of documents and windows,
while the former most certainly requires those.

The key difference is that moving tabs within a single window doesn't involve
creating new window objects.

Comment 20

[John]

I'm sorry boris, it was my mistake. I said the wrong bug. What about bug 102132?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That needs much of the same back-end infrastructure as this bug, with different
front-end code, of course.

Comment 22

[José Jeria]

*** Bug 241193 has been marked as a duplicate of this bug. ***

Comment 23

[Boris Zbarsky [:bzbarsky]]

*** Bug 254711 has been marked as a duplicate of this bug. ***

Comment 24

[Jason Bassford]

*** Bug 293029 has been marked as a duplicate of this bug. ***

Comment 25

[mmortal03]

This is also useful when you have multiple tabs in one window, but you realize
that you want to compare the contents of two (or more) of them side by side. 
So, you open a new Window, and drag one of these tabs to the other window.  You
can currently drag a tab from one window into the address bar of another window,
but this replaces a tab there, and it doesn't remove the window you are dragging
it from, so it is not the same behavior.

Now here is an idea.  OSX has the ability to view all the contents of all the
windows currently open by a certain click or key press (I don't use OSX, so I
don't know off hand what this is.)  Regardless, you can then compare the
contents of one window to another, then release the key press, and you are back
to where you were.

Could there be a way to tile the contents of two somehow selected tabs side by
side within one window with a key press, so that you could compare them side by
side, and then release the key press to go back to where you were?

Comment 26

[Worcester12345]

The duplicate tab extension has a "merge windows" option which allows you to
merge all tabs from all windows, which is pretty decent for what it does, but it
is maybe too much for some people. Just thought I'd throw that in here in case
it helps someone as a workaround for the time being.

Comment 27

[Brian Schack]

(In reply to comment #25)
> Could there be a way to tile the contents of two somehow selected tabs side by
> side within one window with a key press, so that you could compare them side by
> side, and then release the key press to go back to where you were?
Tab Exposé in Shiira (http://hmdt-web.net/shiira/screenshot/en#tabExpose)

Comment 28

[Phil Ringnalda (:philor)]

*** Bug 308310 has been marked as a duplicate of this bug. ***

Comment 29

[Dave Townsend [:mossop]]

*** Bug 311214 has been marked as a duplicate of this bug. ***

Comment 30

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 317120 has been marked as a duplicate of this bug. ***

Comment 31

[baffclan]

*** Bug 319403 has been marked as a duplicate of this bug. ***

Comment 32

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 319793 has been marked as a duplicate of this bug. ***

Comment 33

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 321521 has been marked as a duplicate of this bug. ***

Comment 34

[Mike Connor [:mconnor]]

This should be possible in Gecko 1.9, but not in 1.8.1

Comment 35

[Pascal Chevrel:pascalc (PTO until April 26)]

*** Bug 323887 has been marked as a duplicate of this bug. ***

Comment 36

[jag (Peter Annema)]

Attachment: Proof of concept, needs code clean-up

Go ahead and play with this, I wasn't in the mood to come up with more creative variable names than "fromTab", and that check for localName == "tab" should probably check to make sure it's a tabbrowser tab (suggestions? :-)), but I'm currently mostly interested in whether 1) dragging the last tab in a window to another window should close the first window (implemented), and 2) should D&D leave active whatever window is active (implemented), or make the destination window always the active window?

Comment 37

[jag (Peter Annema)]

As CTho pointed out to me, using this to implement tab D&D across windows would cause flash to get reloaded.

In fact, since the document gets re-loaded, any modifications made through JS are lost too.

Form state is saved and restored properly though.

So, loss of DOM modifications and plugin state. Reason not to land a version of this patch?

Comment 38

[Justin Wood (:Callek)]

To me, making target window active makes the most sense, though I'm not a UI expert.

As far as flash reloading, JS not reflected, etc.  Would it be possible (feasable) to store a copy of the page in the session history (fastback), and load it from the other window?

Comment 39

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #37)
> As CTho pointed out to me, using this to implement tab D&D across windows would
> cause flash to get reloaded.
> 
> In fact, since the document gets re-loaded, any modifications made through JS
> are lost too.
> 
> Form state is saved and restored properly though.
> 
> So, loss of DOM modifications and plugin state. Reason not to land a version of
> this patch?
> 

Neil, can you comment on whether you think this is worth having?  I think it's probably good enough for the vast majority of situations to be worthwhile.  Plus, it might encourage aviary to copy it, and get them to pay someone to make the backend handle it better ;).

(In reply to comment #38)
> As far as flash reloading, JS not reflected, etc.  Would it be possible
> (feasable) to store a copy of the page in the session history (fastback), and
> load it from the other window?

I think that's what this patch does...

Comment 40

[[e-mail]]

I guess this patch is at least as good as browser back group or indeed undo close tab if you wouldn't mind implementing it ;-)

I'm not sure though as to how you could make the current page go into bfcache. Perhaps if you loaded about:blank into the tab first?

Comment 41

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

What are the chances of this getting written and reviewed by SM1.1b?

Comment 42

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #40)
> I guess this patch is at least as good as browser back group or indeed undo
> close tab if you wouldn't mind implementing it ;-)
> 
> I'm not sure though as to how you could make the current page go into bfcache.
> Perhaps if you loaded about:blank into the tab first?
> 

Well, we have an undo close tab patch awaiting review... if it turns out that that approach of using bfcache to save more is good, it would be easy to incorporate into the patch here.  Then, we wouldn't lose JS modifications or any of that good stuff.

Comment 43

[Worcester12345]

(In reply to comment #42)
...
> Well, we have an undo close tab patch awaiting review... if it turns out that
> that approach of using bfcache to save more is good, it would be easy to
> incorporate into the patch here....

Can you mark that bug to block this one then?

Comment 46

[mmortal03]

I think this is partially fixed in the trunk.  What it doesn't do at the moment is drag the content with no reload, it instead just loads the url in the new and closes the tab in the window you are dragging it from.

Comment 47

[Golyc - André Villar]

It works at least on my computer. A bug thought: The down arrow indicating where the tab will be disappears after 1 second ,

Comment 49

[Bear]

(In reply to comment #46)
> I think this is partially fixed in the trunk.  What it doesn't do at the moment
> is drag the content with no reload, it instead just loads the url in the new
> and closes the tab in the window you are dragging it from.

I filed a separate bug for this but it was marked duplicate, even though it is a separate issue.  I'm not requesting the feature, I'm reporting a bug with it.  

This needs to be filed as a separate bug.

Comment 51

[Sam "SammyTheSnake" Penny]

A connected issue (perhaps warranting a separate bug?)

Reproducible: Always

Steps to reproduce:
Set the Tab Bar to disappear when a window has only one tab
Open a window with one tab, and another with multiple tabs.
Drag a tab to a window that has only one tab

Experienced behaviour:
you can't *add* the dragged tab to the destination window, it insists on replacing the existing tab because there's no tab bar to position it on.

Suggested "fix": have the tab bar appear when another tab is dragged over where the tab bar would be if it were there, allowing the new tab to be positioned beside the existing tab or dropped "into" it according to user preference...

(tested in firefox 3.0 and iceweasel 3.0-rc2-2)

Comment 52

[Daniel Dawson]

(In reply to comment #51)
> Steps to reproduce:
> Set the Tab Bar to disappear when a window has only one tab
> Open a window with one tab, and another with multiple tabs.
> Drag a tab to a window that has only one tab
> 
> Experienced behaviour:
> you can't *add* the dragged tab to the destination window, it insists on
> replacing the existing tab because there's no tab bar to position it on.

That's not a bug. It's supposed to go to whatever URL is dragged over the content area. If you want to be able to drag a new tab on, just set the tab bar to always displayed. But perhaps this behavior could be configurable -- browser.tabs.opentabfor.dnd or something?

Comment 53

[Boris Zbarsky [:bzbarsky]]

Attachment: Backend parts of this, first cut

So we talked this over and decided that rather than trying to move a document from one DOMWindow to another we'd just move the entire docshell from one <xul:browser> to another.  This patch pretty much implements the back end of that.  There are a few minor cleanup tasks left there (wrt the docshell tree owner munging), but it basically seems to work.

The problem is testing it, and in particular that more work is needed on the tabbrowser/browser.xml end.  Specifically, there are the following issues outstanding:

1)  tabbrowser registers progress event listeners on the docshell.  Those need to be on the right docshell, and right now they're not.
2)  The movement should look as much like a tab close + tab open as possible.
3)  xbl fields cache their values

I was figuring I'd address this last by having a method on browser.xml that handles swapping all docshell-related field values between two browsers.

To address the first and second, I'm thinking I need to fork off (or refactor) part of removeTab, dispatch the right events as if the tab were closing in the old tabbrowser, create the new tab in the new tabbrowser, unhook various stuff from the new tab, swap, hook the various stuff up to the new tab.  The key being that we swap after dispatching the "tab closing" events, not after as this patch does.

Does that sound reasonable?  Is someone more familiar with tabbrowser willing to take a stab at it, or should I just give it a shot?

Comment 54

[Boris Zbarsky [:bzbarsky]]

Attachment: All working and such

Gavin, can you review the browser/tabbrowser changes?  jst, can you review the rest?

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 331711 [details] [diff] [review]
All working and such

And actually, it'd be great if roc can take a look at the nsSubDocumentFrame changes.

Comment 56

[Boris Zbarsky [:bzbarsky]]

I still need to write some tests, too.  Might not happen till early next week.

Comment 57

[Dão Gottwald [:dao]]

Comment on attachment 331711 [details] [diff] [review]
All working and such

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="removeTab">
>         <parameter name="aTab"/>
>         <body>
>           <![CDATA[
>+            var [oldTab, oldBrowser, index, currentIndex, length] =
>+              this._beginRemoveTab(aTab, true);
>+            this._endRemoveTab(oldTab, oldBrowser, index, currentIndex, length);
>+          ]]>
>+        </body>
>+      </method>


Carrying this stuff around shouldn't be necessary. oldBrowser is oldTab.linkedBrowser, index is oldTab._tPos, currentIndex isn't used in _beginRemoveTab and length is just this.mTabs.length. You only need to make sure to read some of this stuff before removing the tab element in _endRemoveTab.

Comment 58

[Boris Zbarsky [:bzbarsky]]

I sort of went for minimally invasive on the tabbrowser end.  I could do more refactoring if desired, of course.

Comment 59

[Sylvain Pasche]

Could this be used to move the toplevel docshell (<window> element) for bug 420491?

Comment 60

[Boris Zbarsky [:bzbarsky]]

Not easily, no.  This is pretty definitely specific to subframes.

On the other hand, for bug 420491 you could create a new window and move all the tabs from the current one over, right?  Nasty hack, but might be easier than trying to do surgery on nsXULWindow.

Comment 61

[Sylvain Pasche]

yeah, seems reasonable

Comment 62

[Johnny Stenback (:jst)]

Comment on attachment 331711 [details] [diff] [review]
All working and such

- In content/base/src/nsFrameLoader.h:

+  // The guts of an nsIFrameLoaderOwner::SwapFrameLoader implementation.  A
+  // frame loader owner needs to call this, and pass in the two references to nsRefPtrs for frame loaders that need to be swapped.

Re-indent to stay within 80 columns...

- In FirePageShowEvent():

+  for (PRUint32 i = 0; i < kids.Length(); ++i) {
+    if (kids[i]) {
+      FirePageShowEvent(kids[i], aChromeEventHandler);
+    }
+  }
+
+  nsPageTransitionEvent event(PR_TRUE, NS_PAGE_SHOW, PR_TRUE);
+  nsCOMPtr<nsIDOMDocument> doc = do_GetInterface(aItem);
+  event.target = do_QueryInterface(doc);
+  nsEventDispatcher::Dispatch(aChromeEventHandler, nsnull, &event);  

So FirePageHideEvent() fires fist on all the children and then on itself, which makes sense, but shouldn't the order be reversed here? Otherwise, you can have a child being shown in a parent that's not. It's not clear to me that the order really matters here, but conceptually it would seem to make sense to flip it around here.

The rest looks good to me! r+sr=jst

Comment 63

[Boris Zbarsky [:bzbarsky]]

> Re-indent to stay within 80 columns...

Will do.

> Otherwise, you can have a child being shown in a parent that's not.

That's correct.  pageshow is fired at the same time as onload, so that's the order it fires in for regular page loads.  I just modeled this ordering on that.

Comment 64

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 331711 [details] [diff] [review]
All working and such

>diff --git a/toolkit/content/widgets/browser.xml b/toolkit/content/widgets/browser.xml

>+      <method name="swapDocShells">

>+          var ourFieldValues = {};
>+          var otherFieldValues = {};
>+          for each (var field in fieldsToSwap) {
>+            ourFieldValues[field] = this[field];
>+            otherFieldValues[field] = aOtherBrowser[field];
>+          }
>+          this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner)
>+              .swapFrameLoaders(aOtherBrowser);
>+          for each (var field in fieldsToSwap) {
>+            this[field] = otherFieldValues[field];
>+            aOtherBrowser[field] = ourFieldValues[field];
>+          }

Alternatively you could just null out the fields and let the getters reinitialize them again, right? Don't think we really need to worry about the perf hit there.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+      <method name="swapBrowsersAndCloseOther">

>+            var remoteBrowser =
>+              aOtherTab.ownerDocument.defaultView.getBrowser();
>+            var tabCount = remoteBrowser.tabContainer.childNodes.length;

remoteBrowser.mTabs.length

>+            // Unhook our progress listener
...
>+            // Restore the progress listener

As far as I can tell this will break many of the progress notifications, because the listener holds a reference to the tabbrowser/browser/tab elements, and those will be wrong after the move. See:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/tabbrowser.xml&rev=1.271&mark=270,277-279#270
and
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/tabbrowser.xml&rev=1.271&mark=1225-1231#1225

I think you need to basically duplicate that second chunk of code and create a new listener here that has a reference to the correct tab/browser/browser elements.

I think Dao's suggested changes should be made too, though I don't mind if it's done in a followup.

Comment 65

[Boris Zbarsky [:bzbarsky]]

> Alternatively you could just null out the fields

If they're all fields that back up a getter, yes.  But if we ever switch to using the lazy field stuff here, the code as written now will be needed.  I figured I'd be reasonably future-safe, since the cost is low.  Your call on which you prefer, I guess.

I'll make the other changes.

Comment 66

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #65)
> If they're all fields that back up a getter, yes.  But if we ever switch to
> using the lazy field stuff here, the code as written now will be needed.  I
> figured I'd be reasonably future-safe, since the cost is low.

Sure, sounds good.

Comment 67

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Now using .linkedTab instead of oldTab, and this.mTabs.length instead of carrying around the length.

I left the |index| thing as-is, because I can't figure out why the code I factored out into findTabIndex() even existed if ._tPos works.  Perhaps it was just oversight and the body of findTabIndex() should be |return aTab._tPos;|?

But the other reason for leaving index, and also carrying along currentIndex, is that arbitrary chrome JS can run between the _begin and _end when swapping tabs.  So that state can go stale...

I addressed all the other comments.

Still need some tests, though I've done a good bit of manual testing.

Comment 68

[Dão Gottwald [:dao]]

(In reply to comment #67)
> I left the |index| thing as-is, because I can't figure out why the code I
> factored out into findTabIndex() even existed if ._tPos works.  Perhaps it was
> just oversight and the body of findTabIndex() should be |return aTab._tPos;|?

_tPos is updated whenever tabs are added, removed (i.e. in _endRemoveTab), moved etc., so that should be fine. IMHO you shouldn't even add the findTabIndex method.
The old code doesn't use it because it's older than _tPos.

> But the other reason for leaving index, and also carrying along currentIndex,
> is that arbitrary chrome JS can run between the _begin and _end when swapping
> tabs.  So that state can go stale...

I'm not sure what kind of code you have in mind, but isn't that an argument for determining the indices newly? With the current patch I would expect odd results if e.g. a tab is added between _begin and _end.
And in any case: These are pseudo-private methods; I don't think they need to work with arbitrary chrome code between them.

Comment 69

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what kind of code you have in mind

Extension event listeners and anything they care to call.  But ok.  I'm convinced.  At least enough that I just spent another 40 minutes futzing with the idiotic fragile indexing here getting it to work...  that makes more time spent on the tabbrowser part of this than on all the rest of it combined.

Comment 70

[Boris Zbarsky [:bzbarsky]]

Attachment: With all the return values of _beginRemoveTab gone

I have to say... this is some of the worst spaghetti-code I've had the misfortune to deal with in Mozilla.  _Everything_ has random side-effects....  And API documentation is nonexistent (well, possibly so is the concept of API).

It almost makes me want to implement private members in XBL or something.

Comment 71

[Dão Gottwald [:dao]]

(In reply to comment #69)
> > I'm not sure what kind of code you have in mind
> 
> Extension event listeners and anything they care to call.

But this doesn't apply to the current swapBrowsersAndCloseOther, does it? I don't think we're dispatching events there.

(In reply to comment #70)
> Created an attachment (id=332499) [details]
> With all the return values of _beginRemoveTab gone

I'm afraid you need to pass the tab, because, for a reason that I don't understand, _beginRemoveTab does this:
             if (aTab.localName != "tab")
               aTab = this.mCurrentTab;

Sorry for stealing your time with this. I should probably have done this in a followup bug.

Comment 72

[Boris Zbarsky [:bzbarsky]]

> But this doesn't apply to the current swapBrowsersAndCloseOther, does it?

Sure it does.  The C++ method it invokes to do the actual swap fires pagehide and pageshow events on the two browsers involved.

Good catch on the tab thing.

Comment 73

[Boris Zbarsky [:bzbarsky]]

Attachment: Returning the tab

Comment 74

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 332538 [details] [diff] [review]
Returning the tab

It's hard to resist suggesting some code cleanup when this code shows up in diffs (even though it's just being moved around), but I'll restrain myself :)

>diff --git a/toolkit/content/widgets/browser.xml b/toolkit/content/widgets/browser.xml

>+      <method name="swapDocShells">
>+        <parameter name="aOtherBrowser"/>
>+        <body>
>+        <![CDATA[
>+          // We need to swap fields that are tied to our docshell
>+          var fieldsToSwap = [ "_docShell", "_webBrowserFind" ];
>+
>+          // _fastFind is tied to the docshell if we don't have a tabbrowser
>+          if ((this.getTabBrowser() != null) !=
>+              (aOtherBrowser.getTabBrowser() != null)) {
>+            throw "Unable to perform swap on <browsers> if one is in a <tabbrowser> and one is not";
>+          }
>+
>+          if (!this.getTabBrowser()) {
>+            fieldsToSwap.push("_fastFind");
>+          }
>+          

nit: remove brackets around single-line then clause, put this.getTabBrowser() in a local variable, and remove trailing whitespace (you're adding some in other places in the patch too)?

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+            // XXXbz mLastURI seems to be unused
>             mLastURI: null,

It was unused when it was added in bug 250716, too, so just remove it?

>             // cache flags for correct status bar update after tab switching
>             mStateFlags: 0,
>             mStatus: 0,
>             mMessage: "",
>             mTotalProgress: 0,

Hmm, seems like we should probably be persisting these too? I suppose they only matter if there's a load in progress, and in those cases the progress events will eventually cause them to be repopulated anyways...

I tested out the progress listener code and it looks OK. I noticed an issue with bfcache not working after dragging a tab, I will file a followup for that.

I think we probably want to make it possible to drag tabs to a single-tab window as well, but that's a separate bug.

This makes me a bit nervous because I think we'll probably end up finding other bugs caused by event/progress listeners in browser code not handling the moves correctly, but I can't think of any potential problems at the moment. It would be nice to extend the relevant existing browser chrome tests in http://mxr.mozilla.org/seamonkey/source/browser/base/content/test/ to also test that things work after a tab move.

Comment 75

[Boris Zbarsky [:bzbarsky]]

Pushed changeset eb6fda3be5bb.

I still need to add some tests, but I might end up doing that when I get back.

Comment 76

[Philip Chee]

REOPENing as I don't see anything here yet: <http://hg.mozilla.org/comm-central/index.cgi/log/9cbd14a19dfdf208a104ac87baa8c27c4f5a19c4/suite/browser/tabbrowser.xml>

Comment 77

[Boris Zbarsky [:bzbarsky]]

And you won't, if you expect me to write it.  You also won't see a good tab drag/drop setup in Firefox if you expect me to write it.  I just hooked into the existing kinda-sad tab drag/drop code in toolkit tabbrowser (which I should not does NOT exist in the seamonkey tabbrowser) so I could have a way to test the backend changes, which are the part that really needed _me_ to work on it.

Better UI exposing said backend changes is going to have to be done by someone who actually knows the tabbrowser code (whichever tabbrowser is involved), and in a followup bug.

And yes, I just realized which product/component this bug is in.  I should probably have filed yet another bug for this bug to depend on....  Ah, well.  I guess I'll fix things to reflect reality, given that we can't move patches and comments from one bug to another.

Comment 78

[Boris Zbarsky [:bzbarsky]]

Resolving again.  I filed bug 449728 as the followup.

Comment 79

[Philip Chee]

> Resolving again.  I filed bug 449728 as the followup.

Thank you. Much appreciated.

Comment 80

[Dave Townsend [:mossop]]

I've backed this out since it possibly caused Tp regressions on Windows and OSX.

Comment 81

[Dave Townsend [:mossop]]

Looks like this wasn't the cause of the regression. Assuming there is still no change I will reland this in the morning.

Comment 82

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the code this added isn't triggered by anything other than the user performing a drag, so if it caused a Tp effect our tests are just broken.

Comment 83

[Dave Townsend [:mossop]]

Yeah I had already ruled out everything else I thought was in the window, apparently I need to go back to consulting the tea leaves.

Relanded: http://hg.mozilla.org/mozilla-central/index.cgi/rev/48191fdcf8a7

Comment 84

[Boris Zbarsky [:bzbarsky]]

Attachment: Some basic mochitests for this

Comment 85

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Boris, what is difference of this feature between 3.1 and 3.0? It still works for Firefox 3.0. So I wonder what improvements we have for 3.1?

Will there even be added more mochitests or can we mark it as in-testsuite?

Comment 86

[Boris Zbarsky [:bzbarsky]]

Try the following:

1)  Go to gmail
2)  Start composing a message
3)  Type in a to/subject/body
4)  Drag the tab to a different window

in 3.0 and 3.1a2.  Then you tell me what improvements we have.  ;)

> Will there even be added more mochitests or can we mark it as in-testsuite?

There aren't any added yet, since the tree has been closed ever since I wrote them.  Once I check them in (probably tomorrow), I'll mark it.

Comment 87

[Boris Zbarsky [:bzbarsky]]

Test checked in.  Note that this still needs litmus tests, since I'm not testing the tabbrowser parts in the mochitest, just the code and browser.xml code.

Comment 88

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #86)
> in 3.0 and 3.1a2.  Then you tell me what improvements we have.  ;)

That's awesome, Boris! Everything is kept and it looks like that no unload event and even others were fired by the d&d action. So this is the only thing we have to cover by Litmus tests? Or are there more improvements?

I tried to verify this bug in different ways and noticed a way where the transfer doesn't work. If you have opened a Gmail compose window like described above and you move it to another window without a tabbar visible, all the data entered before is lost. Same applies to some running js script e.g. http://www.usingjavascript.com/scripts/countdown.html.

Should I file a new bug on that?

Comment 89

[:Gavin Sharp [email: [e-mail]]]

We just don't hit this code at all in that case. This code only handles dragging tab objects to tabbars. You should file a bug on using this code for other drag actions where possible if they aren't already filed.

Comment 90

[Boris Zbarsky [:bzbarsky]]

> So this is the only thing we have to cover by Litmus tests? 

For now, yes.  I just made the existing UI drop action, which faked the tab moving across windows, do a better job of faking.  ;)  So to test you want a page that can have it's DOM modified (form controls or some other state like that; gmail is pretty ideal) and then drag it and drop it on the tabbar in a different window.  As you noted, dropping elsewhere doesn't use the new code yet; that will probably get changed in followup bugs.  If those aren't already filed, please do
file!

Comment 91

[Fritz]

https://bugzilla.mozilla.org/show_bug.cgi?id=457990

I suppose it's good to place this related bug here.  I don't know if this should be merged into here for a patch based on this bug or treated separated but it is a problem with Drag and Drop implementation at least on Mac.

Comment 92

[Boris Zbarsky [:bzbarsky]]

That has nothing to do with this bug, except insofar as the front-end code that uses this backend functionality sprouted a new (broken) codepath.

Comment 93

[Luke Iliffe (Harlequin99)]

Is there a bug filed for dropping tabs onto the content area of a different window without tab reloading? This bug only covers dropping onto the smaller target of the tabbar.

Comment 94

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We have some tests for tab d&d between windows in our Firefox 3.5 test-run. Flagging in-litmus+.