248612 - holding drag&drop pointer over a tab for a moment should open this tab to allow drag&drop of content from one tab to another tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

[Norbert]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040623 Firefox/0.9.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040623 Firefox/0.9.0+

I see now way to drag&drop from one tab to another!
This could be useful for example to drag&drop words into a form input field of
another site that is open in another tab.

If I select for example a string, drag it and hold the mouse pointer over
another tab, this tab never opens. So it's impossible to drag&drop from tab to tab.


I suggest to open tabs if the drag&drop cursor is placed for a few seconds over it.


(The win32 or KDE task bar also behave so. So it's possible to drag&drop from
window to window.
In Excel or OOo Calc it's possible to drag&drop from sheet to sheet in this way:
Holding the drag&drop-mouse-pointer for a moment over another tab, opens that
sheet.)

I see it as bug, that it's impossible to drag&drop from tab to tab, while it's
possible from window to window.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Norbert]

(In reply to comment #0)

correction:
"drag&drop cursor"->"drag&drop pointer"

Comment 2

[Peter van der Woude [:Peter6]]

I can confirm this "inconsistent" behaviour

Comment 3

[Asaf Romano (gone)]

confirmed.

As this is an RFE, adding this into summary and
Severity -> Enhancement.

Comment 4

[Asaf Romano (gone)]

Oopps, adding now.

Comment 5

[Steffen Wilberg]

"RFE" is deprecated :)
severity=enhancement is enough.

Comment 6

[Martijn Wargers (dead)]

*** This bug has been marked as a duplicate of 45411 ***

Comment 7

[Asaf Romano (gone)]

The tabbed browsing code is implemented separately for Firefox, REOPENING.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch

Ok, this makes it work for me, and takes into account the fix for bug 179656.
So dragging of tabs won't focus other tabs suddenly with this patch.

Comment 9

[Martijn Wargers (dead)]

There is a problem, though, when you select the new tab with this dragover
operation, the url bar won't be updated with the url of that tab, until the drag
operation has been completed. This is most likely caused by bug 203573.

Comment 10

[Martijn Wargers (dead)]

Attachment: patch2

This also fixes the problem mentioned in comment 9.

Comment 11

[Martijn Wargers (dead)]

Attachment: patch2

Fixes stupid errors in previous patch.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 241819 [details] [diff] [review]
patch2

As discussed on IRC, it's about the right time to remove the setTimeout hack altogether.

Comment 13

[Martijn Wargers (dead)]

Attachment: patch3

Updated the patch.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 241825 [details] [diff] [review]
patch3

>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================

>@@ -1723,18 +1720,24 @@
>               else if (targetAnonid == "scrollbutton-down" || 
>                        (targetAnonid == "alltabs-button" && 
>                         this.mTabContainer.getAttribute("overflow") == "true")) {
>                 pixelsToScroll = tabStrip.scrollIncrement;
>                 tabStrip.scrollByPixels(pixelsToScroll);
>               }
> 
>               var isTabDrag = (aDragSession.sourceNode.parentNode == this.mTabContainer);
>-              if (!isTabDrag)
>+              if (!isTabDrag) {
>+               if (!this.dragTime) this.dragTime = Date.now();

This warns at least the first time you drag a tab (!undefined), solution is to add a _dragTime field to this binding (initial value should be 0 IMO, not null).

>+               if (Date.now() > this.dragTime + 350) {

The default delay should be stored in a field too.

>+                  this.dragTime += 100000;

why?

>+                  aEvent.target.parentNode.selectedItem = aEvent.target;

|this.mTabContainer.selectedItem = aDragSession.sourceNode;| would be  better if I read this right.

>+            this.dragTime = null;

s/null/0.

Comment 15

[Martijn Wargers (dead)]

Attachment: patch4

(In reply to comment #14)
> >+                  this.dragTime += 100000;
> 
> why?

To prevent the tab from selecting a lot of times, but it's a stupid optimisation, so I removed it.

> |this.mTabContainer.selectedItem = aDragSession.sourceNode;| would be  better
> if I read this right.

I used the left part, but not the right part, I need aEvent.target, not aDragSession.sourceNode, because that's not a tab.

I followed the rest of your suggestions

Comment 16

[Asaf Romano (gone)]

Comment on attachment 242064 [details] [diff] [review]
patch4

>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================

> 
>+      <field name="mDragTime">0</field>
>+      <field name="mDragOverDelay">350</field>
>       <method name="onDragOver">
>         <parameter name="aEvent"/>
>         <parameter name="aFlavour"/>
>         <parameter name="aDragSession"/>
>         <body>
>           <![CDATA[
>             if (aDragSession.sourceNode) {
>               var tabStrip = this.mTabContainer.mTabstrip;
>@@ -1723,18 +1722,23 @@
>               else if (targetAnonid == "scrollbutton-down" || 
>                        (targetAnonid == "alltabs-button" && 
>                         this.mTabContainer.getAttribute("overflow") == "true")) {
>                 pixelsToScroll = tabStrip.scrollIncrement;
>                 tabStrip.scrollByPixels(pixelsToScroll);
>               }
> 
>               var isTabDrag = (aDragSession.sourceNode.parentNode == this.mTabContainer);
>-              if (!isTabDrag)
>+              if (!isTabDrag && aEvent.target.localName == 'tab') {

nit: s/'/".

>+                if (!this.mDragTime) 
>+                  this.mDragTime = Date.now();
>+                if (Date.now() > this.mDragTime + this.mDragOverDelay)

nit: >=


> 
>               var ib = this.mTabDropIndicatorBar;
>               var ind = ib.firstChild;
>               ib.setAttribute('dragging',
>                  aDragSession.canDrop ? 'true' : 'false');
> 
>@@ -1893,16 +1897,17 @@
>                 aDragSession.canDrop) {
>               var target = aEvent.relatedTarget;
>               while (target && target != this.mStrip)
>                 target = target.parentNode;
>               if (target)
>                 return;
>             }
>             this.mTabDropIndicatorBar.setAttribute('dragging','false');
>+            this.mDragTime = 0;

I think you should reset it right after the |if (!isTabDrag && aEvent.target.localName == 'tab') {| block instead, otherwise it might left set due to early returns.

r=mano otherwise.

Comment 17

[Martijn Wargers (dead)]

Attachment: patch5

This patch has the nits fixed.

(In reply to comment #16)
> I think you should reset it right after the |if (!isTabDrag &&
> aEvent.target.localName == 'tab') {| block instead, otherwise it might left set
> due to early returns.

When I do that, all subsequent tabs get selected directly when I drag over them. I don't think that's desirable. I think you would always want the 350ms pause before the new tab gets selected.
I've moved the 'this.mDragTime = 0;' to the beginning. the ondragexit code seems to be called also when you drop inside the tabbrowser bar, so no 'this.mDragTime = 0;' is needed in ondrop. Is this acceptable?

Comment 18

[Asaf Romano (gone)]

Comment on attachment 242076 [details] [diff] [review]
patch5

r=mano, thanks!

Comment 19

[Martijn Wargers (dead)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.717; previous revision: 1.716
done
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.206; previous revision: 1.205
done

Checked into trunk.

Comment 20

[Martijn Wargers (dead)]

I backed the patch out, since it may have caused 10% argo Ts regression, see:
http://groups.google.com/group/mozilla.dev.performance/browse_thread/thread/a609357a1ce4b402/67bb1aaea3da632a#67bb1aaea3da632a

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So Ts went down on argo, but note that it didn't change anywhere else (either from your backout or from the original fix).  Not really sure what that means..

Comment 22

[Boris Zbarsky [:bzbarsky]]

Might mean that this is profile-dependent or something?

Comment 23

[Dão Gottwald [:dao]]

by the way, this didn't work if the drag source was another application (tested with Thunderbird).

Comment 24

[Adam Guthrie]

Dão, please file another bug for the issue you mentioned and make it block this bug. Thanks!

Comment 25

[Martijn Wargers (dead)]

When the drag source is from another application, then aDragSession.sourceNode is probably null. It shouldn't be too hard to fix, but it might be tricky, I'll try to fix that in a different patch.

But that leaves me the question if this patch can be checked in again?

Comment 26

[Martijn Wargers (dead)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.724; previous revision: 1.723
done
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.210; previous revision: 1.209
done

Checked in again on trunk.

I talked to vlad on this via irc and he agreed this could be checked in again.

For reference, see also the discussion at:
http://groups.google.nl/group/mozilla.dev.performance/browse_thread/thread/a609357a1ce4b402/67bb1aaea3da632a#67bb1aaea3da632a

Comment 27

[Dão Gottwald [:dao]]

please see bug 356819, too.(In reply to comment #25)
> When the drag source is from another application, then aDragSession.sourceNode
> is probably null. It shouldn't be too hard to fix, but it might be tricky, I'll
> try to fix that in a different patch.

I filed bug 356819.