Last Comment Bug 321490 - Make SeaMonkey compatible again with moveTabTo implementation of the Firefox
: Make SeaMonkey compatible again with moveTabTo implementation of the Firefox
Status: RESOLVED FIXED
: fixed-seamonkey1.0, fixed-seamonkey1.1a, fixed1.8.0.1, fixed1.8.1
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: x86 Windows 2000
: -- minor (vote)
: ---
Assigned To: jag (Peter Annema)
:
Mentors:
http://www.extensionsmirror.nl/index....
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-25 09:47 PST by Torsten Bondieck
Modified: 2008-07-31 04:22 PDT (History)
9 users (show)
csthomas: blocking‑seamonkey1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch? (4.58 KB, patch)
2006-01-11 14:44 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Splinter Review
Check if the type of the first parameter is "number", assume it's a <tab> otherwise. (1.78 KB, patch)
2006-01-16 23:35 PST, jag (Peter Annema)
csthomas: review+
neil: superreview-
Details | Diff | Splinter Review
Use mTabs.item() and get tab from insertBefore (1.58 KB, patch)
2006-01-19 18:31 PST, jag (Peter Annema)
csthomas: review+
neil: superreview+
csthomas: approval‑seamonkey1.0+
iann_bugzilla: approval‑seamonkey1.1a+
Details | Diff | Splinter Review
Get tab from aSrcIndex and use mTabs[] || null (1.62 KB, patch)
2006-01-19 18:35 PST, jag (Peter Annema)
neil: superreview-
Details | Diff | Splinter Review
Option (b) with >= (1.51 KB, patch)
2006-01-22 17:00 PST, jag (Peter Annema)
csthomas: review+
Details | Diff | Splinter Review

Description Torsten Bondieck 2005-12-25 09:47:27 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.8) Gecko/20051219 SeaMonkey/1.0b
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.8) Gecko/20051219 SeaMonkey/1.0b

After install the release from seamonkey 1.0b the extension 'undoclosetab' no longer work!

The author from 'undoclosetab' says to this behavior:

"Latest SeaMonkey does have a <tabbrowser>.moveTabTo which was implemented in a way which makes it incompatible with the implementation of the Firefox one on which it was based. The Firefox moveTabTo was based on the moveTabTo by my miniT 
which was designed to be compatible with the one provided by TBE. This function has been API compatible since at least 2002-10-13, and many Extensions use this function if it exists (including undoclosetab), which now would cause breakages. If you want undoclosetab to work in further version, file a bug and ask them to change the name of the function or to be compatible with the Firefox implementation"

Please makes the differences between Firefox and Seamonkey(Mozilla) not bigger!
We have enough extension only for Firefox. Makes work for extension developer not harder...

Reproducible: Always

Steps to Reproduce:
1. open two or three tab
2. close a tab and click right mouse undoclose tab
3. a blank tab open


Actual Results:  
The system is confused now and i can no undo any tab.
Comment 1 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2005-12-25 13:30:20 PST
I believe we broke compat because their API was nonintuitive.
Comment 2 neil@parkwaycc.co.uk 2005-12-26 06:46:46 PST
OK, so what's the best approach here? Rename moveTabTo and provide a stub function that calls through to the real function?
Comment 3 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-11 14:44:49 PST
Created attachment 208228 [details] [diff] [review]
patch?

I can't test this.
Comment 4 jag (Peter Annema) 2006-01-11 16:41:55 PST
Love the JS loose typing. Take a look at the type of the first parameter and take it from there.
Comment 5 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-11 17:32:58 PST
(In reply to comment #4)
> Love the JS loose typing. Take a look at the type of the first parameter and
> take it from there.
> 

I don't understand - did I mess something up?  The Firefox version takes a tab and an index.  Our version takes two indices.
Comment 6 jag (Peter Annema) 2006-01-12 17:38:34 PST
Sorry for being cryptic.

I think you can use |instanceof| or |"localName" in| with the first argument to determine whether a number or a tab was passed in, allowing you to not need two separate method names.
Comment 7 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-12 17:45:49 PST
(In reply to comment #6)
> Sorry for being cryptic.
> 
> I think you can use |instanceof| or |"localName" in| with the first argument to
> determine whether a number or a tab was passed in, allowing you to not need two
> separate method names.
> 

I thought about that.  This method has the advantage of keeping our method (and the wrapper) really simple; that method has the advantage of only having one function.  Whichever the reviewers prefer is fine with me.
Comment 8 jag (Peter Annema) 2006-01-12 22:18:48 PST
      <method name="moveTabTo">
        <parameter name="aSrcIndex"/>
        <parameter name="aDestIndex"/>
        <body>
          <![CDATA[
            // for compatibility with extensions
            if (typeof(aSrcIndex) == "object")
              aSrcIndex = this.getTabIndex(aSrcIndex);

            this.mTabFilters.splice(aDestIndex, 0, this.mTabFilters.splice(aSrcIndex, 1)[0]);
            this.mTabListeners.splice(aDestIndex, 0, this.mTabListeners.splice(aSrcIndex, 1)[0]);

            this.mCurrentTab.selected = false;
            if (aDestIndex == this.mTabs.length - 1)
               this.mTabContainer.appendChild(this.mTabs[aSrcIndex]);
            else {
              var beforeIndex = aDestIndex < aSrcIndex                                    ? aDestIndex : aDestIndex + 1;
              this.mTabContainer.insertBefore(this.mTabs[aSrcIndex],
                                              this.mTabs[beforeIndex]);
            }
            this.mCurrentTab.selected = true;

            return this.mTabs[aDestIndex];
          ]]>
        </body>
      </method>

Not that big a deal I'd say. I added and moved down the |beforeIndex| so we can easily return the moved tab, though something like this would've worked too:

            // for compatibility with extensions
            var tab;
            if (typeof(aSrcIndex) == "object") {
              tab = aSrcIndex;
              aSrcIndex = this.getTabIndex(tab);
            } else
              tab = this.mTabs[aSrcIndex];

            /* old moveTabTo body */

            return tab;
Comment 9 jag (Peter Annema) 2006-01-12 22:19:42 PST
Hmmm, wrapping is a bit odd there, but you get the idea.
Comment 10 neil@parkwaycc.co.uk 2006-01-13 02:53:51 PST
I don't like one-line else (except with one-line then, of course). If you go that route, reverse the test so that the single line in the then portion.
Comment 11 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-13 14:18:23 PST
Comment on attachment 208228 [details] [diff] [review]
patch?

mconnor said he doesn't like this over IRC, and jag said he'd attach his version.
Comment 12 neil@parkwaycc.co.uk 2006-01-14 11:54:49 PST
Comment on attachment 208228 [details] [diff] [review]
patch?

>+            for (var i=0; i<this.mTabs.length; i++) {
>+              if (this.mTabs[i] == aTab)
>+                break;
Didn't jag write a getTabIndex function?

>+            aIndex = (aIndex < i) ? aIndex : aIndex + 1;
if (aIndex >= i)
  aIndex ++;

>+      <method name="moveTabTo2">
Silly name. Use moveTabToIndex?
Comment 13 jag (Peter Annema) 2006-01-16 23:35:08 PST
Created attachment 208715 [details] [diff] [review]
Check if the type of the first parameter is "number", assume it's a <tab> otherwise.

This also puts the |beforeIndex| into a separate var so we have the aDestIndex to return the moved tab from.
Comment 14 neil@parkwaycc.co.uk 2006-01-19 17:01:17 PST
Comment on attachment 208715 [details] [diff] [review]
Check if the type of the first parameter is "number", assume it's a <tab> otherwise.

I don't like the beforeIndex scheme, although I can't decide between a) always returning the first argument b) setting up tab and tab index variables c) using the fact that this.mTabContainer.insertBefore(foo, this.mTabs.item(aDestIndex)) works when aDestIndex == this.mTabs.length plus it returns the moved tab.

Fixing the 3-space indent is good though ;-)
Comment 15 jag (Peter Annema) 2006-01-19 18:31:19 PST
Created attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore
Comment 16 jag (Peter Annema) 2006-01-19 18:35:04 PST
Created attachment 209048 [details] [diff] [review]
Get tab from aSrcIndex and use mTabs[] || null

Also, aDestIndex += ... vs. if (...) ++aDestIndex in the previous patch.

I suspect the final patch will be a little bit from every patch.
Comment 17 neil@parkwaycc.co.uk 2006-01-20 06:06:30 PST
Comment on attachment 209048 [details] [diff] [review]
Get tab from aSrcIndex and use mTabs[] || null

>+            aDestIndex += aDestIndex < aSrcIndex ? 0 : 1;
Heh, why not aDestIndex += aDestIndex >= aSrcIndex; ;-)

>+            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex] || null);
JS strict warning when aDestIndex == this.mTabs.length
Comment 18 neil@parkwaycc.co.uk 2006-01-20 06:13:02 PST
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

>+            if (aDestIndex > aSrcIndex)
Hmm, I should have reviewed these in order... needs to be >=
Comment 19 jag (Peter Annema) 2006-01-20 11:48:54 PST
It shouldn't matter whether we insert a tab before itself or after itself (before its nextSibling), it'll end up in the same place, namely its own spot.
Comment 20 neil@parkwaycc.co.uk 2006-01-20 17:33:27 PST
So does foo.insertBefore(bar, bar); work? It still looks wrong...
Comment 21 jag (Peter Annema) 2006-01-20 18:03:41 PST
It works:

data:text/html,<div id="foo"><a href="#" id="bar" onclick="var foo = document.getElementById('foo'); var bar = document.getElementById('bar'); try { foo.insertBefore(bar, bar); } catch (e) { alert(e); }">bar</a></div>

Also, for `this.mTabs[aDestIndex] || null' the `|| null' part is unnecessary. I thought insertBefore didn't like |undefined| as its second parameter, but it seems to accept it just fine now.

If you wish I can use >=, just let me know whether you want tab returned from insertBefore or from mTabs[aSrcIndex].
Comment 22 neil@parkwaycc.co.uk 2006-01-21 15:53:39 PST
(In reply to comment #21)
>If you wish I can use >=, just let me know whether you want tab returned from
>insertBefore or from mTabs[aSrcIndex].
mTabs[aSrcIndex]? I don't think I've seen that version ;-) The indecision is:
a) always return arguments[0]
b) figure out the source tab and index from arguments[0], return the tab
c) convert the source tab to an index, then back to a tab via insertBefore
Comment 23 jag (Peter Annema) 2006-01-21 17:06:11 PST
(In reply to comment #22)
> mTabs[aSrcIndex]? I don't think I've seen that version ;-)

Oops :-) aDestIndex, of course.

Ok, I see now. And since all of these need to figure out the source index when a source tab is passed in (for removing the filter and the listener), and vice versa (for |insertBefore()|), the indecision seems to be:

a) always return arguments[0]
b) return the source tab from (converted) arguments[0]
c) return the tab via insertBefore()

(a) vs. (b,c) is the more interesting question since it affects the API. For (b) vs. (c) I'd go with (c) since insertBefore already returns the tab anyway and the code is a bit shorter.

I think I'd prefer to always return the moved tab. It's merely a convenience when a source tab is passed in, but actually provides something new (and potentially useful, relatively cheaply) when a source index is passed in.

So, I'd go with (c).
Comment 24 neil@parkwaycc.co.uk 2006-01-22 02:27:05 PST
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

OK, sr=me with >= as noted.
Comment 25 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-22 14:54:39 PST
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

Not sure if you need my review again, but here it is ;)
Comment 26 jag (Peter Annema) 2006-01-22 17:00:34 PST
Created attachment 209313 [details] [diff] [review]
Option (b) with >=

I was ready to check in when I realized that by grabbing |tab| from insertBefore we lose any additional properties set on the tab passed in. So, for full compatibility I'm back to saving and returning the passed in tab.

This is my final answer!

I hope.
Comment 27 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-22 17:10:31 PST
Comment on attachment 209313 [details] [diff] [review]
Option (b) with >=

+            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex]);

Shouldn't this be |, this.mTabs.item(aDestIndex);|?
Comment 28 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-22 17:16:46 PST
(In reply to comment #27)
> (From update of attachment 209313 [details] [diff] [review] [edit])
> +            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex]);
> 
> Shouldn't this be |, this.mTabs.item(aDestIndex);|?
> 

On second thought, it doesn't matter in this version, does it?
Comment 29 jag (Peter Annema) 2006-01-22 17:20:20 PST
Sorry, I should've made that more clear. A quick test seemed to show (to my surprise) that .item(i) was needed 'coz it returns null, where [i] returns undefined. When I ran some more tests on Friday it turned out insertBefore(tab, undefined) works as well. See also comment 21.
Comment 30 jag (Peter Annema) 2006-01-22 17:52:11 PST
Hmm, a quick test suggests that insertBefore() leaves the properties intact, which actually makes sense, you'd want the native object to always be represented by the same js object.
Comment 31 jag (Peter Annema) 2006-01-22 19:31:58 PST
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

Ok, I've convinced myself that this is safe to use.
Comment 32 jag (Peter Annema) 2006-01-22 19:43:41 PST
Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.139; previous revision: 1.138
done
Comment 33 Ian Neal 2006-01-23 04:03:57 PST
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

a=me for SM1.1
a=me for SM1.0, one more needed
Comment 34 neil@parkwaycc.co.uk 2006-01-23 16:18:26 PST
(In reply to comment #27)
>(From update of attachment 209313 [details] [diff] [review])
>>+            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex]);
>Shouldn't this be |, this.mTabs.item(aDestIndex);|?
Yes it should, see comment 17, please fix.
Comment 35 jag (Peter Annema) 2006-01-23 16:59:22 PST
Bah, strict warnings. Ok, I'll fix that.
Comment 36 jag (Peter Annema) 2006-01-23 19:15:59 PST
[] -> .item() checked in.

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

Also checked in on 1_8 and 1_8_0:

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

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.126.2.4.2.7; previous revision: 1.126.2.4.2.6
done
Comment 37 Torsten Bondieck 2006-01-25 13:19:05 PST
thank you for fixes this bug :-)
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.1) Gecko/20060125 SeaMonkey/1.0

a little behaivor disagreement, i have still found.

In mozilla version before seamonkey release the tab i undo, return on selve position if i close. In seamonkey it won't work, here is always the first tab.

On page http://www.extensionsmirror.nl/index.php?showtopic=226
i read this "Tabs will be focused and moved to previous position if gBrowser.moveTabTo is available (included in Firefox 1.0+ and most miniT variants)." Is this gBrowser.moveTabTo not implemented?

I also install the tablib-for-seamonkey.xpi for SeaMonkey from http://mozilla.dorando.at/readme.html without success.
Comment 38 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-25 13:41:31 PST
(In reply to comment #37)
> I also install the tablib-for-seamonkey.xpi for SeaMonkey from
> http://mozilla.dorando.at/readme.html without success.
> 
tablib-for-seamonkey.xpi    19-Feb-2004 08:29     2k  

If it's really from 2004, it is not likely to work - we've made large changes since then.
Comment 39 Torsten Bondieck 2006-01-26 10:41:35 PST
(In reply to comment #38)
> tablib-for-seamonkey.xpi    19-Feb-2004 08:29     2k  
> 
> If it's really from 2004, it is not likely to work - we've made large changes
> since then.

o.k. but why work the extension undoclosetab not with function open on previous position, with SeaMonkey 1.0 at same firefox?
Comment 40 Torsten Bondieck 2006-01-26 10:53:43 PST
i have just now see, Netscape 8.1 actually have the function undoclosetab, see "When a tab is closed, focus to:" picture here http://scr3.golem.de/?d=0601/Netscape_8_1&a=42955&s=15

bye
Torsten
Comment 41 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-26 11:33:07 PST
(In reply to comment #39)
> (In reply to comment #38)
> > tablib-for-seamonkey.xpi    19-Feb-2004 08:29     2k  
> > 
> > If it's really from 2004, it is not likely to work - we've made large changes
> > since then.
> 
> o.k. but why work the extension undoclosetab not with function open on previous
> position, with SeaMonkey 1.0 at same firefox?
> 

Could you join our IRC chat in irc.mozilla.org #seamonkey and talk to one of our German-speaking developers?  If this bug is not properly fixed, we need to know immediately, but I can't understand your comments.
Comment 42 Torsten Bondieck 2006-01-26 12:08:43 PST
oh, is my english so bad?

is it possible, build in the function tabs moved to previous position with 'browser.moveTabTo' after close and reopen?
Comment 43 jag (Peter Annema) 2006-01-26 19:07:08 PST
Marking closed again.

I looked at the code for the undoclosetab extension, and the problem isn't with our implementation of |moveTabTo()|, it's with the extension itself:

eval("gBrowser.removeTab ="+gBrowser.removeTab.toString().replace(
'var oldTab = aTab;',
'this.closedTabs.push([aTab._tPos, oldBrowser.sessionHistory]); \  if(this.closedTabs.length > this.mPrefs.getIntPref("undoclosetab.cache")) this.closedTabs.shift(); \
 undoclosetab.cmd.removeAttribute("disabled"); \
 var oldTab = aTab;'));

This code expects aTab to have a _tPos property, which we don't have. This causes all the problems you see.

Could you contact the extension developer and ask them to do something like this before they remove the tab:

var tabPos = -1; // or maybe gBrowser.browsers.length?
if("_tPos" in aTab)
  tabPos = aTab._tPos;
else if ("getTabIndex" in gBrowser)
  tabPos = gBrowser.getTabIndex(aTab);

getTabIndex() was added when moveTabTo() was added, and you only need the index when you actually want to move the tab back to its old spot, so this code should work for both Firefox and SeaMonkey.
Comment 44 jag (Peter Annema) 2006-01-26 20:07:50 PST
s/closed/fixed/ ;-)
Comment 45 Torsten Bondieck 2006-01-27 10:34:21 PST
(In reply to comment #43)
> I looked at the code for the undoclosetab extension, and the problem isn't with
> our implementation of |moveTabTo()|, it's with the extension itself:

thank you very much for your fact finding, i contact the extension author!

Note You need to log in before you can comment on or make changes to this bug.