The default bug view has changed. See this FAQ.

Make SeaMonkey compatible again with moveTabTo implementation of the Firefox

RESOLVED FIXED

Status

SeaMonkey
UI Design
--
minor
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Torsten Bondieck, Assigned: jag (Peter Annema))

Tracking

(4 keywords)

Trunk
x86
Windows 2000
fixed-seamonkey1.0, fixed-seamonkey1.1a, fixed1.8.0.1, fixed1.8.1
Bug Flags:
blocking-seamonkey1.0 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

1.58 KB, patch
Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
: review+
neil@parkwaycc.co.uk
: superreview+
Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
: approval-seamonkey1.0+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
Assignee: general → guifeatures
Component: General → XP Apps: GUI Features
QA Contact: general
Summary: Make Seamonkey compatible again with moveTabTo implementation of the Firefox → Make SeaMonkey compatible again with moveTabTo implementation of the Firefox
I believe we broke compat because their API was nonintuitive.

Comment 2

11 years ago
OK, so what's the best approach here? Rename moveTabTo and provide a stub function that calls through to the real function?

Updated

11 years ago
Version: unspecified → Trunk
Flags: blocking-seamonkey1.0?
Assignee: guifeatures → cst
Severity: major → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-seamonkey1.0? → blocking-seamonkey1.0+
Created attachment 208228 [details] [diff] [review]
patch?

I can't test this.
Attachment #208228 - Flags: review?(mconnor)
Attachment #208228 - Flags: review?(jag)
(Assignee)

Comment 4

11 years ago
Love the JS loose typing. Take a look at the type of the first parameter and take it from there.
(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.
(Assignee)

Comment 6

11 years ago
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.
(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.
(Assignee)

Comment 8

11 years ago
      <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;
(Assignee)

Comment 9

11 years ago
Hmmm, wrapping is a bit odd there, but you get the idea.

Comment 10

11 years ago
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 on attachment 208228 [details] [diff] [review]
patch?

mconnor said he doesn't like this over IRC, and jag said he'd attach his version.
Attachment #208228 - Attachment is obsolete: true
Attachment #208228 - Flags: review?(mconnor)
Attachment #208228 - Flags: review?(jag)

Comment 12

11 years ago
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?
(Assignee)

Comment 13

11 years ago
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.
Attachment #208715 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #208715 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #208715 - Flags: review? → review?(cst)
Attachment #208715 - Flags: review?(cst) → review+

Comment 14

11 years ago
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 ;-)
Attachment #208715 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
(Assignee)

Comment 15

11 years ago
Created attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore
Attachment #209046 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 16

11 years ago
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.
Attachment #209048 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 17

11 years ago
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
Attachment #209048 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

Comment 18

11 years ago
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 >=
Attachment #209046 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
(Assignee)

Comment 19

11 years ago
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

11 years ago
So does foo.insertBefore(bar, bar); work? It still looks wrong...
(Assignee)

Comment 21

11 years ago
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

11 years ago
(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
(Assignee)

Comment 23

11 years ago
(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

11 years ago
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

OK, sr=me with >= as noted.
Attachment #209046 - Flags: superreview- → superreview+
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 ;)
Attachment #209046 - Flags: review+
Attachment #209046 - Flags: approval-seamonkey1.1?
Attachment #209046 - Flags: approval-seamonkey1.0?
(Assignee)

Comment 26

11 years ago
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.
Assignee: cst → jag
Attachment #208715 - Attachment is obsolete: true
Attachment #209046 - Attachment is obsolete: true
Attachment #209048 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #209313 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209313 - Flags: review?
Attachment #209046 - Flags: approval-seamonkey1.1?
Attachment #209046 - Flags: approval-seamonkey1.0?
(Assignee)

Updated

11 years ago
Attachment #209313 - Flags: review? → review?(cst)
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);|?
Attachment #209313 - Flags: review?(cst) → review+
(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?
(Assignee)

Comment 29

11 years ago
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.
(Assignee)

Comment 30

11 years ago
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.
(Assignee)

Comment 31

11 years ago
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.
Attachment #209046 - Attachment is obsolete: false
Attachment #209046 - Flags: approval-seamonkey1.1?
Attachment #209046 - Flags: approval-seamonkey1.0?
(Assignee)

Updated

11 years ago
Attachment #209313 - Attachment is obsolete: true
Attachment #209313 - Flags: superreview?(neil.parkwaycc.co.uk)
(Assignee)

Comment 32

11 years ago
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
Assignee: jag → guifeatures
Status: ASSIGNED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

11 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

11 years ago
Assignee: guifeatures → jag
Status: ASSIGNED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 33

11 years ago
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
Attachment #209046 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+
Attachment #209046 - Flags: approval-seamonkey1.0? → approval-seamonkey1.0+

Comment 34

11 years ago
(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.
(Assignee)

Comment 35

11 years ago
Bah, strict warnings. Ok, I'll fix that.
(Assignee)

Comment 36

11 years ago
[] -> .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
Keywords: fixed1.8.0.1, fixed1.8.1
Whiteboard: fixed-seamonkey1.0
(Reporter)

Comment 37

11 years ago
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.
(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.
(Reporter)

Comment 39

11 years ago
(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?
(Reporter)

Comment 40

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(Reporter)

Comment 42

11 years ago
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?
(Assignee)

Comment 43

11 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 44

11 years ago
s/closed/fixed/ ;-)
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
(Reporter)

Comment 45

11 years ago
(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!

Updated

11 years ago
Keywords: fixed-seamonkey1.1a

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.