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

VERIFIED FIXED in mozilla1.9.1a2

Status

()

Core
Document Navigation
--
enhancement
VERIFIED FIXED
16 years ago
2 years ago

People

(Reporter: Troy Telford, Assigned: bz)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Trunk
mozilla1.9.1a2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
blocking1.8.1 -
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cst: relnote on fix])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

16 years ago
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...
tabbed browser
Status: UNCONFIRMED → NEW
Component: Browser-General → Tabbed Browser
Ever confirmed: true
reassign for real
Assignee: asa → hyatt
QA Contact: doronr → sairuh

Comment 3

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

-> Future
Target Milestone: --- → Future
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?
Severity: enhancement → normal
Keywords: nsbeta1
OS: Windows XP → All
Hardware: PC → All
Target Milestone: Future → ---
> 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.
Severity: normal → enhancement

Comment 6

16 years ago
->future.  d&d to rearrange tab order would be a first step.
Target Milestone: --- → Future

Comment 7

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

Comment 8

16 years ago
Reassigning to new component owner.
Assignee: hyatt → jaggernaut

Updated

16 years ago
Keywords: helpwanted

Comment 9

16 years ago
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.

Updated

16 years ago
Blocks: 126299

Comment 10

16 years ago
nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Comment 11

16 years ago
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

15 years ago
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

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

Comment 14

15 years ago
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

15 years ago
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"
QA Contact: sairuh → pmac

Comment 16

14 years ago
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.
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

14 years ago
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).
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

14 years ago
I'm sorry boris, it was my mistake. I said the wrong bug. What about bug 102132?
That needs much of the same back-end infrastructure as this bug, with different
front-end code, of course.

Comment 22

13 years ago
*** Bug 241193 has been marked as a duplicate of this bug. ***
*** Bug 254711 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Blocks: 293029

Comment 24

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

Comment 25

12 years ago
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

12 years ago
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.
Flags: blocking-aviary2.0?

Comment 27

12 years ago
(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)
*** Bug 308310 has been marked as a duplicate of this bug. ***
*** Bug 311214 has been marked as a duplicate of this bug. ***
*** Bug 317120 has been marked as a duplicate of this bug. ***

Comment 31

12 years ago
*** Bug 319403 has been marked as a duplicate of this bug. ***
*** Bug 319793 has been marked as a duplicate of this bug. ***
*** Bug 321521 has been marked as a duplicate of this bug. ***
This should be possible in Gecko 1.9, but not in 1.8.1
Flags: blocking-aviary2? → blocking1.8.1-

Updated

12 years ago
Flags: blocking1.9a1?
*** Bug 323887 has been marked as a duplicate of this bug. ***

Comment 36

11 years ago
Created attachment 218596 [details] [diff] [review]
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?
Blocks: 315312
Whiteboard: [cst: relnote on fix]

Comment 37

11 years ago
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?
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?
(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

11 years ago
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?
What are the chances of this getting written and reviewed by SM1.1b?

Updated

11 years ago
Flags: blocking1.9a1? → blocking1.9-
Whiteboard: [cst: relnote on fix] → [cst: relnote on fix] [wanted-1.9]
(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

11 years ago
(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?

Updated

11 years ago
Duplicate of this bug: 365296
Flags: wanted1.9+
Whiteboard: [cst: relnote on fix] [wanted-1.9] → [cst: relnote on fix]

Updated

10 years ago
Duplicate of this bug: 412573

Comment 46

9 years ago
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.
It works at least on my computer. A bug thought: The down arrow indicating where the tab will be disappears after 1 second ,

Updated

9 years ago
Duplicate of this bug: 445192

Comment 49

9 years ago
(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.

Updated

9 years ago
Duplicate of this bug: 445192
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

9 years ago
(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?
Blocks: 102132
Created attachment 331025 [details] [diff] [review]
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?
Assignee: jag → bzbarsky
Status: NEW → ASSIGNED

Updated

9 years ago
Blocks: 420491
Created attachment 331711 [details] [diff] [review]
All working and such

Gavin, can you review the browser/tabbrowser changes?  jst, can you review the rest?
Attachment #218596 - Attachment is obsolete: true
Attachment #331025 - Attachment is obsolete: true
Attachment #331711 - Flags: superreview?(jst)
Attachment #331711 - Flags: review?(gavin.sharp)
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.
Attachment #331711 - Flags: review?(roc)
Attachment #331711 - Flags: review?(jst)
Keywords: helpwanted
Summary: Drag & Drop tabs between browser windows → [FIX]Drag & Drop tabs between browser windows
I still need to write some tests, too.  Might not happen till early next week.
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.
I sort of went for minimally invasive on the tabbrowser end.  I could do more refactoring if desired, of course.

Comment 59

9 years ago
Could this be used to move the toplevel docshell (<window> element) for bug 420491?
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

9 years ago
yeah, seems reasonable

Updated

9 years ago
Blocks: 225680
Product: Core → SeaMonkey
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
Attachment #331711 - Flags: superreview?(jst)
Attachment #331711 - Flags: superreview+
Attachment #331711 - Flags: review?(jst)
Attachment #331711 - Flags: review+
> 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 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.
Attachment #331711 - Flags: review?(gavin.sharp) → review-
> 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.
(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.
Attachment #331711 - Flags: review?(roc) → review+
Created attachment 332448 [details] [diff] [review]
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.
Attachment #331711 - Attachment is obsolete: true
Attachment #332448 - Flags: review?(gavin.sharp)
(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.
> 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.
Created attachment 332499 [details] [diff] [review]
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.
Attachment #332448 - Attachment is obsolete: true
Attachment #332499 - Flags: review?(gavin.sharp)
Attachment #332448 - Flags: review?(gavin.sharp)
(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.
> 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.
Created attachment 332538 [details] [diff] [review]
Returning the tab
Attachment #332499 - Attachment is obsolete: true
Attachment #332538 - Flags: review?(gavin.sharp)
Attachment #332499 - Flags: review?(gavin.sharp)
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.
Attachment #332538 - Flags: review?(gavin.sharp) → review+
Pushed changeset eb6fda3be5bb.

I still need to add some tests, but I might end up doing that when I get back.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

Comment 76

9 years ago
REOPENing as I don't see anything here yet: <http://hg.mozilla.org/comm-central/index.cgi/log/9cbd14a19dfdf208a104ac87baa8c27c4f5a19c4/suite/browser/tabbrowser.xml>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: bzbarsky → nobody
Status: REOPENED → NEW
Component: Tabbed Browser → Document Navigation
Product: SeaMonkey → Core
QA Contact: pmac → docshell
Summary: [FIX]Drag & Drop tabs between browser windows → [FIX]Drag & Drop tabs between browser windows back end
Assignee: nobody → bzbarsky
Target Milestone: Future → mozilla1.9.1a2
Resolving again.  I filed bug 449728 as the followup.
Blocks: 449728
Status: NEW → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 449730

Updated

9 years ago
No longer blocks: 449730
Depends on: 449730

Updated

9 years ago
Blocks: 449734

Comment 79

9 years ago
> Resolving again.  I filed bug 449728 as the followup.

Thank you. Much appreciated.
Depends on: 449778
Depends on: 449780
Depends on: 449781

Updated

9 years ago
Depends on: 449814

Updated

9 years ago
No longer depends on: 449814
I've backed this out since it possibly caused Tp regressions on Windows and OSX.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Looks like this wasn't the cause of the regression. Assuming there is still no change I will reland this in the morning.
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.
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
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: in-litmus?
Created attachment 334955 [details] [diff] [review]
Some basic mochitests for this
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?
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.
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.
Flags: in-testsuite? → in-testsuite+
(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?
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.
> 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!
Blocks: 454366
No longer blocks: 454366

Comment 91

9 years ago
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.
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.
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.
Depends on: 465910
No longer depends on: 465910

Updated

9 years ago
Depends on: 466084
Depends on: 458697

Updated

8 years ago
Depends on: 477014

Updated

8 years ago
Depends on: 462673

Updated

8 years ago
Depends on: 499523
We have some tests for tab d&d between windows in our Firefox 3.5 test-run. Flagging in-litmus+.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+

Updated

8 years ago
Depends on: 505312

Updated

8 years ago
Blocks: 522762

Updated

8 years ago
Blocks: 522787
No longer blocks: 522787

Updated

7 years ago
No longer blocks: 102132
No longer blocks: 315312

Updated

3 years ago
Depends on: 1090274
You need to log in before you can comment on or make changes to this bug.