Closed Bug 105885 Opened 23 years ago Closed 19 years ago

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

Categories

(SeaMonkey :: UI Design, enhancement, P1)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey1.0beta

People

(Reporter: grantbow, Assigned: csthomas)

References

(Blocks 1 open bug)

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8, relnote, Whiteboard: parity-opera)

Attachments

(3 files, 10 obsolete files)

97 bytes, image/gif
timeless
: review+
Details
43.28 KB, patch
jag+mozilla
: review+
csthomas
: superreview+
Details | Diff | Splinter Review
1.02 KB, patch
neil
: review+
Details | Diff | Splinter Review
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.
confused that I cannot find a dup. Marking NEW...
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 106928 has been marked as a duplicate of this bug. ***
*** Bug 107078 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1
Keywords: helpwanted
BTW what about PC --> All, Linux --> All? On Windoze isn't this function too.
*** Bug 111997 has been marked as a duplicate of this bug. ***
-->All, All
OS: Linux → All
Hardware: PC → All
QA Contact: blakeross → sairuh
*** Bug 112946 has been marked as a duplicate of this bug. ***
The Windows taskbar doesn't let you rearrange buttons either...
*** Bug 113734 has been marked as a duplicate of this bug. ***
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?
Keywords: nsbeta1
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.
Reassigning to new component owner.
Assignee: hyatt → jaggernaut
Status: ASSIGNED → NEW
-> 1.1
Target Milestone: mozilla1.0.1 → mozilla1.1
*** Bug 123665 has been marked as a duplicate of this bug. ***
nsbeta1- per ADT triage team, ->future
Keywords: nsbeta1nsbeta1-
Target Milestone: mozilla1.1 → Future
Blocks: 126299
*** Bug 128860 has been marked as a duplicate of this bug. ***
*** Bug 142051 has been marked as a duplicate of this bug. ***
*** Bug 148514 has been marked as a duplicate of this bug. ***
*** Bug 156069 has been marked as a duplicate of this bug. ***
Summary: Changing tabs' order with tabbrowser → reorder/rearrange tabs in tabbed browser
nominating --if cycles are available to implement this for buffy...
Keywords: nsbeta1-nsbeta1
FYI: this addon has ability to reorder/rearrange tabs with drag'n'drop.
http://www.cc-net.or.jp/~piro/xul/_tabextensions.html
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!! :(
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...
QA Contact: sairuh → pmac
nsbeta1+/adt3 per the nav triage team.

Keywords: nsbeta1nsbeta1+
Whiteboard: [adt3]
*** Bug 180553 has been marked as a duplicate of this bug. ***
http://multizilla.mozdev.org in case you don't want to wait :-)
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).

Mac - see bug 132674 re selecting multiple and/or non-adjacent tabs
nsbeta1- per the nav triage team.
Keywords: nsbeta1+nsbeta1-
*** Bug 191257 has been marked as a duplicate of this bug. ***
*** Bug 191278 has been marked as a duplicate of this bug. ***
*** Bug 160720 has been marked as a duplicate of this bug. ***
*** Bug 193044 has been marked as a duplicate of this bug. ***
I didn't see this before I filled a dupe.  Guess I didn't search using the right
terminology.  I vote for this too!! ;)
*** Bug 194913 has been marked as a duplicate of this bug. ***
*** Bug 198805 has been marked as a duplicate of this bug. ***
*** Bug 204941 has been marked as a duplicate of this bug. ***
so many dupes,
and 48 votes.

any plan on implementing this?
*** Bug 212274 has been marked as a duplicate of this bug. ***
*** Bug 214261 has been marked as a duplicate of this bug. ***
Blocks: majorbugs
*** Bug 221136 has been marked as a duplicate of this bug. ***
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.



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.
er... that would be "closed" instead of "switched" in the pevious comment.
*** Bug 232282 has been marked as a duplicate of this bug. ***
comment 27,
the tabs<==>windows transfer is a different issue. It's bug 102132.
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.
*** Bug 232865 has been marked as a duplicate of this bug. ***
pulled vote because for me this bug is fixed
John-

Stop spamming bugs just because you have found the funtionality in an extension.  
*** Bug 232952 has been marked as a duplicate of this bug. ***
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
Summary: reorder/rearrange tabs in tabbed browser → Reorder / Rearrange / Move tabs in tabbed browser
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
*** Bug 241365 has been marked as a duplicate of this bug. ***
Whiteboard: [adt3] → [adt3] parity-opera
First requested in 2001 ... no hope for this one, huh? Well, voted anyway.
Yeah... almost pointless, it seems, but I voted anyway.
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.
That extension only works with Firefox - this bug has been filed under Mozilla
(Seamonkey).
Does multizilla allow tab reordering?
(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.
One more vote, especially for keyboard shortcuts for moving a tab left / right!
Even konqueror offers this for quite some time now!
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??
(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.
(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?
(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.
Assignee: jag → cst
Keywords: helpwanted
Target Milestone: Future → mozilla1.9alpha
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 ;)
I think it would be much easier to play with ordinals just like we do in tree.xml
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. 
(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.
What is this "miniT-drag extension" and where do I get it?
(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.
> 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).
(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.
Status: NEW → ASSIGNED
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.
QA Contact: pmac → bugzilla
(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
Attached patch patch (obsolete) — Splinter Review
Attachment #173594 - Attachment is obsolete: true
Attachment #174710 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #174710 - Flags: review?(timeless)
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.
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 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?
Attachment #174710 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attached patch mostly-fixed patch (obsolete) — Splinter Review
(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
+	       }
Attachment #174710 - Attachment is obsolete: true
Is this a duplicate of bug 179656?

They seem to be about the same thing.
Blocks: MoveTabs
No.  That bug is targetted against Firefox specifically.  (Although, I suppose,
a dependency could be created.)
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).
(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.
*** Bug 285760 has been marked as a duplicate of this bug. ***
(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().
*** Bug 287468 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1?
Bug 179656 covers the Firefox port of this bug's work, and is marked blocking
aviary1.1 already.
Flags: blocking-aviary1.1?
*** Bug 288205 has been marked as a duplicate of this bug. ***
Wait, wasn't this marked as blocking 1.1? How come it is blank now? 
Flags: blocking-aviary1.1?
This bug blocks 179656 which is marked blocking-aviary1.1

From comment #88

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

------- Additional Comments From gavin.sharp@gmail.com  2005-03-25 12:56 PST -------
Bug 179656 covers the Firefox port of this bug's work, and is marked blocking
aviary1.1 already.
(In reply to comment #90)
> Wait, wasn't this marked as blocking 1.1? How come it is blank now? 

Read comment 88.
Flags: blocking-aviary1.1?
(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.
No longer blocks: majorbugs
Component: Tabbed Browser → XP Apps: GUI Features
Product: Core → Mozilla Application Suite
Target Milestone: mozilla1.9alpha → ---
Attachment #175606 - Attachment is obsolete: true
Since bug 179656 is now fixed on the trunk, shouldn't this bug be closed, or
duped to that bug?
No longer blocks: MoveTabs
Depends on: MoveTabs
Summary: Reorder / Rearrange / Move tabs in tabbed browser → Reorder / Rearrange / Move tabs in the Suite's tabbed browser
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.
Flags: blocking-seamonkey1.0a?
Comment on attachment 187979 [details] [diff] [review]
port of what firefox uses

Bugzilla timed out while I was attaching the patch.
Attachment #187979 - Attachment is obsolete: true
Removing <method name="getBrowserForTab"> will break a lot of add-ons, so it
should not be removed.
Attachment #187978 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #187978 - Flags: review?(db48x) → review-
Attachment #187978 - Attachment is obsolete: true
Attachment #187978 - Flags: superreview?(neil.parkwaycc.co.uk)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #188647 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #188647 - Flags: review?(db48x)
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 on attachment 188647 [details] [diff] [review]
patch v2

r=db48x, just make sure you address Ian's comments  on the theme stuff
Attachment #188647 - Flags: review?(db48x) → review+
Priority: -- → P1
QA Contact: bugzilla → stdonner
Whiteboard: [adt3] parity-opera → [adt3] parity-opera [cst:sr?]
Target Milestone: --- → Seamonkey1.0beta
Target Milestone: Seamonkey1.0beta → Seamonkey1.0alpha
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 ;-)
Flags: blocking-seamonkey1.0a? → blocking-seamonkey1.0a-
Target Milestone: Seamonkey1.0alpha → Seamonkey1.0beta
Whiteboard: [adt3] parity-opera [cst:sr?] → [adt3] parity-opera [cst:sr?][cst: see also bug 309452 after fixing this]
Can this get a review so it gets in? Rather ask than mark to block 1.0b. Thanks.
Attached patch unbitrotted, bugfixed (obsolete) — Splinter Review
carrying forward r=db48x
Attachment #188647 - Attachment is obsolete: true
Attachment #199714 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #199714 - Flags: review+
Attachment #188647 - Flags: superreview?(neil.parkwaycc.co.uk)
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 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 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.
Attachment #199714 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attached patch updated patch (obsolete) — Splinter Review
(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
Attachment #199714 - Attachment is obsolete: true
Attachment #202599 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #202599 - Flags: review?(db48x)
Attached image 256-colour safe indicator (obsolete) —
Note: I don't see the point of calling it tabDragDrop/tabDragIndicator.png
Attachment #202660 - Flags: review?(db48x)
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.
Attached patch updated patch, v2 (obsolete) — Splinter Review
Addresses multiple issues discussed on IRC.  I'm still using the .png image because it's smaller than the .gif.
Attachment #202599 - Attachment is obsolete: true
Attachment #202774 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #202774 - Flags: review?(db48x)
Attachment #202599 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #202599 - Flags: review?(db48x)
Attached image optimized version
Attachment #202660 - Attachment is obsolete: true
Attachment #202845 - Flags: review?(db48x)
Attachment #202660 - Flags: review?(db48x)
Blocks: 316258
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?
Attachment #202774 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Whoops, I can't drag links to background tabs now :-(
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.
You have the same problem with reload:

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

Note to self: continue from drag&drop code
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.
Carrying forward sr=neil

Addresses all of neil's nits and a couple of jag's.  The rest should go in followup bugs.
Attachment #202774 - Attachment is obsolete: true
Attachment #205086 - Flags: superreview+
Attachment #205086 - Flags: review?(jag)
Attachment #202774 - Flags: review?(db48x)
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.
Attachment #205086 - Flags: review?(jag) → review+
Attachment #202845 - Flags: review?(db48x) → review+
Comment on attachment 205086 [details] [diff] [review]
(hopefully) final patch

SeaMonkey-only patch
Attachment #205086 - Flags: approval1.8.0.1?
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [adt3] parity-opera [cst:sr?][cst: see also bug 309452 after fixing this] → parity-opera [cst: see also bug 309452 after fixing this]
Attachment #205086 - Flags: approval1.8.0.1? → approval-seamonkey1.0?
Comment on attachment 205086 [details] [diff] [review]
(hopefully) final patch

a=me given that trunk testing showed it's working well :)
Attachment #205086 - Flags: approval-seamonkey1.0? → approval-seamonkey1.0+
Landed on 1.8 branch with approval-seamonkey1.0=KaiRo,me
V.Fixed. (on Trunk)
No longer blocks: 319876
Status: RESOLVED → VERIFIED
Whiteboard: parity-opera [cst: see also bug 309452 after fixing this] → parity-opera
No longer blocks: 319246
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';
I fixed this when I checked in for bug 319244.
Depends on: 320161
Like jag already mentioned in comment 119, this fix caused usability problems, eg. bug 320161 and bug 321128. 
Depends on: 321128
Uh, sorry for spamming...
Blocks: 320161, 321128
No longer depends on: 320161, 321128
Blocks: 320842, 321330
No longer depends on: 320842
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.
Blocks: 321578
Eyal: should be fixed, see bug 321128
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.
Attachment #209575 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209575 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #209575 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209575 - Flags: superreview+
Attachment #209575 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #209575 - Flags: review+
Attachment #209575 - Flags: approval-seamonkey1.1?
Attachment #209575 - Flags: approval-seamonkey1.0?
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...
Attachment #209575 - Flags: approval-seamonkey1.1?
Attachment #209575 - Flags: approval-seamonkey1.1+
Attachment #209575 - Flags: approval-seamonkey1.0?
Attachment #209575 - Flags: approval-seamonkey1.0+
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 ;-)
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
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.