298571 - support tab duplication (using ctrl) on tab drag and drop

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[Andreas Wuest]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050620 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050620 Firefox/1.0+

When dragging and dropping tabs (neat feature!) and pressing ctrl to duplicate,
the dragged tab is not duplicated at its landing position but normally moved.

The HIG defines pressing ctrl and dropping an object as "copy object to dropped
location," as opposed to "move object to dropped location."

Reproducible: Always

Steps to Reproduce:
1. Drag a tab to another position within the tab bar
2. Press ctrl
3. Drop tab

Actual Results:  
The tab is ordinarily moved instead of copied

Expected Results:  
Copy the tab to dropping location by opening a new tab at this position with the
same URI (maybe even copying the horizontal and vertical scroll position of the
original tab).

Comment 1

[Ria Klaassen (not reading all bugmail)]

WFM on Windows when I hold Ctrl before dragging / during the entire action.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050623
Firefox/1.0+ ID:2005062304

Comment 2

[Andreas Wuest]

(In reply to comment #1)

> WFM on Windows when I hold Ctrl before dragging / during the entire action.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050623
> Firefox/1.0+ ID:2005062304

And you are not talking about bookmarks now but the tabs itself?

If this is the case, then it might be a problem with the gtk+ binding maybe?

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050619
Firefox/1.0+
I can confirm this behavior. Tab isn't copied.

Comment 4

[Mike Connor [:mconnor]]

Not going to happen for Deer Park, for the simple reason that we can't clone
browsers properly that I'm aware of.  Marking as enhancement, not a deer park
blocker.

Comment 5

[Andreas Wuest]

(In reply to comment #4)

> Not going to happen for Deer Park, for the simple reason that we can't clone
> browsers properly that I'm aware of.

Yeah, complete cloning is (nearly) impossible, think about currently running
scripts or external viewers bound to a document.

But what would be possible is to view tab duplication the same as opening a link
in a new tab (at the specified position, where URI is the URI copied from the
tab to be duplicated).

> Marking as enhancement, not a deer park
> blocker.

If we could implement this simple scheme with URI copying only, then we could
make it a bug against the tab drag & drop support, but not tagging it blocking
maybe?

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

I think this doesn approximately what you want.
I'm not sure if this is the most efficient way to do this, but it doesn't
really seem to cause any visual artefacts. (I was there was a way to directly
insert a new tab at a specific location)

Comment 7

[Martijn Wargers (dead)]

Actually, with bug 274784 (the fast back/forward bug) fixed, it could be that a
scripted solution like mentioned in comment 4 could be possible.

Comment 8

[Martijn Wargers (dead)]

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

Comment 9

[Martijn Wargers (dead)]

Actually, the clone window extension seems to do approximately what is wanted here:
http://extensionroom.mozdev.org/more-info/clonewindow

Comment 10

[Josh Birnbaum]

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

Comment 11

[Simon Bünzli]

In case you can't wait to get this functionality, I've adapted Pike's Clone Window extension for this case: http://forums.mozillazine.org/viewtopic.php?t=372494

BTW: Fixing this bug might/should get us tab dragging between windows almost for free (the above linked extension does this as well). And this bug might be related to bug 159357 - since getting full tab serialization would make both easy to fix.

Comment 12

[Martijn Wargers (dead)]

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

Comment 13

[John Mellor (Jomel)]

This would make a lot of sense, especially since the plus symbol added to the cursor already implies that the tab will be copied.

Could I ask the final implementor to make sure the same thing happens when dragging the url proxy icon (aka. the favicon to the left of the location bar)? Ctrl-dragging it should insert a copy of the current tab at the destination, instead of replacing it (feel free to open a new bug if the implementation of that turns out to be significantly different).

Comment 14

[Nickolay_Ponomarev]

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

Comment 15

[Simon Bünzli]

Using the new SessionStore API, copying most of a tab takes about a dozen lines (see bug 344640 comment #24). I've updated the proof-of-concept extension mentioned in comment #11 for testing and as a reference.

Comment 16

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #1)
> WFM on Windows when I hold Ctrl before dragging / during the entire action.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050623
> Firefox/1.0+ ID:2005062304

Sorry, wrong info; not in a new profile. Firefox behaves this way (also without pressing Ctrl) when I have tabs on the bottom and a certain (combination of) extensions installed. Having tabs on the bottom seem to trigger this behaviour easily, for I have different extensions now and still the same tab duplication.

Comment 18

[Simon Bünzli]

Attachment: copy tabs by means of SessionStore where available (fall back to URL copying)

As suggested in comment #15, this patch uses SessionStore to clone tabs as accurately as currently possible, falling back to minimal URL copying where SessionStore isn't available (i.e. for everything but Firefox; a tab then behaves at least as a bookmark / the page proxy icon / any other dropped link).

The patch fixes the issue of comment #13 and additionally adds copying/moving of tabs between windows (which is basically free due to the way tabs are serialized).

Comment 19

[Simon Bünzli]

Asaf: any estimates when you'll get to review this patch (or any recommended reviewer to ask instead)?

Comment 20

[Asaf Romano (gone)]

Once it gets ui-review... (preferably from mconnor), I just noticed you don't have that set at all.

Comment 21

[Simon Bünzli]

Comment on attachment 262293 [details] [diff] [review]
copy tabs by means of SessionStore where available (fall back to URL copying)

(In reply to comment #20)
> Once it gets ui-review... (preferably from mconnor)

If he's your preference, would you mind getting him to visit this bug anytime soon? ;-)

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 262293 [details] [diff] [review]
copy tabs by means of SessionStore where available (fall back to URL copying)

I actually read review mail again these days... :)

This is a fast pass code and UI review, its a major step forward to be sure, but there's some key things I want to settle before I ui-r this.  very nice though!
 
>-              var isTabDrag = (aDragSession.sourceNode.parentNode == this.mTabContainer);
>+              var isTabDrag = (aDragSession.sourceNode.localName == "tab");

if someone's using a <tabbox> in content, this gets weird, I think.  Edgecasey, but worth figuring out a less breakable check.  We don't have dnd on tabbox, yet, but people keep talking about it belonging there.


>+#ifndef XP_MACOSX
>+            var copyKeyHit = aEvent.ctrlKey;
>+#else
>+            var copyKeyHit = aEvent.metaKey;
>+#endif

we _really_ need to get a fix for bug 180840 someday

That said, accelKeyPressed is what I'd want for a varname.


>+            } else if (aDragSession.sourceNode && aDragSession.sourceNode.localName == "tab") {
>+              // copy the dropped tab and remove it from the other window
>+              // (making it seem to have moved between windows)
>+              newIndex = this.getNewIndex(aEvent);
>+              oldTab = aDragSession.sourceNode;
>+              newTab = this.duplicateTab(oldTab);
>+              this.moveTabTo(newTab, newIndex);
>+              this.selectedTab = newTab;
>+              oldTab.ownerDocument.defaultView.getBrowser().removeTab(oldTab);
>+              window.focus(); // removing the tab from the other window steals the focus

why is that?  one of the focus() calls in tabbrowser? can we suppress those sanely?

I don't think this part is quite right, because removeTab will blank the last tab, instead of closing the window.  Unfortunately, tabbrowser's still in toolkit, and thus we can't really close the window from here, though I'm pondering whether we'd want to just get the old window here and call close() on it... if its just a empty about:blank tab, it shouldn't warn/prompt...


>+      <method name="duplicateTab">

I didn't look at this behaviour, except to note that it follows default prefs and thus doesn't save/duplicate data from https pages.  I'd like to be able to specifically override in this case and preserve form data, etc.

Comment 23

[Simon Bünzli]

Attachment: copy tabs by means of SessionStore where available (fall back to URL copying)

(In reply to comment #22)
> if someone's using a <tabbox> in content, this gets weird, I think.

Added a further test: aTab has to belong to a ChromeWindow of windowtype == "navigator:browser" (or should we just check that the windowtypes of both windows involved match?).

> accelKeyPressed is what I'd want for a varname.

Done.

> one of the focus() calls in tabbrowser?

If it ever was - it's no longer there. Removed the focus() call.

> removeTab will blank the last tab, instead of closing the window.

Closing the window manually if we've moved the last tab. We could additionally check for the presence of SessionStore in that place, never close the tab if it isn't there, and assume that all browsers including SessionStore will behave as Firefox does. Of course, getting tabbrowser.xml out of Toolkit would be nicer.

> I'd like to be able to specifically override in this case and preserve form data, etc.

Refactored SessionStore to make this possible. We now provide a duplicateTab function which contains all the magic. I hope Dietrich will later find the time to review those bits.

Comment 24

[Simon Bünzli]

Attachment: use SessionStore (unbitrotted)

Mike: Any estimate when you'll get to this bug another time?

Comment 25

[Simon Bünzli]

Comment on attachment 279332 [details] [diff] [review]
use SessionStore (unbitrotted)

Cancel that. This is not even wanted-1.9.

Comment 26

[Mike Connor [:mconnor]]

actually it is, we just lost track of the bug (and the review)

Sadly, the patch is bitrotted, do you have cycles to unbitrot and re-request review from me?  This has been on my whiteboard since last week.

Sorry for the delays here, we do really want this in Fx3.

Comment 27

[Simon Bünzli]

I should have a few cycles available next week.

Comment 28

[Simon Bünzli]

Attachment: unbitrotted

Comment 29

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

(In reply to comment #28)
> Created an attachment (id=283868) [details]
> unbitrotted

Looks like this patch contains a bunch of unrelated session store changes?

Comment 30

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

(In reply to comment #29)
> Looks like this patch contains a bunch of unrelated session store changes?

Never mind this comment, I didn't realize this patch was using sessionstore.

Comment 31

[Simon Bünzli]

This is bitrotting again due to bug 398807. Don't have any cycles left for this for the weeks to come, though. Maybe Dietrich can take on what's left to do.

Comment 32

[Simon Bünzli]

Let's try this once again. The plan is to first refactor SessionStore in bug 407162 to make this easier (patch reviewed and hopefully to land shortly after Beta 2), then extend our SessionStore API to allow handling individual tabs in bug 393716 and finally just make the tabbrowser changes in this bug.

Comment 33

[Simon Bünzli]

Attachment: tabbrowser changes only

This patch adds a new duplicateTab method to tabbrowser.xml which either uses the new SessionStore API from bug 393716 for thorough copying/moving (note: not yet checked in!) or does basic URL copying when SessionStore has been disabled (extensions replacing SessionStore can of course easily replace this method).

Furthermore it adds a few checks and branches to onDrop to distinguish between tab copying and moving and between tabs from the current window as opposed to tabs from different windows.

Mike: In case you think you might not get to this before Beta 3, please advise in time, so that I can go looking for a different (ui-)reviewer. Thanks.

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 292631 [details] [diff] [review]
tabbrowser changes only


>+              }
>+              else {

>+              }
>+            } else if (draggedTab) {

please fix style to be consistent

this looks pretty good, thanks, and sorry for the delays

Comment 35

[Simon Bünzli]

Attachment: nit fixed

Thanks! So, what did I do right this time? ;-)

Comment 36

[Reed Loden [:reed]]

Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.254; previous revision: 1.253
done

Comment 37

[Steffen Wilberg]

metaKey is the command key on Mac, isn't it?
And we need that because aEvent.accelKey still doesn't work, which is bug 180840, correct?

Comment 38

[Worcester12345]

Does this mean I no longer need Duplicate Tab extension in trunk Firefox? Thanks!

Comment 39

[Martijn Wargers (dead)]

Simon, it seems to work fine, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121305 Minefield/3.0b3pre
However, I also thought that session data like text entered in forms also would be duplicated, but that doesn't seem to happen. Or am I wrong in thinking that?

Comment 40

[Simon Bünzli]

(In reply to comment #39)
It will once bug 393716 is fixed (see the whiteboard).

Comment 41

[Jesse Ruderman]

This feature isn't working for me.  I'm using the Proto theme with a debug build.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007121812 Minefield/3.0b3pre

Comment 42

[Simon Bünzli]

(In reply to comment #41)
Please define "not working".

And some more information would also help: Does it work with the official builds? Does it work with the default theme? Do you get any errors in the console?

Comment 43

[Oakwine]

It does not work for me either on a Mac.  New profile, no theme, official nightly build.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007122004 Minefield/3.0b3pre ID:2007122004

Instead of being copied, the tab is moved.  (Note: on the Mac I am using the Command key, since the Control key creates a drop-down menu).  Also, the 'plus cursor' does not show up.

There are no messages in the Error Console.

Comment 44

[Jesse Ruderman]

I filed bug 410569 about this feature not working on Mac.