Closed
Bug 113934
Opened 23 years ago
Closed 16 years ago
[FIX]Drag & Drop tabs between browser windows back end
Categories
(Core :: DOM: Navigation, enhancement)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla1.9.1a2
People
(Reporter: troyt, Assigned: bzbarsky)
References
(Depends on 2 open bugs, Blocks 2 open bugs)
Details
(Whiteboard: [cst: relnote on fix])
Attachments
(2 files, 5 obsolete files)
54.26 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
5.60 KB,
patch
|
Details | Diff | Splinter Review |
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...
Assignee | ||
Comment 1•23 years ago
|
||
tabbed browser
Status: UNCONFIRMED → NEW
Component: Browser-General → Tabbed Browser
Ever confirmed: true
Comment 3•23 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
Comment 4•23 years ago
|
||
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 → ---
Assignee | ||
Comment 5•23 years ago
|
||
> 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•23 years ago
|
||
->future. d&d to rearrange tab order would be a first step.
Target Milestone: --- → Future
Comment 7•23 years ago
|
||
*** Bug 118842 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Keywords: helpwanted
Comment 9•22 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.
Comment 11•22 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•22 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•22 years ago
|
||
jeff: Not sure what you mean. There already are keyboard shortcuts for switching between tabs.
Comment 14•22 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•22 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"
Updated•22 years ago
|
QA Contact: sairuh → pmac
Comment 16•20 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.
Assignee | ||
Comment 17•20 years ago
|
||
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•20 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).
Assignee | ||
Comment 19•20 years ago
|
||
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•20 years ago
|
||
I'm sorry boris, it was my mistake. I said the wrong bug. What about bug 102132?
Assignee | ||
Comment 21•20 years ago
|
||
That needs much of the same back-end infrastructure as this bug, with different front-end code, of course.
Comment 22•20 years ago
|
||
*** Bug 241193 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 23•20 years ago
|
||
*** Bug 254711 has been marked as a duplicate of this bug. ***
Comment 24•19 years ago
|
||
*** Bug 293029 has been marked as a duplicate of this bug. ***
Comment 25•19 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•19 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•19 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)
Comment 28•19 years ago
|
||
*** Bug 308310 has been marked as a duplicate of this bug. ***
Comment 29•19 years ago
|
||
*** Bug 311214 has been marked as a duplicate of this bug. ***
Comment 30•19 years ago
|
||
*** Bug 317120 has been marked as a duplicate of this bug. ***
Comment 31•19 years ago
|
||
*** Bug 319403 has been marked as a duplicate of this bug. ***
Comment 32•19 years ago
|
||
*** Bug 319793 has been marked as a duplicate of this bug. ***
Comment 33•19 years ago
|
||
*** Bug 321521 has been marked as a duplicate of this bug. ***
Comment 34•19 years ago
|
||
This should be possible in Gecko 1.9, but not in 1.8.1
Flags: blocking-aviary2? → blocking1.8.1-
Updated•18 years ago
|
Flags: blocking1.9a1?
Comment 35•18 years ago
|
||
*** Bug 323887 has been marked as a duplicate of this bug. ***
Comment 36•18 years ago
|
||
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: sm1.1
Whiteboard: [cst: relnote on fix]
Comment 37•18 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?
Comment 38•18 years ago
|
||
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•18 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•18 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•18 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•16 years ago
|
Flags: wanted1.9+
Whiteboard: [cst: relnote on fix] [wanted-1.9] → [cst: relnote on fix]
Comment 46•16 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.
Comment 47•16 years ago
|
||
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•16 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.
Comment 51•16 years ago
|
||
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•16 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?
Assignee | ||
Comment 53•16 years ago
|
||
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
Assignee | ||
Comment 54•16 years ago
|
||
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)
Assignee | ||
Comment 55•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Keywords: helpwanted
Summary: Drag & Drop tabs between browser windows → [FIX]Drag & Drop tabs between browser windows
Assignee | ||
Comment 56•16 years ago
|
||
I still need to write some tests, too. Might not happen till early next week.
Comment 57•16 years ago
|
||
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.
Assignee | ||
Comment 58•16 years ago
|
||
I sort of went for minimally invasive on the tabbrowser end. I could do more refactoring if desired, of course.
Comment 59•16 years ago
|
||
Could this be used to move the toplevel docshell (<window> element) for bug 420491?
Assignee | ||
Comment 60•16 years ago
|
||
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•16 years ago
|
||
yeah, seems reasonable
Updated•16 years ago
|
Product: Core → SeaMonkey
Comment 62•16 years ago
|
||
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+
Assignee | ||
Comment 63•16 years ago
|
||
> 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•16 years ago
|
||
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-
Assignee | ||
Comment 65•16 years ago
|
||
> 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•16 years ago
|
||
(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+
Assignee | ||
Comment 67•16 years ago
|
||
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)
Comment 68•16 years ago
|
||
(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.
Assignee | ||
Comment 69•16 years ago
|
||
> 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.
Assignee | ||
Comment 70•16 years ago
|
||
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)
Comment 71•16 years ago
|
||
(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.
Assignee | ||
Comment 72•16 years ago
|
||
> 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.
Assignee | ||
Comment 73•16 years ago
|
||
Attachment #332499 -
Attachment is obsolete: true
Attachment #332538 -
Flags: review?(gavin.sharp)
Attachment #332499 -
Flags: review?(gavin.sharp)
Comment 74•16 years ago
|
||
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+
Assignee | ||
Comment 75•16 years ago
|
||
Pushed changeset eb6fda3be5bb. I still need to add some tests, but I might end up doing that when I get back.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment 76•16 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 → ---
Assignee | ||
Comment 77•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee: nobody → bzbarsky
Target Milestone: Future → mozilla1.9.1a2
Assignee | ||
Comment 78•16 years ago
|
||
Resolving again. I filed bug 449728 as the followup.
Updated•16 years ago
|
Comment 79•16 years ago
|
||
> Resolving again. I filed bug 449728 as the followup.
Thank you. Much appreciated.
Comment 80•16 years ago
|
||
I've backed this out since it possibly caused Tp regressions on Windows and OSX.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 81•16 years ago
|
||
Looks like this wasn't the cause of the regression. Assuming there is still no change I will reland this in the morning.
Assignee | ||
Comment 82•16 years ago
|
||
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•16 years ago
|
||
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
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-litmus?
Assignee | ||
Comment 84•16 years ago
|
||
Comment 85•16 years ago
|
||
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?
Assignee | ||
Comment 86•16 years ago
|
||
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.
Assignee | ||
Comment 87•16 years ago
|
||
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+
Comment 88•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee | ||
Comment 90•16 years ago
|
||
> 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•16 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.
Assignee | ||
Comment 92•16 years ago
|
||
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•16 years ago
|
||
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•15 years ago
|
||
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+
You need to log in
before you can comment on or make changes to this bug.
Description
•