105885 - Reorder / Rearrange / Move tabs in the Suite's tabbed browser

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement, P1)

Description

[Grant Bowman]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:0.9.5) Gecko/20011012
BuildID:    2001101202

There is presently no way to re-arrange the tabs from left to right.  This could
either be done with drag & drop or with a (move tab left) and (move tab right)
on the context menu of the tab itself.

Reproducible: Always
Steps to Reproduce:
There presently is no way to change tab ordering.

If saved states of tabs is implemented, saving this state with other saved state
data is also needed.

Comment 1

[Lars Linek]

confused that I cannot find a dup. Marking NEW...

Comment 2

[Gilles Durys]

*** Bug 106928 has been marked as a duplicate of this bug. ***

Comment 3

[R.K.Aa.]

*** Bug 107078 has been marked as a duplicate of this bug. ***

Comment 4

[Adam Hauner]

BTW what about PC --> All, Linux --> All? On Windoze isn't this function too.

Comment 5

[Alfonso Martinez]

*** Bug 111997 has been marked as a duplicate of this bug. ***

Comment 6

[Alfonso Martinez]

-->All, All

Comment 7

[Niklas Mehner]

*** Bug 112946 has been marked as a duplicate of this bug. ***

Comment 8

[Jesse Ruderman]

The Windows taskbar doesn't let you rearrange buttons either...

Comment 9

[R.K.Aa.]

*** Bug 113734 has been marked as a duplicate of this bug. ***

Comment 10

[sairuh (rarely reading bugmail)]

ooh, this would be tres useful. however, i don't know if this is even part of
the machv UI requirements for this feature. marlon, trudelle, is it?

Comment 11

[Peter Trudelle]

Yes, Opera has this and it is very nice, especially combined with our single
close box.  However, since we have *no* requirements for this feature, this
cannot be one.  leaving at 1.0.1/helpwanted.

Comment 12

[David Hyatt]

Reassigning to new component owner.

Comment 13

[jag (Peter Annema)]

-> 1.1

Comment 14

[xyzzy]

*** Bug 123665 has been marked as a duplicate of this bug. ***

Comment 15

[Peter Trudelle]

nsbeta1- per ADT triage team, ->future

Comment 16

[Alfonso Martinez]

*** Bug 128860 has been marked as a duplicate of this bug. ***

Comment 17

[Boris Zbarsky [:bzbarsky]]

*** Bug 142051 has been marked as a duplicate of this bug. ***

Comment 18

[Boris Zbarsky [:bzbarsky]]

*** Bug 148514 has been marked as a duplicate of this bug. ***

Comment 19

[R.K.Aa.]

*** Bug 156069 has been marked as a duplicate of this bug. ***

Comment 20

[sairuh (rarely reading bugmail)]

nominating --if cycles are available to implement this for buffy...

Comment 21

[Gashu]

FYI: this addon has ability to reorder/rearrange tabs with drag'n'drop.
http://www.cc-net.or.jp/~piro/xul/_tabextensions.html

Comment 22

[Mike Neman]

OOOOooohhhhh, I _found_ it!
Here's My vote!
I wanna cycle my tabs too! I hate it when I M-click to open a new tab and when I
close it it reverts NOT to the one I came from but to the next-leftmost
(adjacent) one (which 9-out-of-10 times is an unrelated tab). More needless
mouse draggin' and not _even_ ALT+left/right-keypad functionality like Opera!! :(

Comment 23

[Daniel Clemente]

It would be nice if "drag&drop tabs" worked, but before that feature is
finished, we can use "Ctrl+Shift+PgUp" and "Ctrl+Shift+PgDwn" or something
similar to move tabs left and right. (So "Ctrl+PgUp" and "Ctrl+PgDown" will move
among tabs, and the same keys with Shift will arrange them).

Adding my vote to this bug and CC'ing me...

Comment 24

[Paul Wyskoczka]

nsbeta1+/adt3 per the nav triage team.

Comment 25

[R.K.Aa.]

*** Bug 180553 has been marked as a duplicate of this bug. ***

Comment 26

[HJ]

http://multizilla.mozdev.org in case you don't want to wait :-)

Comment 27

[Mac]

It would also be really nice to be able to do other types of rearranging with
tabs. For instance, I would really like to be able to move a tab into a new or
existing window (easily [not a URL cut/paste into a new window], and without
reloading the page). A drag-and-drop interface for this would be really nice,
along with an option on the tab's context menu to make a new window with it.

Once we have a way to rearrange tabs, it would also be nice to be able to select
multiple tabs at the same time for rearranging, so you could grab a set of
related (and perhaps non-adjacent) tabs, and drag them all to one place in the
tab order, or into a new window. For the UI, perhaps Shift-Click on another tab
adds it to the set of selected tabs, but leaves the current one on front, and
Ctrl-Click on a tab adds it to the selected set and makes it the new front tab.
Shift-Click and Ctrl-Click could also unselect tabs that are currently selected
(except for the one on front).

Comment 28

[guanxi]

Mac - see bug 132674 re selecting multiple and/or non-adjacent tabs

Comment 29

[Rafael Ebron (:rebron)]

nsbeta1- per the nav triage team.

Comment 30

[Benedikt Kantus]

*** Bug 191257 has been marked as a duplicate of this bug. ***

Comment 31

[José Jeria]

*** Bug 191278 has been marked as a duplicate of this bug. ***

Comment 32

[Andrew Hagen]

*** Bug 160720 has been marked as a duplicate of this bug. ***

Comment 33

[Sébastien Delahaye]

*** Bug 193044 has been marked as a duplicate of this bug. ***

Comment 34

[Josh Sowin]

I didn't see this before I filled a dupe.  Guess I didn't search using the right
terminology.  I vote for this too!! ;)

Comment 35

[Alfonso Martinez]

*** Bug 194913 has been marked as a duplicate of this bug. ***

Comment 36

[R.K.Aa.]

*** Bug 198805 has been marked as a duplicate of this bug. ***

Comment 37

[Jo Hermans]

*** Bug 204941 has been marked as a duplicate of this bug. ***

Comment 38

[Waheed Islam]

so many dupes,
and 48 votes.

any plan on implementing this?

Comment 39

[Jo Hermans]

*** Bug 212274 has been marked as a duplicate of this bug. ***

Comment 40

[Jo Hermans]

*** Bug 214261 has been marked as a duplicate of this bug. ***

Comment 41

[Matthias Versen [:Matti]]

*** Bug 221136 has been marked as a duplicate of this bug. ***

Comment 42

[Thib]

63 votes already, and many duplicates. :-) I'm voting for this too.

I've clicked on "show dependency graph" but couldn't get a response from the
server at this point. I'll try again later... but I just wanted to check if Bug
#194913 (http://bugzilla.mozilla.org/show_bug.cgi?id=194913) had been linked to
this one? It's a Chatzilla bug/RFE about reordering the view tabs in Chatzilla.
If a generic tab re-ordering mechanism is implemented through this bug, they say
they will use it for Chatzilla, otherwise they say they'll have to do it themselves.

Comment 43

[Lenny Domnitser]

This feature is especially useful when closing tabs. Since whenever a tab is
switched it falls to the one directly to its right (or directly to its left it
it is the right-most tab) I would like to be able to put a tab in the fallback
position.

Comment 44

[Lenny Domnitser]

er... that would be "closed" instead of "switched" in the pevious comment.

Comment 45

[Oleg Sidletskiy]

*** Bug 232282 has been marked as a duplicate of this bug. ***

Comment 46

[John]

comment 27,
the tabs<==>windows transfer is a different issue. It's bug 102132.

Comment 47

[John]

Oh, and if you really want this, like HJ said, just download multizilla. It
installs automatically (you have to restart mozilla). It adds a lot of cool
features like rearranging tabs, the ability to move the tab bar to the bottom,
moving tabs to new (or existing) windows, undeleting tabs (ctrl-z), but mainly
just presents me with the powerful tools I use in an accessible way. I'm finding
all those features I always wanted in multizilla, there are so many prefs it's
hard to absorb it all (read: multizilla has more UI prefs than navigator, many
more) (why bother reporting bugs anymore?). Only problem is, it slows down my
computer (celery 300, 32mb :() but for most users this should be a great
solution. And if this bug is ever to be fixed, multizilla code might be usable.
P.S. if you want to flame me for my comment about not reporting bugs, do so at
my email address so you don't bother all the people actually working on this
bug, and I'll actually reply if you want me to.
Bye bye for now bugzilla, I have all the enhancements I need.

Comment 48

[Isaac Hwak Han]

*** Bug 232865 has been marked as a duplicate of this bug. ***

Comment 49

[John]

pulled vote because for me this bug is fixed

Comment 50

[alanjstr]

John-

Stop spamming bugs just because you have found the funtionality in an extension.  

Comment 51

[Isaac Hwak Han]

*** Bug 232952 has been marked as a duplicate of this bug. ***

Comment 52

[guanxi]

Changing Summary to make bug easier to find (inspired by Summary of duped bug):

was:  reorder/rearrange tabs in tabbed browser
now:  Reorder / Rearrange / Move tabs in tabbed browser

Comment 53

[John]

Sorry to clutter the bug, an email to "alanjstr" bounced. If he still reads this
bug, I ask that he follow the url:
http://www.geocities.com/johnathlon/alanjstr.txt

Comment 54

[Stefan Borggraefe]

*** Bug 241365 has been marked as a duplicate of this bug. ***

Comment 55

[ow]

First requested in 2001 ... no hope for this one, huh? Well, voted anyway.

Comment 56

[Victor]

Yeah... almost pointless, it seems, but I voted anyway.

Comment 57

[Az]

For those who are as frustrated with this lack of functionality as i am there is
an extension (thanks asa's blog).  The url is:

http://update.mozilla.org/extensions/moreinfo.php?id=176&vid=335&category=Tabbed%20Browsing

Perhaps the author could be encouraged to submit a patch for this bug or some
other willing soul could use his code (if allowed) to do one themselves.

Comment 58

[Jason Bassford]

That extension only works with Firefox - this bug has been filed under Mozilla
(Seamonkey).

Comment 59

[alanjstr]

Does multizilla allow tab reordering?

Comment 60

[Jens Bannmann]

(In reply to comment #59)
> Does multizilla allow tab reordering?

It does. However, extracting only this functionality from Multizilla might be
difficult, as it is IIRC a complete replacement of the tabbrowser code.

It might be easier to start from the Firefox extension "MiniT (drag+indicator)"
mentioned above, however this extension seems to have serious problems on Mac OS X.

Comment 61

[Klaus Kusche]

One more vote, especially for keyboard shortcuts for moving a tab left / right!
Even konqueror offers this for quite some time now!

Comment 62

[James Norris]

I don't ever remember voting for this bug, but it's there in my list... However,
I also have bug 179656 is that a dupe of this??

Comment 63

[Serge Gautherie (:sgautherie)]

(In reply to comment #62)
> I also have bug 179656 is that a dupe of this??

No: it's said there that it's FireFox, while here is MozillaAS.

Comment 64

[James Norris]

(In reply to comment #63)
> 
> No: it's said there that it's FireFox, while here is MozillaAS.

OK, but surely if it's implemented into the core, it will be available to ff also?

Comment 65

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #64)
> (In reply to comment #63)
> > 
> > No: it's said there that it's FireFox, while here is MozillaAS.
> 
> OK, but surely if it's implemented into the core, it will be available to ff also?

Given that xpfe and toolkit have forked their tabbrowsers, and may have forked
tabs, any fix would likely be slightly different between the two products.

Comment 66

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: provisional patch (does not work)

This patch will not work until removeChild() on a <browser> node does not do
whatever it does now:

<CTho> bz: but when i move the <browser> associated with the tag, it ends up
white and blank
<bz> CTho: ah
<bz> Ctho: you remove the <browser> from the DOM
<bz> CTho: "we don't support that"
<bz> CTho: yet
<bz> CTho: removing it from the DOM tears down its docshell

In the mean time, I'm looking at an alternate way of doing this to work around
the problem.  I'm attaching this patch in case I give up, to give the next
person a starting point ;)

Comment 67

[Jan Varga [:janv]]

I think it would be much easier to play with ordinals just like we do in tree.xml

Comment 68

[Corinne Mayans]

Here's another vote for this feature. I like using tabs, but I often end up
closing and reopening them to get them in the order that I want. 

Comment 69

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #67)
> I think it would be much easier to play with ordinals just like we do in tree.xml

Yeah, that's what I ended up doing.  The patch basically a port of the
miniT-drag extension now.  I gave the patch to Neil and a few people to look at
before I attach it to request review.  At this rate, it's unlikely to make 1.8b1
:(.  Hopefully it's accepted for 1.8b2.

Comment 70

[HJ]

What is this "miniT-drag extension" and where do I get it?

Comment 71

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #70)
> What is this "miniT-drag extension" and where do I get it?

It's firefox-only, http://dorando.emuverse.com/projects/mozilla/ or
http://www.v2studio.com/k/moz/ - google also gave more URLs including
update.mozilla.org.

(In reply to comment #67)
> I think it would be much easier to play with ordinals just like we do in tree.xml
I don't think it's easier or cleaner, it just happens to actually work.

Comment 72

[Jan Varga [:janv]]

> I don't think it's easier or cleaner, it just happens to actually work.

IMHO, it's cleaner to reorder the tabs and tabpanels at presentation layer
(using ordinals) than reordering real content (DOM).

Comment 73

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #72)
> > I don't think it's easier or cleaner, it just happens to actually work.
> 
> IMHO, it's cleaner to reorder the tabs and tabpanels at presentation layer
> (using ordinals) than reordering real content (DOM).

I have a patch that handles ordinals and drag'n'drop entirely in tabbrowser.xml,
and it's not particularly clean.  However, I added support for ordinals to
tabbox.xml per Neil's suggestion, and it did turn out to be a lot cleaner.  I've
asked Neil to take another look at it, and if it passes a "first glance" check,
I'll attach it and see about reviews.

Comment 74

[HJ]

Ordinals are the way to go, but I don't see as to how adding the same code to
tabbox.xml should be any cleaner, and you will also have to add checks to
prevent DND for non tabbrowser tabs, because I'm sure that it will brake some
mozilla code and other extensions.

Comment 75

[Jan Varga [:janv]]

(In reply to comment #73)
> However, I added support for ordinals to
> tabbox.xml per Neil's suggestion, and it did turn out to be a lot cleaner.

even better

Comment 76

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 77

[Ian Neal]

Just a note, (as Chris mentioned to me on irc) to fully test this patch I
believe you need a build with the fix to Bug 282481 in it, otherwise ctrl+page
up on the first tab sends the script into a loop.

Comment 78

[YUKI "Piro" Hiroshi]

The patch seems based on the implementation of old Tabbrowser Extensions,
but we shoudn't apply it.
Because, this method may cause fatal problems like crashing.

I think that the implementation of tablib is better than TbE.
http://mozilla.dorando.at/readme.html
(New versions of TbE also be based on it.)

Comment 79

[[e-mail]]

Comment on attachment 174710 [details] [diff] [review]
patch

Provisional review:

>+          while (next != startTab && (!next || next.localName != "tab")) {
>+            if (next) {
>+              next = next.boxObject[aDir == -1 ? "previousSibling" : "nextSibling"];
>+	    }
Spaces, not tabs, please (check all your changes).

>+	      next = startTab.boxObject.parentBox.boxObject.lastChild.boxObject.previousSibling;
Why the extra boxObject.previousSibling?

>+            for(var i = position+1; i < this.mTabs.length; i++)
This will never happen because position = .length - 1 above.

>+            // If it was to the left, start renumbering back one, otherwise start here
>+            newIndex = ((this.mCurrentTab.ordinal-0 > oldTab.ordinal-0 || this.mCurrentTab.ordinal == l-1)
>+                         ? this.mCurrentTab.ordinal-1 : this.mCurrentTab.ordinal);
You shouldn't recalculate the new index, you should calculate it correctly
originally.

>+              if (aEvent.clientX < this.mTabs[0].boxObject.x) newIndex = 0;
This won't work in rtl locales.

>+              else newIndex = (aEvent.clientX < aEvent.target.boxObject.x + aEvent.target.boxObject.width/2)
>+                               ? aEvent.target.ordinal-0 : aEvent.target.ordinal-0+1;
Nor will this.

>+          document.getAnonymousElementByAttribute(this.mTabContainer, "class", "tabs-left").ordinal = 0;
Please set this ordinal in the tabbox binding (also renaming the binding so it
doesn't clash with toolkit would be nice).

>+          document.getAnonymousElementByAttribute(this.mTabContainer, "class", "tabs-right").ordinal = -1;
Ugh :-( Please use a large number as per tree.xml

>+          for(var i = 0; i < this.mTabContainer.childNodes.length; i++)
This should be exactly one at this point of time.

>+              this.mTabContainer.childNodes[i].ordinal=i;
So you can set this explicitly on the tab.

>+              this.mTabs[i] = this.mTabContainer.childNodes[i];
And initialize your array with [this.mCurrentTab] (if you want to do that in
the field rather than the constructor you'll have to fix the mCurrentTab field
too).

>+          this.mCurrentTab.selected = true;
Isn't it already?

Comment 80

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: mostly-fixed patch

(In reply to comment #79)
> >+		  if (aEvent.clientX < this.mTabs[0].boxObject.x) newIndex = 0;

> This won't work in rtl locales.
> 
> >+		  else newIndex = (aEvent.clientX < aEvent.target.boxObject.x +
aEvent.target.boxObject.width/2)
> >+				   ? aEvent.target.ordinal-0 :
aEvent.target.ordinal-0+1;
> Nor will this.

Mano said he'd help out with the RTL code (I wasn't having much luck).	If it
isn't obvious, the "problem code" is:
+	       else {
+		 //Mano's code here
+	       }

Comment 81

[Worcester12345]

Is this a duplicate of bug 179656?

They seem to be about the same thing.

Comment 82

[Jason Bassford]

No.  That bug is targetted against Firefox specifically.  (Although, I suppose,
a dependency could be created.)

Comment 83

[Dorando]

Using ordinals on <tab>s together with Flowing Tabs or the CSS on which it is
based ( http://forums.mozillazine.org/viewtopic.php?t=68504 ) will cause a
startup crash. Unless you can fix this, I recommend that you use linkedPanel
like miniT/tablib has done since 20040920 (without the compatibility wrappers of
course).

You should make "Home Page -> Use Current Group" and "Bookmark This Group of
Tabs..." compatible with reordered tabs...

This newIndex calculation will not only fail for RTL locales (tree.xml has this
problem, too) but it will have problems for themes with spaces between the
<tab>s. Also it would be nice if you could move this calculation into a separate
function so it can be used by an extension (for adding a drop indicator for
example).

Comment 84

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #83)
> Using ordinals on <tab>s together with Flowing Tabs or the CSS on which it is
> based ( http://forums.mozillazine.org/viewtopic.php?t=68504 ) will cause a
> startup crash. Unless you can fix this, I recommend that you use linkedPanel
> like miniT/tablib has done since 20040920 (without the compatibility wrappers of
> course).
Is there a bug filed for this crash?  If not, can you file one with information
on how to reproduce it and a stack?  I'd rather not hack around a crash if
fixing the bug causing the crash is an option.

> You should make "Home Page -> Use Current Group" and "Bookmark This Group of
> Tabs..." compatible with reordered tabs...
Hmm, you're right - I'll have to fix that.

> This newIndex calculation will not only fail for RTL locales (tree.xml has this
> problem, too)
Mano is handling the RTL code.

> but it will have problems for themes with spaces between the
> <tab>s.
Is there an easy way to test that?  Does the new version of the extension have
useful code?

> Also it would be nice if you could move this calculation into a separate
> function so it can be used by an extension (for adding a drop indicator for
> example).
That sounds like a reasonable idea.

Comment 85

[R.K.Aa.]

*** Bug 285760 has been marked as a duplicate of this bug. ***

Comment 86

[Dorando]

(In reply to comment #84)
> (In reply to comment #83)
> > Using ordinals on <tab>s together with Flowing Tabs [...] will cause a
> > startup crash.
> Is there a bug filed for this crash?
I think this is bug 170674 , I crash at the same line ( TB4290717E )

> > but it will have problems for themes with spaces between the
> > <tab>s.
> Is there an easy way to test that?
tab { margin: 0px 20px !important }

> Does the new version of the extension have useful code?
Yes, in .getNewIndex().

Comment 87

[Steve England [:stevee]]

*** Bug 287468 has been marked as a duplicate of this bug. ***

Comment 88

[:Gavin Sharp [email: [e-mail]]]

Bug 179656 covers the Firefox port of this bug's work, and is marked blocking
aviary1.1 already.

Comment 89

[Steve England [:stevee]]

*** Bug 288205 has been marked as a duplicate of this bug. ***

Comment 90

[Worcester12345]

Wait, wasn't this marked as blocking 1.1? How come it is blank now? 

Comment 91

[Ed Lee :Mardak]

This bug blocks 179656 which is marked blocking-aviary1.1

From comment #88

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flag|blocking-aviary1.1?         |

------- Additional Comments From [e-mail]  2005-03-25 12:56 PST -------
Bug 179656 covers the Firefox port of this bug's work, and is marked blocking
aviary1.1 already.

Comment 92

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #90)
> Wait, wasn't this marked as blocking 1.1? How come it is blank now? 

Read comment 88.

Comment 93

[Worcester12345]

(In reply to comment #92)
> (In reply to comment #90)
> > Wait, wasn't this marked as blocking 1.1? How come it is blank now? 
> 
> Read comment 88.

Aha! Thanks. I was looking for the - which should be in the blocking field.

Comment 94

[Aaron P]

Since bug 179656 is now fixed on the trunk, shouldn't this bug be closed, or
duped to that bug?

Comment 95

[Jason Bassford]

Yes - if it *were* fixed on the trunk.  It isn't.  Bug 179656 only applied to
Firefox.  The suite doesn't have that functionality yet.  That's this bug.

Comment 96

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: port of what firefox uses

Comment 97

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: port of what firefox uses

Comment 98

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 187979 [details] [diff] [review]
port of what firefox uses

Bugzilla timed out while I was attaching the patch.

Comment 99

[mv_van_rantwijk]

Removing <method name="getBrowserForTab"> will break a lot of add-ons, so it
should not be removed.

Comment 100

[Daniel Brooks [:db48x]]

Comment on attachment 187978 [details] [diff] [review]
port of what firefox uses

I'd say this patch is only OK. Here are my recommendations:

>+#ifdef XP_MACOSX
>+          if ('metaKey' in aEvent && aEvent.metaKey) {
>+#else
>+          if ('ctrlKey' in aEvent && aEvent.ctrlKey) {
>+            if (aEvent.keyCode == KeyEvent.DOM_VK_F4 && 
>+                this.tabbrowser.mTabBox.handleCtrlPageUpDown) {
>+              this.tabbrowser.removeCurrentTab();
>+              return;
>+            }
>+#endif
>+            if (aEvent.target.localName == "tabbrowser") {
>+              switch (aEvent.keyCode) {
>+                case KeyEvent.DOM_VK_UP:
…
>+                case KeyEvent.DOM_VK_END:
>+                  this.tabbrowser.moveTabToEnd();

Wouldn't it be easier to use <key> (and perhaps <command>, if you want it to be
possible to use these actions from a context menu) elements instead of all this
code? You'd be able to get rid of the ifdef here because the mac keys are
already defined sepereately.

>+                else if (this.mBrowser.contentDocument instanceof ImageDocument) {
>+                  this.mTab.setAttribute("image", this.mBrowser.currentURI.spec);
>+                }

Isn't this part of a different patch?

>-      <method name="getBrowserForTab">
>-        <parameter name="aTab"/>
>-        <body>
…
>-        </body>
>-      </method>

Yes, as mv_van_rantwijk notes, this isn't such a good idea. Replace the body
with a simple 'return aTab.linkedBrowser;' so that extensions and so on keep
working.

>+              
>+              if (window.getComputedStyle(this.parentNode, null).direction == "ltr") {
>+                if (newIndex == this.mTabs.length) {
>+                  ind.style.marginLeft = this.mTabs[newIndex-1].boxObject.x + 
>+                                         this.mTabs[newIndex-1].boxObject.width - this.boxObject.x - 7 + 'px';
>+                } else {
>+                  ind.style.marginLeft = this.mTabs[newIndex].boxObject.x - this.boxObject.x - 7 + 'px';
>+                }
>+              } else {
>+                if (newIndex == gBrowser.mTabs.length) {
>+                  ind.style.marginRight = gBrowser.boxObject.width + gBrowser.boxObject.x -
>+                                          gBrowser.mTabs[newIndex-1].boxObject.x + 'px';
>+              } else {
>+                  ind.style.marginRight = gBrowser.boxObject.width + gBrowser.boxObject.x -
>+                                          gBrowser.mTabs[newIndex].boxObject.x -
>+                                          gBrowser.mTabs[newIndex].boxObject.width + 'px';
>+                }
>+              }

I don't particularly like all this adding and subtracting of widths and
coordinates, but I suppose if it's really necessary…

At the very least you could put gBrowser.mTabs[newIndex] and
gBrowser.mTabs[newIndex-1] into variables and make the code a little less
verbose.


>+            if (window.getComputedStyle(this.parentNode, null).direction == "ltr") {
>+              for (i = aEvent.target.localName == "tab" ? aEvent.target._tPos : 0; i < this.mTabs.length; i++)
>+                if (aEvent.clientX < this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) 
>+                  return i;
>+            } else {
>+               for (i = aEvent.target.localName == "tab" ? aEvent.target._tPos : 0; i < this.mTabs.length; i++)
>+                if (aEvent.clientX > this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) 
>+                  return i;
>+            }

You could put the loop outside the if, since the loop conditions are the same
for either ltr or rtl. 

>+      <method name="moveTabOver">
>+        <parameter name="aEvent"/>
>+        <body>
>+          <![CDATA[
>+            var direction = window.getComputedStyle(this.parentNode, null).direction;
>+            if ((direction == "ltr" && aEvent.keyCode == KeyEvent.DOM_VK_RIGHT) ||
>+                (direction == "rtl" && aEvent.keyCode == KeyEvent.DOM_VK_LEFT))
>+              this.moveTabForward();
>+            else
>+              this.moveTabBackward();
>+          ]]>
>+        </body>
>+      </method>

Same recommendation as before.

I'll do a build with this patch in a bit so I can test it.

Comment 101

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v2

Comment 102

[Ian Neal]

Comment on attachment 188647 [details] [diff] [review]
patch v2

>Index: xpfe/global/resources/content/bindings/tabbrowser.xml
>===================================================================

>@@ -861,19 +915,27 @@
>             this.mStrip.collapsed = false;
> 
>             this.mPrefs.setBoolPref("browser.tabs.forceHide", false);
> 
>             // wire up a progress listener for the new browser object.
>-            var position = this.mTabContainer.childNodes.length-1;
>+            var position = this.mTabs.length-1;
Nit: spaces before and after the -

>@@ -995,11 +1047,11 @@
> 
>             // We are no longer the primary content area.
>             oldBrowser.setAttribute("type", "content");
> 
>             // Now select the new tab before nuking the old one.
>-            var currentIndex = this.mPanelContainer.selectedIndex;
>+            var currentIndex = this.mTabContainer.selectedIndex;
> 
>             var newIndex = -1;
>             if (currentIndex > index)
>               newIndex = currentIndex-1;
Nit: spaces before and after the - (though you are not changing this line so
less of a nit).

>@@ -1130,77 +1178,264 @@
.
.
.
>+              var ib = document.getElementById('tab-drop-indicator-bar');
>+              var ind = document.getElementById('tab-drop-indicator');
>+              ib.setAttribute('dragging','true');
>+              
>+              var tabAtPrevIndex = this.mTabs[newIndex-1];
Nit: spaces before and after the -

>Index: themes/classic/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/themes/classic/jar.mn,v
>retrieving revision 1.134
>diff -u -p -5 -r1.134 jar.mn
>--- themes/classic/jar.mn	13 May 2005 21:23:10 -0000	1.134
>+++ themes/classic/jar.mn	8 Jul 2005 07:51:46 -0000
>@@ -283,10 +283,11 @@ classic.jar:
>   skin/classic/global/menu/menu-arrow.gif                     (global/win/menu/menu-arrow.gif)
>   skin/classic/global/menu/menu-radio.gif                     (global/win/menu/menu-radio.gif)
>   skin/classic/global/menu/menu-radio-disabled.gif            (global/win/menu/menu-radio-disabled.gif)
>   skin/classic/global/menu/menu-radio-hover.gif               (global/win/menu/menu-radio-hover.gif)
>   skin/classic/global/scrollbar/slider.gif                    (global/win/scrollbar/slider.gif)
>+  skin/classic/global/tabDragDrop/tabDragIndicator.png        (global/tabDragDrop/tabDragIndicator.png)
This is in the middle of #ifdef so wouldn't be added for OSX, this probably
needs to be moved 40 or so lines further on by the tree or toolbar GIFs.

Also you won't you need changes for modern theme as well as classic?

Comment 103

[Daniel Brooks [:db48x]]

Comment on attachment 188647 [details] [diff] [review]
patch v2

r=db48x, just make sure you address Ian's comments  on the theme stuff

Comment 104

[Robert Kaiser]

I don't believe we're holding SeaMonkey 1.0 Alpha for a feature, even if it's a
nice one to have. This does not mean it can't land for Alpha, given it gets the
needed sr/a in time - I'm sure those tab users out there would like to have it ;-)

Comment 105

[Worcester12345]

Can this get a review so it gets in? Rather ask than mark to block 1.0b. Thanks.

Comment 106

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: unbitrotted, bugfixed

carrying forward r=db48x

Comment 107

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Some notes:
1. the dragdropsecurity method check can be removed.  s/this./nsDragAndDrop./ for the calls to it
2. remove the ifdef, set arrowkeysshouldwrap to null by default and change it in the constructor

Comment 108

[[e-mail]]

Comment on attachment 199714 [details] [diff] [review]
unbitrotted, bugfixed

Note: review of the actual drag drop code to follow!

>+        <xul:hbox id="tab-drop-indicator-bar">
>+          <xul:hbox id="tab-drop-indicator"/>
>+        </xul:hbox>
>+        <xul:hbox class="tabbrowser-strip chromeclass-toolbar" collapsed="true" tooltip="_child" context="_child"
>+                  anonid="strip" selectedIndex="0"
1. You're mixing ids and anonids.
2. Why isn't the tab drop indicator an image?
3. You should use hidden="true" to hide the indicator.
4. Might it be possible to make the strip a stack to position the indicator?
(position: relative doesn't strike me as very XUL-friendly but check with bz)

>+                  ondraggesture="nsDragAndDrop.startDrag(event, this.parentNode.parentNode); event.stopPropagation();"
>+                  ondragover="nsDragAndDrop.dragOver(event, this.parentNode.parentNode); event.stopPropagation();"
>+                  ondragdrop="nsDragAndDrop.drop(event, this.parentNode.parentNode); event.stopPropagation();"
>+                  ondragexit="nsDragAndDrop.dragExit(event, this.parentNode.parentNode); event.stopPropagation();">
Are we now doing drag and drop over the whole tabbrowser? If so, it might be better to convert these to <handler>s.

>+                    anonid="tabcontainer" selectexTabIndex="0"
selectexTabIndex?

>-      <field name="mTabBox">
>+      <field name="mTabBox" readonly="true">
>         document.getAnonymousNodes(this)[1]
>       </field>
>-      <field name="mStrip">
>-        this.mTabBox.firstChild
>+      <field name="mStrip" readonly="true">
>+        document.getAnonymousElementByAttribute(this, "anonid", "strip");
>+      </field>
>+      <field name="mTabContainer" readonly="true">
>+        document.getAnonymousElementByAttribute(this, "anonid", "tabcontainer");
>       </field>
>-      <field name="mTabContainer">
>-        this.mStrip.childNodes[2]
>+      <field name="mPanelContainer" readonly="true">
>+        document.getAnonymousElementByAttribute(this, "anonid", "panelcontainer");
>       </field>
You're mixing getAnonymousNodes and getAnonymousElementByAttribute.

>+#ifdef XP_MACOSX
:-P

>+      <method name="dragDropSecurityCheck">
Remove this as mentioned on IRC.

>       <method name="updateCurrentBrowser">
>         <body>
>           <![CDATA[
>-            var newBrowser = this.mPanelContainer.childNodes[this.mPanelContainer.selectedIndex];
>+            var newBrowser = this.selectedTab.linkedBrowser;
Why doesn't the old code work? Or try this.mPanelContainer.selectedPanel instead.

>-                var listener = this.mTabListeners[this.mPanelContainer.selectedIndex];
>+                var listener = this.mTabListeners[this.mTabContainer.selectedIndex];
How easy would it be to make the listeners (and filters) match up with the panel index, rather than the tab index?

>+            var uniqueId = "panel" + Date.now() + position;
The unique ID generator is unnecessarily complicated. Just use "panel" plus an incrementing counter.

>+            this.mPanelContainer.lastChild.id = uniqueId;
b.id?

>+            t.linkedBrowser = b;
I'm not keen on using this to link the tab and browser, but I suppose anything else is too inconvenient.

>+            t._tPos = position;
You never seem to usefully use this.
e.g. this.mCurrentTab._tPos becomes this.mTabContainer.selectedIndex
also change moveTabTo to take two tab indexes rather than a tab and an index.

>+            this.mCurrentTab.selected = true;
>             this.mTabContainer.selectedIndex = newIndex;
>+            this.mTabBox.selectedPanel = this.selectedTab.linkedBrowser;
> 
>-            // When removing a tab to the left of the current tab
>-            // fix up the panel index without firing any events
>-            this.mPanelContainer.selectedIndex = newIndex;
>+            this.updateCurrentBrowser();
Why are you setting this.mCurrentTab.selected?
That's probably what's forcing you to update the current browser.

>+            //Firefox does "this.updateCurrentBrowser()" and "oldBrowser.focused(Window|Element) = null"
We do the latter in destroy() which is the right place for it :-P

>+            var i;
>+            browsers.item = function(i) {return this[i];}
>+            for (i = 0; i < this.mTabs.length; i++)
Not: for (var i = 0;

>-          this.mCurrentBrowser = this.mPanelContainer.firstChild;
>+          this.mCurrentBrowser = this.mPanelContainer.childNodes[0];
Why this change?

>+          this.mPanelContainer.childNodes[0].id = uniqueId;
Using the new uniqueId scheme can set this id in the XUL?

>Index: themes/classic/jar.mn
What about Modern changes?

>+  skin/classic/global/tabDragDrop/tabDragIndicator.png        (global/tabDragDrop/tabDragIndicator.png)
Remind me to check the image for 256-colour safety.

> #endif
It looks as if you added this image inside an #else XP_MACOSX block by mistake?

>+    <!-- Tab reordering keys -->
>+    <key id="key_moveTabLeft" keycode="VK_LEFT" modifiers="accel" command="cmd_moveTabLeft"/>
>+    <key id="key_moveTabRight" keycode="VK_RIGHT" modifiers="accel" command="cmd_moveTabRight"/>
>+    <key id="key_moveTabBackward" keycode="VK_UP" modifiers="accel" command="cmd_moveTabBackward"/>
>+    <key id="key_moveTabForward" keycode="VK_DOWN" modifiers="accel" command="cmd_moveTabForward"/>
>+    <key id="key_moveTabToStart" keycode="VK_HOME" modifiers="accel" command="cmd_moveTabToStart"/>
>+    <key id="key_moveTabToEnd" keycode="VK_END" modifiers="accel" command="cmd_moveTabToEnd"/>
Might as well call the functions directly rather than via commands.

Comment 109

[[e-mail]]

Comment on attachment 199714 [details] [diff] [review]
unbitrotted, bugfixed

>+          if (aEvent.target.localName == "tab") {
>+            aXferData.data = new TransferData();
>+            aXferData.data.addDataForFlavour("text/x-moz-tab", aEvent.target._tPos);
>+            
>+            var URI = aEvent.target.linkedBrowser.currentURI;
>+            if (URI) {
>+              aXferData.data.addDataForFlavour("text/unicode", URI.spec);
>+              aXferData.data.addDataForFlavour("text/x-moz-url", URI.spec + "\n" + aEvent.target.label);
>+              aXferData.data.addDataForFlavour("text/html", '<a href="' + URI.spec + '">' + aEvent.target.label + '</a>');
>+            }
>+          }
Why would a tab not have a URI?
You should use linkedBrowser.contentTitle || URI.spec in case the tab is loading.

>+              var ib = document.getElementById('tab-drop-indicator-bar');
>+              ib.setAttribute('dragging','true');
Should use .hidden to show/hide XUL elements.

>+              if (window.getComputedStyle(this.parentNode, null).direction == "ltr") {
Why this.parentNode?

>+                if (newIndex == this.mTabs.length) {dump("end?\n");
No dump ;-)

>+                  ind.style.marginLeft = tabAtPrevIndex.boxObject.x + 
>+                                         tabAtPrevIndex.boxObject.width - this.boxObject.x - 7 + 'px';
>+                } else {dump("not end!\n");
>+                  ind.style.marginLeft = tabAtNewIndex.boxObject.x - this.boxObject.x - 7 + 'px';
Where do the 7s come from? Assuming a certain image size, perhaps?

>+              var newIndex = this.getNewIndex(aEvent);
>+              if (newIndex > aXferData.data) 
>+                newIndex--;
>+              if (newIndex != aXferData.data) 
>+                this.moveTabTo(this.mTabs[aXferData.data], newIndex);
So, we subtract one here, and add it on again in moveTabTo. How silly.

>+          this.mTabContainer.selectedIndex = this.mCurrentTab._tPos;
>+          this.updateCurrentBrowser();
These shouldn't be necessary, there's nothing to update.

>+            var i;
>+            for (i = aEvent.target.localName == "tab" ? aEvent.target._tPos : 0; i < this.mTabs.length; i++) {
for (var i = ...
Unless you change the return i; to break; and return i; at the end instead.

>+                if (aEvent.clientX > this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) 
>+                return i;
Indentation nit.

+            (window.getComputedStyle(this.parentNode, null).direction == "ltr") ? this.moveTabBackward() : this.moveTabForward() ;
Use if/else please.

Comment 110

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated patch

(In reply to comment #108)
> (From update of attachment 199714 [details] [diff] [review] [edit])
> Note: review of the actual drag drop code to follow!
> 
> >+        <xul:hbox id="tab-drop-indicator-bar">
> >+          <xul:hbox id="tab-drop-indicator"/>
> >+        </xul:hbox>
> >+        <xul:hbox class="tabbrowser-strip chromeclass-toolbar" collapsed="true" tooltip="_child" context="_child"
> >+                  anonid="strip" selectedIndex="0"
> 1. You're mixing ids and anonids.
The ids are matched from CSS.  Changed, though it makes a couple other places less nice (getAnonymousElementByAttribute(this, "class", "...") instead of a simple getElementById)

> 2. Why isn't the tab drop indicator an image?
Fixed

> 3. You should use hidden="true" to hide the indicator.
Fixed

> 4. Might it be possible to make the strip a stack to position the indicator?
> (position: relative doesn't strike me as very XUL-friendly but check with bz)
bz said it should be ok.  I suspect a stack might make things a little messier.

> >+                  ondragexit="nsDragAndDrop.dragExit(event, this.parentNode.parentNode); event.stopPropagation();">
> Are we now doing drag and drop over the whole tabbrowser? If so, it might be
> better to convert these to <handler>s.
Only the tab strip.  I'd rather not risk seeing drags from any other parts of the tabbrowser that someone may manage to start a drag on.

> >+                    anonid="tabcontainer" selectexTabIndex="0"
> selectexTabIndex?
Fixed

> >-      <field name="mTabBox">
> >+      <field name="mTabBox" readonly="true">
      ...<snip>...
> >+        document.getAnonymousElementByAttribute(this, "anonid", "panelcontainer");
> >       </field>
> You're mixing getAnonymousNodes and getAnonymousElementByAttribute.
Fixed.  I gave more things anonids.

> 
> >+#ifdef XP_MACOSX
> :-P
Fixed

 
> >+      <method name="dragDropSecurityCheck">
> Remove this as mentioned on IRC.
Fixed

> >       <method name="updateCurrentBrowser">
> >         <body>
> >           <![CDATA[
> >-            var newBrowser = this.mPanelContainer.childNodes[this.mPanelContainer.selectedIndex];
> >+            var newBrowser = this.selectedTab.linkedBrowser;
> Why doesn't the old code work? Or try this.mPanelContainer.selectedPanel
> instead.
I find the code from the patch cleaner, but fixed.

> >-                var listener = this.mTabListeners[this.mPanelContainer.selectedIndex];
> >+                var listener = this.mTabListeners[this.mTabContainer.selectedIndex];
> How easy would it be to make the listeners (and filters) match up with the
> panel index, rather than the tab index?
What's the benefit?  I don't really know what they're used for, and in the interest in having a patch done soon, am not investigating this.

> >+            var uniqueId = "panel" + Date.now() + position;
> The unique ID generator is unnecessarily complicated. Just use "panel" plus an
> incrementing counter.
Done

> 
> >+            this.mPanelContainer.lastChild.id = uniqueId;
> b.id?
Fixed

> 
> >+            t.linkedBrowser = b;
> I'm not keen on using this to link the tab and browser, but I suppose anything
> else is too inconvenient.
> 
> >+            t._tPos = position;
> You never seem to usefully use this.
> e.g. this.mCurrentTab._tPos becomes this.mTabContainer.selectedIndex
I'm keeping it for now.

> also change moveTabTo to take two tab indexes rather than a tab and an index.
Fixed

> >+            this.mCurrentTab.selected = true;
     ...<snip>...
> >+            this.updateCurrentBrowser();
> Why are you setting this.mCurrentTab.selected?
> That's probably what's forcing you to update the current browser.
You partly fixed it in the unbitrotted version you sent me, I fixed it the rest of the way (i.e. by removing the updatecurrentbrowser call).

> >+            //Firefox does "this.updateCurrentBrowser()" and "oldBrowser.focused(Window|Element) = null"
> We do the latter in destroy() which is the right place for it :-P
Ok.  Comment removed.

> >+            var i;
> >+            browsers.item = function(i) {return this[i];}
> >+            for (i = 0; i < this.mTabs.length; i++)
> Not: for (var i = 0;
Assuming you meant "Nit", fixed, in a bunch of places.

> >-          this.mCurrentBrowser = this.mPanelContainer.firstChild;
> >+          this.mCurrentBrowser = this.mPanelContainer.childNodes[0];
> Why this change?
No idea.  Fixed.

> >+          this.mPanelContainer.childNodes[0].id = uniqueId;
> Using the new uniqueId scheme can set this id in the XUL?
No.  Well maybe, but I'm not going to.

> >Index: themes/classic/jar.mn
> What about Modern changes?
Fixed

> 
> >+  skin/classic/global/tabDragDrop/tabDragIndicator.png        (global/tabDragDrop/tabDragIndicator.png)
> Remind me to check the image for 256-colour safety.
Ok

> > #endif
> It looks as if you added this image inside an #else XP_MACOSX block by mistake?
Fixed
 
> >+    <!-- Tab reordering keys -->
> >+    <key id="key_moveTabLeft" keycode="VK_LEFT" modifiers="accel" command="cmd_moveTabLeft"/>
> ...<snip>...
> >+    <key id="key_moveTabToEnd" keycode="VK_END" modifiers="accel" command="cmd_moveTabToEnd"/>
> Might as well call the functions directly rather than via commands.
Fixed

(In reply to comment #109)
> (From update of attachment 199714 [details] [diff] [review] [edit])
> >+          if (aEvent.target.localName == "tab") {
          ...<snip>....
> >+              aXferData.data.addDataForFlavour("text/html", '<a href="' + URI.spec + '">' + aEvent.target.label + '</a>');
> >+            }
> >+          }
> Why would a tab not have a URI?
> You should use linkedBrowser.contentTitle || URI.spec in case the tab is
> loading.
Fixed

> >+              var ib = document.getElementById('tab-drop-indicator-bar');
> >+              ib.setAttribute('dragging','true');
> Should use .hidden to show/hide XUL elements.
Fixed


> >+              if (window.getComputedStyle(this.parentNode, null).direction == "ltr") {
> Why this.parentNode?
Fixed

> >+                if (newIndex == this.mTabs.length) {dump("end?\n");
> No dump ;-)
Fixed

> >+                  ind.style.marginLeft = tabAtPrevIndex.boxObject.x + 
> >+                                         tabAtPrevIndex.boxObject.width - this.boxObject.x - 7 + 'px';
> >+                } else {dump("not end!\n");
> >+                  ind.style.marginLeft = tabAtNewIndex.boxObject.x - this.boxObject.x - 7 + 'px';
> Where do the 7s come from? Assuming a certain image size, perhaps?
They're related to the image size, but ind.boxObject.width/2 doesn't work very well.  I need suggestions for alternatives, I guess.  I changed the 7 to an 8 because it works better for now.

> >+              var newIndex = this.getNewIndex(aEvent);
> >+              if (newIndex > aXferData.data) 
> >+                newIndex--;
> >+              if (newIndex != aXferData.data) 
> >+                this.moveTabTo(this.mTabs[aXferData.data], newIndex);
> So, we subtract one here, and add it on again in moveTabTo. How silly.
I'm not going to change that one.  Right now, moveTabForward uess tabPos+1 as the new index, and moveTabBackward uses tabPos-1.  Changing this would require making them use less clear new indicies.

> 
> >+          this.mTabContainer.selectedIndex = this.mCurrentTab._tPos;
> >+          this.updateCurrentBrowser();
> These shouldn't be necessary, there's nothing to update.
Fixed

> >+            var i;
> >+            for (i = aEvent.target.localName == "tab" ? aEvent.target._tPos : 0; i < this.mTabs.length; i++) {
> for (var i = ...
> Unless you change the return i; to break; and return i; at the end instead.
Fixed (kept loop style consistent with the rest of the file)

> >+                if (aEvent.clientX > this.mTabs[i].boxObject.x + this.mTabs[i].boxObject.width / 2) 
> >+                return i;
> Indentation nit.
Fixed

> +            (window.getComputedStyle(this.parentNode, null).direction ==
> "ltr") ? this.moveTabBackward() : this.moveTabForward() ;
> Use if/else please.
Fixed

Comment 111

[[e-mail]]

Attachment: 256-colour safe indicator

Note: I don't see the point of calling it tabDragDrop/tabDragIndicator.png

Comment 112

[[e-mail]]

Comment on attachment 202599 [details] [diff] [review]
updated patch

>+  skin/classic/global/tabDragDrop/tabDragIndicator.png        (global/tabDragDrop/tabDragIndicator.png)
As I mentioned, I don't see the point of an extra folder for one file.
Also theme style is to use dash-separated-filename-component-format.

>+    background: url('chrome://global/skin/tabDragDrop/tabDragIndicator.png') 50% 50% no-repeat;
Should be list-style-image: url("..."); (both apply to Modern too of course).

>+                  anonid="strip" selectedIndex="0"
Where do you use selectedIndex?

>+                  ondraggesture="nsDragAndDrop.startDrag(event, this.parentNode.parentNode); event.stopPropagation();"
>+                  ondragover="nsDragAndDrop.dragOver(event, this.parentNode.parentNode); event.stopPropagation();"
>+                  ondragdrop="nsDragAndDrop.drop(event, this.parentNode.parentNode); event.stopPropagation();"
>+                  ondragexit="nsDragAndDrop.dragExit(event, this.parentNode.parentNode); event.stopPropagation();">
This covers the strip, and the other set cover the content; which other parts of the tabbrowser are you worried about?

>+                    anonid="tabcontainer" selectedTabIndex="0"
Where do you use selectedTabIndex?

>+      <field name="lastTabNumber">
Nit: the way you use it it's actually the next tab number...

>-            var position = this.mTabContainer.childNodes.length-1;
>+            var position = this.mTabs.length-1;
Nit: might as well add spaces around the - sign.

>+            t._tPos = position;
I'm still not convinced of the utility of _tPos, you seem to spend more time updating it than using it.

>+                for (var i = this.mTabs.length - 1; i >= 0; --i) {
>+                if (this.mTabs[i] != aTab)
>+                    this.removeTab(this.mTabs[i]);
Some of your blocks have the wrong indentation, unfortunately I forgot to make a list, but the if here has the wrong indentation (although strangely enough the this.removeTab is correct, once the if has been corrected).

>+          this.mTabContainer.selectedIndex = this.mCurrentTab._tPos;
>+          return this.mTabs[aSrcIndex];
What good are these lines?

>+              this.moveTabForward() ;
Nit: extraneous space before ;

>+            (window.getComputedStyle(this.parentNode, null).direction == "ltr") ? this.moveTabForward() : this.moveTabBackward() ;
Not using if, and extraneous space before ;

>+            var tabPos = this.mCurrentTab._tPos;
I really don't like this. Please use the selectedIndex.

Comment 113

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated patch, v2

Addresses multiple issues discussed on IRC.  I'm still using the .png image because it's smaller than the .gif.

Comment 114

[[e-mail]]

Attachment: optimized version

Comment 115

[[e-mail]]

Comment on attachment 202774 [details] [diff] [review]
updated patch, v2

>+  skin/classic/global/icons/tab-drag-indicator.png                      (global/icons/tab-drag-indicator.png)
Nit: use the .gif from attachment 202845 [details] ;-)

>       <method name="getBrowserForTab">
>         <parameter name="aTab"/>
>         <body>
>+          <![CDATA[
>+            return aTab.linkedBrowser;
>+          ]]>
Nit: this should validate aTab (i.e. it should be an element whose parent should be our <tabs> element).

>+            var tabAtPrevIndex = (newIndex > 0) ? this.mTabs[newIndex-1] : null;
Nit: newIndex - 1

>-            // Perform a security check before loading the URI
Nit: Don't delete this.

>+            if (window.getComputedStyle(this.parentNode, null).direction == "ltr")
Why this.parentNode? this should work, no?

Comment 116

[[e-mail]]

Whoops, I can't drag links to background tabs now :-(

Comment 117

[[e-mail]]

Comment on attachment 202774 [details] [diff] [review]
updated patch, v2

>-              this.getBrowserForTab(tab).loadURI(getShortcutOrURI(url));
>+              this.tab.linkedBrowser.loadURI(getShortcutOrURI(url));
>             }
>-            if (this.mCurrentTab != tab && !bgLoad)
>-              this.selectedTab = tab;
This looks like it's your issue: this.getBrowserForTab(tab) is tab.linkedBrowser, not this.tab.linkedBrowser, also those last two lines should not have been deleted.

Comment 118

[jag (Peter Annema)]

You have the same problem with reload:

-            this.getBrowserForTab(aTab).reload();
+            this.aTab.linkedBrowser.reload();

Note to self: continue from drag&drop code

Comment 119

[jag (Peter Annema)]

In testing the patch I was kinda surprised that ctrl+left/right/etc. work from the content area too. Great on the one hand, annoying on the other (I was trying to move left by a word with focus in the wrong place). Almost as annoying as backspace taking me back a page (which of course I turned off). Not saying this should change per se, but a point for discussion.

Change |getNewIndex(aEvent)| to |getDropIndex(aTab, aEvent)| (or perhaps aTabIndex for the first param) and put the "subtract one if newIndex > tabIndex" in there. That will make getDropIndex return the tab's desired index, much nicer API-wise I think.

I'd also suggest making non-sensical drops (i.e. right before and right after the dragged tab) give the can't-drop feedback. Should be pretty easy, add method |canDrop| and return false if getDropIndex(...) == draggedTabIndex, return true if it's anywhere else inside the tab strip, and false otherwise.

One thing to note there is that currently the drop indicator bar only gets hidden onDragExit, you'd have to also hide it onDragOver when getDropIndex(...) == draggedTabIndex.

Speaking of onDragExit, to reduce flicker you could make it smarter so it only hides the indicator bar when you leave the tab strip or are over the don't-drop region inside it. The latter would already be taken care of if you've implemented my previous suggestion, otherwise you could implement it through onDragOver, but only with the dragged tab as the dead area.

Code nits:

The prevalent style seems to be to have <![CDATA[ indented with regard to <body>, could you do that for the code you're touching / writing?


+      <method name="moveTabTo">
+          aDestIndex = aDestIndex < aSrcIndex ? aDestIndex: aDestIndex + 1;

Space before the :


Other than that and the other bugs pointed out by Neil and I, it looks good.

Oh, heh, so, since I'm a ctrl+page-up/down tab switcher, I just found myself wanting to use ctrl+home to switch to the first tab. Instead, my current tab moved. Not that ctrl+home has ever worked, but ... I'm sure my fingers will learn, sooner or later.

Oh, one more question. Why do arrow keys only wrap on Mac? Since ctrl+tab wraps, I was expecting moving tabs to also wrap.

Comment 120

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: (hopefully) final patch

Carrying forward sr=neil

Addresses all of neil's nits and a couple of jag's.  The rest should go in followup bugs.

Comment 121

[jag (Peter Annema)]

Comment on attachment 205086 [details] [diff] [review]
(hopefully) final patch

Per discussion on irc, we're not stuffing the index adjustment in getDropIndex, since the other place that calls it doesn't want it. We're also post-poning most of my other suggestions as follow-up work.

r=jag if you make us wrap on all platforms.

Comment 122

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 205086 [details] [diff] [review]
(hopefully) final patch

SeaMonkey-only patch

Comment 123

[Robert Kaiser]

Comment on attachment 205086 [details] [diff] [review]
(hopefully) final patch

a=me given that trunk testing showed it's working well :)

Comment 124

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Landed on 1.8 branch with approval-seamonkey1.0=KaiRo,me

Comment 125

[Serge Gautherie (:sgautherie)]

V.Fixed. (on Trunk)

Comment 126

[[e-mail]]

Comment on attachment 202774 [details] [diff] [review]
updated patch, v2

>+            if (window.getComputedStyle(this, null).direction == "ltr") {
>+              if (newIndex == this.mTabs.length) {
>+                ind.style.marginLeft = tabAtPrevIndex.boxObject.x + 
>+                                       tabAtPrevIndex.boxObject.width - this.boxObject.x + 'px';
>+              }
>+              else {
>+                var tabAtNewIndex = this.mTabs[newIndex];
>+                ind.style.marginLeft = tabAtNewIndex.boxObject.x - this.boxObject.x + 'px';
>+              }            
>+            } else {
>+              if (newIndex == this.mTabs.length) {
>+                ind.style.marginRight = this.boxObject.width + this.boxObject.x -
>+                                        tabAtPrevIndex.boxObject.x + 'px';
>+              }
>+              else {
>+                var tabAtNewIndex = this.mTabs[newIndex];
>+                ind.style.marginRight = this.boxObject.width + this.boxObject.x -
>+                                        tabAtNewIndex.boxObject.x -
>+                                        tabAtNewIndex.boxObject.width + 'px';
>+              }
>+            }
Redeclaration of var tabAtNewIndex (you won't see this because ben got brendan to disable those warnings) :-/ If someone gets a chance, it would be nice to inline the first version:
ind.style.marginLeft = this.mTabs[newIndex].boxObject.x - this.boxObject.x + 'px';

Comment 127

[jag (Peter Annema)]

I fixed this when I checked in for bug 319244.

Comment 128

[Karsten Düsterloh]

Like jag already mentioned in comment 119, this fix caused usability problems, eg. bug 320161 and bug 321128. 

Comment 129

[Stefan [:stefanh]]

Uh, sorry for spamming...

Comment 130

[Eyal Rozenberg]

Kindly remove the tab reordering key bindings. Thank you and have a nice day.

BTW, Accel+Shift+arrow is no good either, that's used for word selection.

Comment 131

[jag (Peter Annema)]

Eyal: should be fixed, see bug 321128

Comment 132

[jag (Peter Annema)]

Attachment: Missed one place that needed a browser lookup through tab.linkedBrowser

Say you have tabs tA and tB with browsers bA and bB and a different URL in each. If you switch the two tabs, then load a bookmark group with your pref set to replace instead of append, we'll grab bA's sessionHistory, store it, and close tB (which removes bB, whose sessionHistory is now lost forever), leaving bA as still the first child and we'll grab and store bA's sessionHistory again. On restore this results in two tabs with the same URL loaded. Not to mention that even if this had worked we'd be restoring the tabs in the wrong order.

Comment 133

[Robert Kaiser]

Comment on attachment 209575 [details] [diff] [review]
Missed one place that needed a browser lookup through tab.linkedBrowser

a=me for both branches so that we can ship without that glitch...

Comment 134

[jag (Peter Annema)]

Comment on attachment 209575 [details] [diff] [review]
Missed one place that needed a browser lookup through tab.linkedBrowser

That ; shouldn't be after sessionHistory (it makes tboxen go orange, and while I happen to think that's a pretty colour, fields of green look much better). I removed this on trunk and will check in without it on the branches ;-)

Comment 135

[jag (Peter Annema)]

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.126.2.13; previous revision: 1.126.2.12
done

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.126.2.4.2.9; previous revision: 1.126.2.4.2.8
done