Currently we can only D&D stuf (boormarks/urls) either into an existing tab or onto the tabbar which creates a new last tab. It would be extremely usefull if we could D&D in between tabs so that we can create a new tab anywhere on the tabbar. (for this functionality see e.g. the tabsidebar extension http://users.blueprintit.co.uk/~dave/web/firefox/tabsidebar/index.html )

See also bug 324591.

Would this be like current GNOME (and I think Windows) implementation, where dragging something to the tab and holding it there for a second or so switches to that tab, so that you can drop it into that tab? For example, select something from one tab, drag it to the tab bar, wait, and then drop it into a textarea in another tab.

(In reply to comment #2) > Would this be like current GNOME (and I think Windows) implementation, where > dragging something to the tab and holding it there for a second or so switches > to that tab, so that you can drop it into that tab? For example, select > something from one tab, drag it to the tab bar, wait, and then drop it into a > textarea in another tab. > Grab a SeaMonkey trunk or 1.8-branch build to see how we did it with SeaMonkey.

I was already tinkering with this and didn't realize a bug was open for it (or that SeaMonkey already did it). If nobody else is already working on it in secret, I'll have a look at the SeaMonkey code and see how it differs to mine and see if I can submit a patch soon. Thanks Chris. I think that focusing a tab by hovering over it is an entirely different feature to what this bug asks for, and probably should be spun off to a different bug. So I won't touch that. I just want to get something that inserts new tabs for dnd. We should be able to use the standard dnd 'arrow' indicator for inserting between tabs. But then to be consistent, we should probably indicate when it's going to be dropped ON a tab as well (which we currently don't). We can try setting the arrow over the middle of the tab, but if that looks ugly then maybe try an outline /fake focus around the tab instead.

Here's a first-draft patch to try out. If any of you guys who can compile could please check it out, it'd be much appreciated. It should allow DnD on AND between tabs for URIs, and draw the dnd indicator arrow accordingly.

I haven't tested the rtl code, as I'm not sure how to switch my browser to rtl. Will investigate later, if nobody else has tested it.

I also haven't tested on Windows, only Mac. The Mac drag code has a few other bugs, so I expect the Windows implementation can only be better.

For dropping on to tabs, I believe I've set the indicator to draw in the exact middle of the tab. However I notice that it doesn't always seem to be centred with respect to the text (e.g. try dragging over an (Untitled) tab). I haven't investigated why, yet.

Anyway, please let me know what ya think!

(In reply to comment #5) > Created an attachment (id=232381) [edit] > Patch to play with, v.1 > > Here's a first-draft patch to try out. If any of you guys who can compile could > please check it out, it'd be much appreciated. It should allow DnD on AND > between tabs for URIs, and draw the dnd indicator arrow accordingly.

I > haven't tested the rtl code, as I'm not sure how to switch my browser to rtl. > Will investigate later, if nobody else has tested it.

I also haven't > tested on Windows, only Mac. The Mac drag code has a few other bugs, so I > expect the Windows implementation can only be better.

For dropping on > to tabs, I believe I've set the indicator to draw in the exact middle of the > tab. However I notice that it doesn't always seem to be centred with respect to > the text (e.g. try dragging over an (Untitled) tab). I haven't investigated > why, yet.

Anyway, please let me know what ya think! > Out of curiosity, why did you not just copy the SeaMonkey code? That's been reviewed, superreviewed, and tested for months. Both projects benefit when code isn't forked needlessly. Bug 324591, diff here: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=tabbrowser.xml&branch=&root=/cvsroot&subdir=mozilla/xpfe/global/resources/content/bindings&command=DIFF_FRAMESET&rev1=1.143&rev2=1.145

(In reply to comment #6)
> Out of curiosity, why did you not just copy the SeaMonkey code? That's been
> reviewed, superreviewed, and tested for months. Both projects benefit when
> code isn't forked needlessly.

Because (a) I'd already done a lot of the patch before I found this bug and the SM reference, and (b) tabbrowser.xml has already forked way beyond simply copying the patch... the fact that the SM patch has been reviewed is not very meaningful, I don't think, as an entirely different set of bugs are likely to pop up when trying to adapt it.

This version shrinks tabbrowser.xml by 37 lines while adding the functionality (mostly by condensing redundant RTL vs LTR code), and is based on the SeaMonkey code. As Wayne pointed out, the widgets are pretty forked, so it's not a direct copy.

Comment on attachment 232463 [details] [diff] [review] my version >+ ib.hidden = true; I haven't tested out the rest of the code yet, but in Firefox the indicator is generally hidden and revealed by toggling the 'dragging' property. At least by convention, if not mandatory.

(In reply to comment #9) >(From update of attachment 232463 [details] [diff] [review]) >>+ ib.hidden = true; >I haven't tested out the rest of the code yet, but in Firefox the indicator is >generally hidden and revealed by toggling the 'dragging' property. <rant> Geez, so they've even forked the "hidden" attribute/property? </rant>

Finally had a chance to play with Chris's patch tonight. I like the way you cleaned up the ltr/rtl code. Looks nice! I wasn't game to touch that. A few issues I've noticed so far: - Dragging links over icons on tabs creates the "flicker" effect for the indicator. You need to remove the |aDragSession.sourceNode.parentNode == this.mTabContainer| check from onDragExit - The indicator is drawn too far to the right (exactly one half-indicator-width) because you deleted halfIndWidth from the calculations - The rtl version probably won't work at all because of this typo in OnDragOver: + ind.styl.margineRight = I've also noticed weird behaviour when scrolling tabs while dragging (the indicator keeps changing position, widgets on the right exhibit weird stretching behaviour and the window scrollbar keeps vanishing). But drag-scrolling barely works for me even on the normal version, so I'm not sure whether your changes have actually made it any worse, or just revealed other problems.

Hope this helps... this is a modification of your patch, to address the bugs I mentioned earlier, except for two to do with scrolling:&#10;&#10;1. I would guess the indicator jumping around while scrolling is because you deleted the "put the drop indicator at the edge" bit, as well as the max and min margin safety checks. See the comment at:&#10;http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1784&#10;&#10;2. And I suspect the weird stretching I mentioned before is related to that. See the warning (which you also deleted ;) ) at: http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1773&#10;&#10;I would guess that all the max and min margin code needs to be added back, but I left it because maybe you have some other trick.&#10;&#10;Regarding the patch:&#10;I haven't tested the rtl version of the indicator with the halfIndWidth readded (or subtracted, rather). It seems to mirror the old version, but I'm not sure what else you've done there. Maybe the indicator is displaced in a different way in the rtl version.&#10;&#10;I also removed the isTabDrag variable, since it was used once only (immediately after being declared).

Sorry for all the bad formatting. Form text on my latest nightly builds seems to be playing up.

Thanks for the fixes. This is a bit less jumpy - I think some of the remaining jumps are arguably reasonable (e.g. mouse very close to the ">", and the indicator jumps between the ">" and the middle of the last tab as you scroll)... unfortunately I don't really have the time to figure out the toolkit tabbrowser well enough to write the code to get it to not jump at all. It's also pretty unreadable (nested min/max due to laziness) and is unlikely to be right for RTL... just a proof of concept for reduced-jumping. I suspect the simplest method for pegging the indicator involves checking whether the mouse is over (or beyond) the arrows, which shouldn't really be difficult (you could detect this condition and then conditionally clobber arrowX without modifying the rest of the code).

Hmm, no, the jumps during scrolling still look bad. Maybe it doesn't look as bad on platforms/machines where scrolling is faster than it is on mine. The code that was added to prevent them was done so deliberately. And I'd be unhappy for any functionality or aesthetics to be removed again, purely for the sake of increasing code symmetry with SeaMonkey. This case, at least, seems to be as simple as forcing the indicator to the far right or left, if pixelsToScroll is greater or less than 0.&#13;&#10;&#13;&#10;Only other margin weirdness of note so far is that the minimum left margin is too far right. On Mac, at least, this is most obvious when having only a few tabs open.... the indicator is inserted at the start of the tab close button.&#13;&#10;&#13;&#10;I don't have time to look tonight, but I'll see what I can do to improve these things.

Just out of curiosity I want to ask, does the tabbar scrolling code in this patch utilize or duplicate the scrolling code being developed in Bug 347363.

(In reply to comment #15) > And I'd be > unhappy for any functionality or aesthetics to be removed again, purely for the > sake of increasing code symmetry with SeaMonkey. This case, at least, seems to > be as simple as forcing the indicator to the far right or left, if > pixelsToScroll is greater or less than 0. I certainly wouldn't want to do something very silly just to match SeaMonkey code. Using pixelsToScroll to clobber arrowX is almost certainly a good way to do it - that hadn't occurred to me. (In reply to comment #16) > Just out of curiosity I want to ask, does the tabbar scrolling code in this > patch utilize or duplicate the scrolling code being developed in Bug 347363. It doesn't affect the actual scrolling functionality in any way. It just uses what's there.

(In reply to comment #17) &#13;&#10;> Using pixelsToScroll to clobber arrowX is almost certainly a good way to&#13;&#10;> do it - that hadn't occurred to me.&#13;&#10;&#13;&#10;It was how the original code did it. pixelsToScroll > 0 -> use the max margin, pixelsToScroll < 0 use the min margin.

I experimented with adding back the pixelsToScroll checks tonight, plus things to correct the fact that the max and min margins still aren't quite right.&#13;&#10;&#13;&#10;To be frank, the more checks we have to add back, the less simple and streamlined the onDragOver code becomes. Its complexity is creeping back towards the level of the current code (well, not quite that bad, but still...), only with some unfamiliar var names and things done in a different order.&#13;&#10;&#13;&#10;If I might make a couple of suggestions:&#13;&#10;1. That a new bug be opened to rewrite/streamline the Fx onDragOver code (or make it more like SeaMonkey), since it's not what this bug is intended for. Getting it to work right is only slowing down the procedure and risking introducing new bugs that have nothing to do with the intentions of this bug.&#13;&#10;2. That we check in a patch here that only adds the functionality intended for this bug, based on the current Fx tabbrowser code. For the new code, we can use the same new function name as the SM version, and anything else, where practical, to make it easier to sync the two code sets, or at least code patches for both. Apart from the func name difference, I think a lot of what was done in the original patch was quite similar to Chris's way of doing things, anyway.

Sorry, I thought that text enter bug had been fixed. Here's my post again, in readable English: I experimented with adding back the pixelsToScroll checks tonight, plus things to correct the fact that the max and min margins still aren't quite right. To be frank, the more checks we have to add back, the less simple and streamlined the onDragOver code becomes. Its complexity is creeping back towards the level of the current code (well, not quite that bad, but still...), only with some unfamiliar var names and things done in a different order. If I might make a couple of suggestions: 1. That a new bug be opened to rewrite/streamline the Fx onDragOver code (or make it more like SeaMonkey), since it's not what this bug is intended for. Getting it to work right is only slowing down the procedure and risking introducing new bugs that have nothing to do with the intentions of this bug. 2. That we check in a patch here that only adds the functionality intended for this bug, based on the current Fx tabbrowser code. For the new code, we can use the same new function name as the SM version, and anything else, where practical, to make it easier to sync the two code sets, or at least code patches for both. Apart from the func name difference, I think a lot of what was done in the original patch was quite similar to Chris's way of doing things, anyway.

I like wayne's idea of splitting this out into two bugs: one for a low risk patch that adds support for D&D between tabs and one for code cleanup - streamlining. note, it might already be too late for a low risk patch, but if we have a patch in hand, and drivers feel the risk is low and the reward is high, they might take it. (I can't promise anything.) wayne / chris, do you have cycles to work up a low risk patch that we can present to drivers before ff2 goes out the door?

This lower-risk patch is a modification of my first one. It just works in with current Firefox tabbrowser semantics and quirks and so doesn't have much bearing on the rest of the d&d code. The changes over the first patch are simply to bring a few of the additions closer to what was done to SeaMonkey. Chiefly, the name of the method that determines if the URI is being dropped on between tabs has been changed, and a few var names here and there. I haven't changed much else, though. Most of the changes would have made no sense in the context of the rest of the code staying the same. I've only tried it out on the trunk, but I can't seem to break it. I haven't tested on Windows, as I can't build that at home, so I'd appreciate someone doing that. One thing that doesn't work on Mac: if I drag a link from another application (say, Apple Mail), and use ctrl-tab to bring the browser to the front, the link will drop onto or between tabs fine, but the d&d indicator will never appear. The reason is that when dragging an external link that way, aDragSession.sourceNode is null in onDragOver. Presumably it's not null in onDrop, though, since the link drops fine. But this is probably a(nother) bug with the Mac drag code (similar to bug 345425?).

Probably not gonna make it now, but I can always ask. :)

We're past the point of any feature additions, especially of this nature, but we definitely want this for trunk/Fx3!

Reassigning. I assume Seth is still interested in this? :) I'll check for any bit rot in the patch tonight and resubmit if there's any.

> I assume Seth is still interested in this? hey wayne, sorry for the delay. yes, still interested! is the only valid patch your last one?

(In reply to comment #26) > yes, still interested! is the only valid patch your last one? Yeah, the "low risk" one is the only one that worked without breaking anything. I'm currently trying to update it to account for bit rot. There've been some changes to tabbrowser.xml that I still need to understand before I modify anything. Also, unfortunately, I build on Mac, and d&d on Mac is still a bit screwy at the moment, so it's difficult to test.

Version 4 of the patch updated for the latest tabbrowser changes (esp. bug 248612). Aside from a simple update of the code, I also added: 1. Incorporated a fix for bug 356819, which allows d&d from an external app to a backgrounded tab bar. That bug already has a patch, but it would have to be modified for this bug if it landed subsequently. If bug 356819 lands first, then I'll just modify this patch again. But if this patch lands first, I'll let them know. 2. Minor tidying... moved the max and min margin calculations to outside the rtl/ltr code, since they were the same for both. Like the SeaMonkey version, this patch assumes drop *between* tabs if the left or right quarters of the tab are hovered over, and drop *on* the tab if the mouse is over the central half. Do you think that's a fair division? Or maybe restrict the drop-between to a narrower area? Another thought... I wonder if it'd be better to have a slightly different indicator for drop-on, compared to drop between, to make it more obvious what's happening? Maybe a fatter arrow or a tab outline or something... Finally... since I deleted the |if (aDragSession.sourceNode)| check, I should decrement each line in the method by 2 spaces. I'll leave that till you're happy with the code changes, though, so as not to make a huge patch or relatively meaningless changes. Thanks :)

sorry for the bug spam, re-assigning bugs back to default owner if I'm not working actively on them.

Comment on attachment 247808 [details] [diff] [review] Patch v.4 hey wayne, sorry for the delay. can you ask mano or gavin to review this?

Thanks Seth. Flipping a coin, I picked on Gavin. Hope he doesn't mind. Here's v.5 patch. No changes other than updates to the line numbers. Gavin, could you please review this? If / when it passes review, I'll make a final change to the indents for one of the methods, but I didn't want to spam the current patch with that.

Setting the dependency since bug 390979 moves around a lot of code in the tabbed browser and I can't redo the patch here till that lands. With the code freeze approaching, I guess it's unlikely this'll make Firefox 3, either :( But can't hurt to ask.

If your cleanup patch isn't going to land for Fx3 and you won't fix this without it, I can give it a try tomorrow. Let me know.

Thanks Chris :) I have a modified patch sitting on my desktop waiting for the cleanup stuff to land (which should be in the next day or so), and any last minute modifications that are required. I believe the feature freeze for Firefox is within 2 days (is that still the case?), so either way we'll be cutting it close, but if I run into problems, I'll let ya know.

This should be all that's needed once bug 390979 lands. Posting this just in case, but I can generate a patch with proper version numbers once the 390979 patch is in.

Still really interested in this.

Updated for a couple of minor changes on bug 390979. The patch on bug 390979 has passed Gavin's review and is awaiting the unfreeze and the approval1.9 request before checkin. Despite the feature freeze, he still reckons we could get this in for Fx 3. Will this also require ui-review?

(In reply to comment #38) > Will this also require ui-review? Yes, can you go ahead and request it now?

Comment on attachment 280742 [details] [diff] [review] Patch v6b - requires bug 390979 checkin The approval flag needs to wait on review.

It should be ready for review now. Thanks! I can only build on Mac. Could someone please test on Windows and/or Linux, especially with rtl enabled to ensure the drop arrow is centred over the correct tab when dropping a link ON a tab. I've tested pretty thoroughly with DnDing external links and html files over the tab bar, as well as dragging tabs from other browser windows, but more testing is always appreciated!

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091802 Minefield/3.0a8pre ID:2007091802 + patch applied Works great, i couldn't find any errors and none in the errorconsole

This still possible to get in, Gavin? Current patch is still right to go (no bit rot).

Wayne, he'll look at it later today.

Comment on attachment 281298 [details] [diff] [review] Patch v7 - ready now bug 390979 is checked in >Index: browser/base/content/tabbrowser.xml >+ if (!isTabDrag && (newIndex = this.getDropOnIndex(aEvent)) > -1) { >+ if (this.getDropOnIndex(aEvent) != -1) { Use either != or > consistently? > <method name="onDragExit"> >- if (aDragSession.sourceNode && >- aDragSession.sourceNode.parentNode == this.mTabContainer && >- aDragSession.canDrop) { >+ if (aDragSession.canDrop) { Can you explain this change? >+ <method name="getDropOnIndex"> >+ <parameter name="aEvent"/> >+ <body> >+ <![CDATA[ >+ for (var i = aEvent.target.localName == "tab" ? aEvent.target._tPos : 0; >+ i < this.mTabs.length; i++) { I'm not sure I understand this loop. Why do you need to iterate through all tabs? Can't you just check the coordinates of event.target, if it's a tab, and return -1 otherwise?

(In reply to comment #45) > (From update of attachment 281298 [details] [diff] [review]) > >Index: browser/base/content/tabbrowser.xml > > >+ if (!isTabDrag && (newIndex = this.getDropOnIndex(aEvent)) > -1) { > > >+ if (this.getDropOnIndex(aEvent) != -1) { > > Use either != or > consistently? You're right. It'll be corrected in the next patch > > <method name="onDragExit"> > > >- if (aDragSession.sourceNode && > >- aDragSession.sourceNode.parentNode == this.mTabContainer && > >- aDragSession.canDrop) { > >+ if (aDragSession.canDrop) { > > Can you explain this change? canDrop does the sourceNode/parentNode checks, so it seemed redundant to do them twice. Do you want the checks back? > >+ <method name="getDropOnIndex"> > >+ <parameter name="aEvent"/> > >+ <body> > >+ <![CDATA[ > >+ for (var i = aEvent.target.localName == "tab" ? aEvent.target._tPos : 0; > >+ i < this.mTabs.length; i++) { > > I'm not sure I understand this loop. Why do you need to iterate through all > tabs? Can't you just check the coordinates of event.target, if it's a tab, and > return -1 otherwise? getDropOnIndex uses the same methods as getNewIndex to determine the current index of the tab. This was to retain some consistency with SeaMonkey. I'll experiment with more efficient methods, using your suggestions, though.

(In reply to comment #46) > canDrop does the sourceNode/parentNode checks, so it seemed redundant to do > them twice. Do you want the checks back? Ah, I see. That's fine, then. No need to add them back. > getDropOnIndex uses the same methods as getNewIndex to determine the current > index of the tab. This was to retain some consistency with SeaMonkey. I'll > experiment with more efficient methods, using your suggestions, though. Hmm, I'm not sure I understand the logic in getNewIndex either (could use some comments!). I guess I'll have to look into it further or ask someone who does.

(In reply to comment #47) > Hmm, I'm not sure I understand the logic in getNewIndex either (could use some > comments!). I guess I'll have to look into it further or ask someone who does. It does seem a bit weird at first, but it's not actually so horribly inefficient. It doesn't normally iterate over all tabs: if the drag is over a tab, it starts at that index. If the drag is over or before the first half of the tab, it'll return that index immediately (drop is just before the tab). If it's over the second half of the tab, then the loop will only iterate once before also returning. If the drag is outside a tab, but at the start of the tab bar, it'll only reach the first loop before returning as well. The only case in which it iterates over all tabs is if the drag is after the last tab. There's probably some way this could be done more efficiently. For getDropOnIndex, I can definitely do things more efficiently and avoid the loop altogether, as you suggested. New patch shortly.

Actually, I can't avoid the loop altogether. There are cases where the drag is on the tab bar, but below a tab (in the thin tab border strip). In those cases, the drop should be treated as if it was on the tab it's below, and to find that tab you have to iterate over the tabs to match up coordinates.

OK, that's fine.

As long as you're happy with the method, I'll modify it to first check if a tab is being hovered over, and use the quick method, but then fall back on the loop method if it isn't. I'd guess probably 90% of the time the drag will be over a tab, so this is probably worth checking. At the same time I'm happy to look at modifying getNewIndex to behave the same way, even though it isn't part of this bug.

I fixed the review comments, namely: - consistent use of != -1, rather than > -1 - avoided iterating over tabs to determine the drop point, if the drag was over the tab. Otherwise we still have to Also: - adjusted getNewIndex to also avoid iterating over tabs if possible (I tested with rtl as well and it seems to be fine, though more testers are always appreciated :) - in the course of testing this, I came across a long-standing problem with the way that links were DnDed onto the tab bar (geez, the more you look...). Basically, in this particular situation (for both drag and drop) the code assumed that the drag was always over a tab (i.e. drop point == aEvent.target). But if the drag was on the border under a tab and the drop was supposed to be ON the tab, then the dnd arrow would appear when and where it shouldn't, and the drop could either fail or drop at the end of the tab bar instead. This is fixed by replacing aEvent.target with this.mTabs[newIndex] in the relevant places. Hopefully that's it :)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100211 Minefield/3.0a9pre ID:2007100211 + V8 patch applied I found something that doesn't work right, but I don't know if it's this bug or not. Set When I open a link in a new Tab, switch to it immediately [V] (Options/tabs) Fill the tabbar go to the last tab and drag a link from that page to the left of the first tab. result: the new page opens, but the tabbar doesn't properly scroll to the new tab, it actually scrolls back a bit.

Comment on attachment 283173 [details] [diff] [review] Patch v8 - addresses review comments, and also fix for dragging links on tab bar border >Index: browser/base/content/tabbrowser.xml >+ else { >+ // We're adding (inserting or appending) a new tab >+ newIndex = this.getNewIndex(aEvent); >+ tab = this.loadOneTab(getShortcutOrURI(url), null, null, null, bgLoad, false); >+ if (newIndex != this.mTabs.length - 1) >+ this.moveTabTo(tab, newIndex); You could compare against tab._tPos here instead of this.mTabs.length - 1, right? > <method name="getNewIndex"> >+ if (aEvent.screenX < dragTarget.boxObject.screenX + dragTarget.boxObject.width / 2) >+ if (aEvent.screenX > dragTarget.boxObject.screenX + dragTarget.boxObject.width / 2) These comparisons are repeated 4 times, perhaps it would be worth factoring them out into a function that takes two arguments, like: var isLTR = window.getComputedStyle(this.parentNode, null).direction == "ltr" function eventWithinXBounds(aEvent, aTargetBoxObject) { if (isLTR) return aEvent.screenX < aTargetBoxObject.screenX + aTargetBoxObject.width / 2; return aEvent.screenX > aTargetBoxObject.screenX + aTargetBoxObject.width / 2; } This would significantly simplify getNewIndex, I think. It also avoids having to constantly retrieve the boxObject from the target node, and avoids calculating the direction more than once. It'd be nice to do it in a way that didn't require extra function calls, but the loop makes that harder... This looks good otherwise. I'll r+ the next patch.

(In reply to comment #54) > You could compare against tab._tPos here instead of this.mTabs.length - 1, > right? Yes, makes sense. Done! > These comparisons are repeated 4 times, perhaps it would be worth factoring > them out into a function that takes two arguments, like: > var isLTR = window.getComputedStyle(this.parentNode, null).direction == "ltr" > function eventWithinXBounds(aEvent, aTargetBoxObject) { Take a look what I did with the latest version of getNewIndex and see if that's OK. Since aEvent.screenX is a constant within the method, I assigned it to a var outside the function, rather than made it an arg.

Comment on attachment 283281 [details] [diff] [review] Patch v9 - addresses latest review comments Great, thanks a bunch for your patience Wayne :)

Thanks Gavin :) Is there anything I need to do to trigger the ui-r? Or do I just wait for that to happen now? (In reply to comment #53) > I found something that doesn't work right, but I don't know if it's this bug or > not. > > Set When I open a link in a new Tab, switch to it immediately [V] > (Options/tabs) > Fill the tabbar > go to the last tab and drag a link from that page to the left of the first tab. > > result: > the new page opens, but the tabbar doesn't properly scroll to the new tab, it > actually scrolls back a bit. It doesn't sound exactly like this bug, but I'll have a look and see if I can figure it out.

(In reply to comment #57) > (In reply to comment #53) > > I found something that doesn't work right, but I don't know if it's this bug or > > not. > > > > Set When I open a link in a new Tab, switch to it immediately [V] > > (Options/tabs) > > Fill the tabbar > > go to the last tab and drag a link from that page to the left of the first tab. > > > > result: > > the new page opens, but the tabbar doesn't properly scroll to the new tab, it > > actually scrolls back a bit. > > It doesn't sound exactly like this bug, but I'll have a look and see if I can > figure it out. > i filed Bug 398456 for it.

(In reply to comment #49) > Actually, I can't avoid the loop altogether. There are cases where the drag is > on the tab bar, but below a tab (in the thin tab border strip). In those cases, > the drop should be treated as if it was on the tab it's below, and to find that > tab you have to iterate over the tabs to match up coordinates. > Technically, couldn't you find the matching tab via a binary search - check whether the middle tab is before or after the coordinates, then check whether the middle tab of the remaining half of the tabs is before or after the coordinates, etc., which only takes logarithmic time as opposed to linear time in the number of tabs? While fairly simple, I admit that this isn't crucial :)

Yeah, I also considered other ways of shortening the search, such as checking if x > the last tab. But in the end I figured that the extra bloat is probably not worth the frequency of its use. But I admit I don't know much about the costs involved. If you think a binary search would be worth the change, then sure :)

... but if so, in another bug of course :) I've been trying to get this added since before Firefox 2, so I don't want to risk anything at this stage ;)

Comment on attachment 283281 [details] [diff] [review] Patch v9 - addresses latest review comments >+ <method name="getDropOnIndex"> >+ <parameter name="aEvent"/> >+ <body> >+ <![CDATA[ >+ var target = aEvent.target; >+ if (target.localName == "tab") { >+ var tabBoxObject = target.boxObject; >+ if (aEvent.screenX < tabBoxObject.screenX + tabBoxObject.width * 0.25 || >+ aEvent.screenX > tabBoxObject.screenX + tabBoxObject.width * 0.75) var whereOverTab = (aEvent.screenX - tabBoxObject.screenX) / tabBoxObject.width; if (whereOverTab < 0.25 || whereOverTab > 0.75) >+ return -1; // Not over central half of tab, so drop is between tabs >+ return target._tPos; // Drop is on tab >+ } >+ // If the drag isn't directly on a tab, check x coords against each tab in case it's >+ // in the tab bar border directly above or below. If so, treat it like it's on the tab: >+ for (var i = 0; i < this.mTabs.length; i++) { >+ var tabBoxObject = this.mTabs[i].boxObject; >+ if (aEvent.screenX > tabBoxObject.screenX + tabBoxObject.width * 0.25 && >+ aEvent.screenX < tabBoxObject.screenX + tabBoxObject.width * 0.75) same here >+ return i; // Over central half of tab, so drop is on tab >+ } >+ return -1; // Drop between tabs, or not at all >+ ]]> >+ </body> >+ </method>

(In reply to comment #60) > Yeah, I also considered other ways of shortening the search, such as checking > if x > the last tab. But in the end I figured that the extra bloat is probably > not worth the frequency of its use. But I admit I don't know much about the > costs involved. If you think a binary search would be worth the change, then > sure :) binary search is already implemented in tabbrowser.tabContainer.mTabstrip._elementFromPoint

Thanks for reviewing this, Dao. It's timely, too... I only just got my computer back after a three month downtime. So I'll get to it asap. Now to work out what's happened to cvs in the meantime...

It seems to be functionnal right now, but the UI is not responding as well. Theses bugs report defects of the GUI behavior while d&d into the tab bar : bug 498858 bug 475296 bug 423003

this really should have blocked FF3.5 but that's not realistic anymore i guess.

I don't understand; when I try to do what's asked for in comment 0, it works fine. How is this not RESO WFM? I see that it's not the best in terms of target size or feedback, but the actual bug seems fixed on a 20090902 1.9.2 nightly, meaning that this needs to morph, or spin off into a new bug about improving feedback. Please renominate with rationale as to why this needs to block the 3.6 release?

The asked behavior has been improved a lot when comparing to Firefox 3.5. In Shiretoko you have to use the tiny area of the underlying tabstrip. Only then the marker appears and let you drop the bookmark as a new tab at the given index. But I can only see it fixed on OS X and Windows. On Linux it is still hard to create a new tab at the specified position.

Comment on attachment 467398 [details] [diff] [review] patch uir=beltzner

shorlander might want to look at the theming here ...

There's no theming involved.

Comment on attachment 467398 [details] [diff] [review] patch >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > <method name="_getDragTargetTab"> >+ if (tab && >+ event.type == "drop" || event.type == "dragover" && >+ event.dataTransfer.dropEffect == "link") { This doesn't appear to be what was intended - && binds tighter than ||, so you need parentheses around the event.type checks. > if (effects == "link") { > let tab = this._getDragTargetTab(event); > if (tab) { > if (!this._dragTime) > this._dragTime = Date.now(); > if (Date.now() >= this._dragTime + this._dragOverDelay) > this.selectedItem = tab; >+ ind.collapsed = true; > return; > } > } Why this change?

parentheses added

> > if (effects == "link") { > > let tab = this._getDragTargetTab(event); > > if (tab) { > > if (!this._dragTime) > > this._dragTime = Date.now(); > > if (Date.now() >= this._dragTime + this._dragOverDelay) > > this.selectedItem = tab; > >+ ind.collapsed = true; > > return; > > } > > } > > Why this change? The indicator isn't supposed to show up when dropping a link won't open a new tab.

(In reply to comment #79) > The indicator isn't supposed to show up when dropping a link won't open a new > tab. Why not ? it makes it a lot more obvious what will happen next (open in a tab that is already in use). Right now there is no visual feedback about that at all.

(In reply to comment #80) > (In reply to comment #79) > > The indicator isn't supposed to show up when dropping a link won't open a new > > tab. > > Why not ? > it makes it a lot more obvious what will happen next (open in a tab that is > already in use). > Right now there is no visual feedback about that at all. No, it wouldn't make anything more obvious, it would just sit between tabs and mislead the user. What you're asking for is a different kind of indicator /on/ the tab, for which I'd recommend filing a new bug. Like you said, there's no such indicator right now; this bug doesn't change this.

(In reply to comment #81) > (In reply to comment #80) > > (In reply to comment #79) > > > The indicator isn't supposed to show up when dropping a link won't open a new > > > tab. > > > > Why not ? > > it makes it a lot more obvious what will happen next (open in a tab that is > > already in use). > > Right now there is no visual feedback about that at all. > > No, it wouldn't make anything more obvious, it would just sit between tabs and > mislead the user. What you're asking for is a different kind of indicator /on/ > the tab, for which I'd recommend filing a new bug. Like you said, there's no > such indicator right now; this bug doesn't change this. Bug 593094 , I'm not sure if it should depend on this bug or not.

Thanks on fixing this one Mr. Gottwald =) Works as it should on Mozilla/5.0 (Windows NT 6.0; rv:2.0b6pre) Gecko/20100903 Firefox/4.0b6pre ID:20100903040836 now

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100903 Firefox/4.0b6pre

The problem is still persistent on OS X. I have filed bug 594540.