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

RESOLVED FIXED in Firefox 3 beta3

Status

()

Firefox
Tabbed Browser
--
enhancement
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Andreas Wuest, Assigned: Simon Bünzli)

Tracking

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [requires bug 393716 for full functionality])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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).
(Reporter)

Updated

12 years ago
Blocks: 290395
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
(Reporter)

Comment 2

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

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.
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.
No longer blocks: 290395
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: tab duplication (using ctrl) on tab drag and drop does not work → support tab duplication (using ctrl) on tab drag and drop
(Reporter)

Comment 5

12 years ago
(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?
Created attachment 192869 [details] [diff] [review]
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)
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.

Updated

12 years ago
Attachment #192869 - Flags: review?(mconnor)

Updated

12 years ago
Status: NEW → ASSIGNED

Updated

12 years ago
Assignee: nobody → martijn.martijn
Status: ASSIGNED → NEW
*** Bug 306011 has been marked as a duplicate of this bug. ***
Actually, the clone window extension seems to do approximately what is wanted here:
http://extensionroom.mozdev.org/more-info/clonewindow

Comment 10

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

Updated

12 years ago
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 11

12 years ago
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.
*** Bug 324920 has been marked as a duplicate of this bug. ***

Comment 13

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

11 years ago
*** Bug 354581 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 15

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

Duplicate of this bug: 365044

Updated

10 years ago
Attachment #192869 - Flags: review?(mconnor)
(Assignee)

Comment 18

10 years ago
Created attachment 262293 [details] [diff] [review]
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).
Assignee: martijn.martijn → zeniko
Status: NEW → ASSIGNED
Attachment #262293 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #262293 - Flags: review? → review?(mano)
(Assignee)

Comment 19

10 years ago
Asaf: any estimates when you'll get to review this patch (or any recommended reviewer to ask instead)?
Once it gets ui-review... (preferably from mconnor), I just noticed you don't have that set at all.
(Assignee)

Comment 21

10 years ago
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? ;-)
Attachment #262293 - Flags: ui-review?(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.
Attachment #262293 - Flags: ui-review?(mconnor) → ui-review-
(Assignee)

Comment 23

10 years ago
Created attachment 271014 [details] [diff] [review]
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.
Attachment #262293 - Attachment is obsolete: true
Attachment #271014 - Flags: ui-review?(mconnor)
Attachment #262293 - Flags: review?(mano)
(Assignee)

Comment 24

10 years ago
Created attachment 279332 [details] [diff] [review]
use SessionStore (unbitrotted)

Mike: Any estimate when you'll get to this bug another time?
Attachment #271014 - Attachment is obsolete: true
Attachment #279332 - Flags: ui-review?(mconnor)
Attachment #271014 - Flags: ui-review?(mconnor)
(Assignee)

Comment 25

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

Cancel that. This is not even wanted-1.9.
Attachment #279332 - Flags: ui-review?(mconnor)
(Assignee)

Updated

10 years ago
Assignee: zeniko → nobody
Status: ASSIGNED → NEW

Updated

10 years ago
Whiteboard: [wanted-firefox3]
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.
(Assignee)

Comment 27

10 years ago
I should have a few cycles available next week.
Assignee: nobody → zeniko
(Assignee)

Comment 28

10 years ago
Created attachment 283868 [details] [diff] [review]
unbitrotted
Attachment #279332 - Attachment is obsolete: true
Attachment #283868 - Flags: ui-review?(mconnor)
Attachment #283868 - Flags: review?(mconnor)
(In reply to comment #28)
> Created an attachment (id=283868) [details]
> unbitrotted

Looks like this patch contains a bunch of unrelated session store changes?
(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.
(Assignee)

Comment 31

10 years ago
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.
Assignee: zeniko → nobody
(Assignee)

Updated

10 years ago
Attachment #283868 - Flags: ui-review?(mconnor)
Attachment #283868 - Flags: review?(mconnor)
(Assignee)

Updated

10 years ago
Depends on: 407162
(Assignee)

Comment 32

10 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3 M11
(Assignee)

Updated

10 years ago
Assignee: nobody → zeniko
Status: ASSIGNED → NEW
(Assignee)

Updated

10 years ago
Depends on: 393716
(Assignee)

Comment 33

10 years ago
Created attachment 292631 [details] [diff] [review]
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.
Attachment #283868 - Attachment is obsolete: true
Attachment #292631 - Flags: ui-review?(mconnor)
Attachment #292631 - Flags: review?(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
Attachment #292631 - Flags: ui-review?(mconnor)
Attachment #292631 - Flags: ui-review+
Attachment #292631 - Flags: review?(mconnor)
Attachment #292631 - Flags: review+
(Assignee)

Comment 35

10 years ago
Created attachment 292646 [details] [diff] [review]
nit fixed

Thanks! So, what did I do right this time? ;-)
Attachment #292631 - Attachment is obsolete: true
Attachment #292646 - Flags: approval1.9?

Updated

10 years ago
Attachment #292646 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [wanted-firefox3] → [wanted-firefox3][requires bug 393716 for full functionality]
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Version: unspecified → Trunk

Comment 37

10 years ago
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?

Updated

10 years ago
Blocks: 408163

Comment 38

10 years ago
Does this mean I no longer need Duplicate Tab extension in trunk Firefox? Thanks!
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?
(Assignee)

Comment 40

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

Comment 41

10 years ago
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
(Assignee)

Comment 42

10 years ago
(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

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

Updated

10 years ago
Depends on: 410569

Comment 44

10 years ago
I filed bug 410569 about this feature not working on Mac.
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3][requires bug 393716 for full functionality] → [requires bug 393716 for full functionality]
You need to log in before you can comment on or make changes to this bug.