Closed Bug 248612 Opened 16 years ago Closed 13 years ago

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

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha1

People

(Reporter: norbert.notz, Assigned: martijn.martijn)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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:
(In reply to comment #0)

correction:
"drag&drop cursor"->"drag&drop pointer"
I can confirm this "inconsistent" behaviour
confirmed.

As this is an RFE, adding this into summary and
Severity -> Enhancement.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Oopps, adding now.
Summary: 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 → RFE: 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
"RFE" is deprecated :)
severity=enhancement is enough.
Summary: RFE: 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 → 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

*** This bug has been marked as a duplicate of 45411 ***
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
The tabbed browsing code is implemented separately for Firefox, REOPENING.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: bugs → martijn.martijn
Status: REOPENED → NEW
Attachment #185887 - Flags: review?(mconnor)
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.
Depends on: 203573
Attached patch patch2 (obsolete) — Splinter Review
This also fixes the problem mentioned in comment 9.
Attachment #185887 - Attachment is obsolete: true
Attachment #241817 - Flags: review?(mano)
Attachment #185887 - Flags: review?(mconnor)
Attached patch patch2 (obsolete) — Splinter Review
Fixes stupid errors in previous patch.
Attachment #241817 - Attachment is obsolete: true
Attachment #241819 - Flags: review?(mano)
Attachment #241817 - Flags: review?(mano)
Comment on attachment 241819 [details] [diff] [review]
patch2

As discussed on IRC, it's about the right time to remove the setTimeout hack altogether.
Attachment #241819 - Flags: review?(mano) → review-
Attached patch patch3 (obsolete) — Splinter Review
Updated the patch.
Attachment #241825 - Flags: review?(mano)
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.
Attachment #241825 - Flags: review?(mano) → review-
Attached patch patch4 (obsolete) — Splinter Review
(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
Attachment #241819 - Attachment is obsolete: true
Attachment #241825 - Attachment is obsolete: true
Attachment #242064 - Flags: review?(mano)
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.
Attachment #242064 - Flags: review?(mano) → review+
Attached patch patch5Splinter Review
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?
Attachment #242076 - Flags: review?(mano)
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.
Status: NEW → RESOLVED
Closed: 16 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 alpha1
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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..
Might mean that this is profile-dependent or something?
by the way, this didn't work if the drag source was another application (tested with Thunderbird).
Dão, please file another bug for the issue you mentioned and make it block this bug. Thanks!
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?
Depends on: 356819
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
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Attachment #242064 - Attachment is obsolete: true
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.
Duplicate of this bug: 367295
No longer depends on: 203573
Version: unspecified → Trunk
Depends on: 358202
Depends on: 436717
You need to log in before you can comment on or make changes to this bug.