Closed
Bug 248612
Opened 20 years ago
Closed 18 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)
Firefox
Tabbed Browser
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)
6.64 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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"
Comment 2•20 years ago
|
||
I can confirm this "inconsistent" behaviour
Comment 3•20 years ago
|
||
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
Comment 4•20 years ago
|
||
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
Comment 5•20 years ago
|
||
"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
Assignee | ||
Comment 6•20 years ago
|
||
*** This bug has been marked as a duplicate of 45411 ***
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → DUPLICATE
Comment 7•20 years ago
|
||
The tabbed browsing code is implemented separately for Firefox, REOPENING.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 8•19 years ago
|
||
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 | ||
Updated•19 years ago
|
Assignee: bugs → martijn.martijn
Status: REOPENED → NEW
Assignee | ||
Updated•19 years ago
|
Attachment #185887 -
Flags: review?(mconnor)
Assignee | ||
Comment 9•19 years ago
|
||
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
Assignee | ||
Comment 10•18 years ago
|
||
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)
Assignee | ||
Comment 11•18 years ago
|
||
Fixes stupid errors in previous patch.
Attachment #241817 -
Attachment is obsolete: true
Attachment #241819 -
Flags: review?(mano)
Attachment #241817 -
Flags: review?(mano)
Comment 12•18 years ago
|
||
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-
Comment 14•18 years ago
|
||
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-
Assignee | ||
Comment 15•18 years ago
|
||
(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 16•18 years ago
|
||
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+
Assignee | ||
Comment 17•18 years ago
|
||
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)
Comment 18•18 years ago
|
||
Comment on attachment 242076 [details] [diff] [review] patch5 r=mano, thanks!
Attachment #242076 -
Flags: review?(mano) → review+
Assignee | ||
Comment 19•18 years ago
|
||
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: 20 years ago → 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Target Milestone: --- → Firefox 3 alpha1
Assignee | ||
Comment 20•18 years ago
|
||
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..
Comment 22•18 years ago
|
||
Might mean that this is profile-dependent or something?
Comment 23•18 years ago
|
||
by the way, this didn't work if the drag source was another application (tested with Thunderbird).
Comment 24•18 years ago
|
||
Dão, please file another bug for the issue you mentioned and make it block this bug. Thanks!
Assignee | ||
Comment 25•18 years ago
|
||
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?
Assignee | ||
Comment 26•18 years ago
|
||
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: 18 years ago → 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #242064 -
Attachment is obsolete: true
Comment 27•18 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•