612577 - (TB_tab_animation) Animate Thunderbird tabs the same way we do Firefox's.

Status: REOPENED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[Andreas Nilsson (:andreasn)]

The tab transitions in Firefox trunk is really neat. We should do this in Thunderbird too.
Relevant bugs:
Opening tabs: bug 543206
Closing tabs: bug 380960

Comment 1

[Josiah Bruner [:jsbruner]]

In addition, I am going to make this bug a request for Firefox tab animations on Firefox, not just the opening and closing opens, but the sliding as well.

This is what is needed for a blogpost that was published that we are pushing.

===================

http://infinite-josiah.blogspot.com/2013/02/thunderbird-ui-concept.html

Point 8: Tabs should animate when re-arranged

===================

The best example of what we are going for is from the UX Trunk, in which Firefox is using more "Thunderbird style tabs".

I'm updating the blog post with a link here, and hopefully we'll get some momentum on this.

Comment 2

[Josiah Bruner [:jsbruner]]

Attachment: Current Thunderbird re-ordering.

Screenshot of tab re-ordering on TB.

Comment 3

[Josiah Bruner [:jsbruner]]

Attachment: Current Firefox tab re-ordering

Firefox's implementation of tab re-ordering.

Comment 4

[Josiah Bruner [:jsbruner]]

Attachment: UX branch's tab re-ordering.

Here's a screenshot of the UX-trunk's tab animation.

Comment 5

[Josiah Bruner [:jsbruner]]

Attachment: WIP

Basic implementation done, but more to do...

Comment 6

[Josiah Bruner [:jsbruner]]

Attachment: WIP 2

Very close to completion...

Comment 7

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

This seems pretty good now. There is some issues with the tab box scrolling but am dealing with that in the bug I just made this depend on, since it seems that is pretty much its own issue.

ui-review=Blake, review=Mike, and feedback=Richard for the CSS stuff.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 783956 [details] [diff] [review]
Patch.

Review of attachment 783956 [details] [diff] [review]:
-----------------------------------------------------------------

When I have three tabs open with the last as the active and I close this tab, then no other tab becomes active and the old content from closed tab is still shown. It should first switch to the left tab and then close the last tab (with the animation).

With only two tabs open there is no animation shown when I close the second tab. On FirefoxUX there is still an animation shown.

::: mail/base/content/tabmail.css
@@ +8,5 @@
>  
>  .tabmail-tab {
>    -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-tab");
> +  -moz-transition: min-width .2s ease-out, max-width .25s ease-out, opacity 50ms ease-out;
> +  transition-delay: 0s,0s, 20ms;

Please add a space after the comma.

Comment 9

[Josiah Bruner [:jsbruner]]

(In reply to Richard Marti [:Paenglab] from comment #8)
> Comment on attachment 783956 [details] [diff] [review]
> Patch.
> 
> Review of attachment 783956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When I have three tabs open with the last as the active and I close this
> tab, then no other tab becomes active and the old content from closed tab is
> still shown. It should first switch to the left tab and then close the last
> tab (with the animation).
> 
> With only two tabs open there is no animation shown when I close the second
> tab. On FirefoxUX there is still an animation shown.
> 
> ::: mail/base/content/tabmail.css
> @@ +8,5 @@
> >  
> >  .tabmail-tab {
> >    -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-tab");
> > +  -moz-transition: min-width .2s ease-out, max-width .25s ease-out, opacity 50ms ease-out;
> > +  transition-delay: 0s,0s, 20ms;
> 
> Please add a space after the comma.

Richard,

I can not reproduce the first issue you mentioned. I did have that problem on an older patch, but that was fixed. Are you using the right version? What platform are you on?

And Firefox does *not* show an animation if you remove the new tab button from the tab bar. Without this element the animation looks incomplete and odd. That is why I opted to remove the animation completely.

Comment 10

[Richard Marti (:Paenglab)]

This is with your patch for review on Win7. And yes without the new tab button Firefox UX closes without this animation.

Comment 11

[Josiah Bruner [:jsbruner]]

Looks like it is an issue when opening the chat tab and then the addon tab after. Not sure why, but I'll look into it...

Comment 12

[Josiah Bruner [:jsbruner]]

Turns out the issue has to do with the add-on tab always returning 0 when trying to use indexOf. Filed bug 900653 on the issue.

Comment 13

[Josiah Bruner [:jsbruner]]

Comment on attachment 783956 [details] [diff] [review]
Patch.

Killing the reviews at least until I can land a fix/workaround for bug 900653...

Comment 14

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

This should work properly now.

r = mconley for the JS.

ui-r = bwinton

f = Paenglab for the CSS

Note: For best results you may need the patch in bug 900139. Though it shouldn't be required.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

(As a side note, Paenglab or mconley, if you want to steal the ui-r, feel free to… ;)

Comment 16

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Updated for trunk and just going to let Richard have the ui-review.

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 794220 [details] [diff] [review]
Patch.

Review of attachment 794220 [details] [diff] [review]:
-----------------------------------------------------------------

F+ with my comments addressed.

Only one thing prevents me for a ui-r+: Without patch when I open a message in a tab and then open the Add-on manager from main tab and now close the active Add-on tab, the main tab becomes the active tab. With patch it is always the tab on the left (in this case the message tab).

I know without patch when multiple message tabs are open, closing a message tab let's activate every time the tab on the left. Please can you check if you could revert to the old behavior? And if you could expand this also to the message tabs. But this would probably a new bug.

::: mail/base/content/tabmail.css
@@ +7,5 @@
>  }
>  
>  .tabmail-tab {
>    -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-tab");
> +  -moz-transition: min-width .2s ease-out, max-width .25s ease-out, opacity 50ms ease-out;

Please can you use transition instead of the -moz one. Then we don't need to update when Gecko stops to support the old -moz version.

@@ +13,5 @@
> +  opacity: 1;
> +}
> +
> +.tabmail-tab[fadein="false"] {
> +  -moz-transition: none !important;

Here too.

Comment 18

[Joe Sabash [:JoeS1]]

(In reply to Richard Marti [:Paenglab] from comment #17)

> 
> Only one thing prevents me for a ui-r+: Without patch when I open a message
> in a tab and then open the Add-on manager from main tab and now close the
> active Add-on tab, the main tab becomes the active tab. With patch it is
> always the tab on the left (in this case the message tab).
> 
> I know without patch when multiple message tabs are open, closing a message
> tab let's activate every time the tab on the left. Please can you check if
> you could revert to the old behavior? And if you could expand this also to
> the message tabs. But this would probably a new bug.

Yes, I think this is the concept of the "Home tab"
I don't care much about returning to the last open tab. If you arrange your tabs properly, that could always be the first (main) tab as the default any time you close a tab.

Comment 19

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

This should fix what Richard mentioned except that message tabs don't follow it. Turns out that will need to be done in a separate bug.

Comment 20

[Richard Marti (:Paenglab)]

Comment on attachment 794414 [details] [diff] [review]
Patch.

Yeah, I like it. Maybe this could also go to comm-beta to let this the next ESR24 users enjoy.

Comment 21

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

There was an issue with closing tabs that weren't currently selected. Fixed. Carrying flags.

Comment 22

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Don't know what I was thinking, but the last thing I did didn't fix the issue. This does.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 795092 [details] [diff] [review]
Patch.

Review of attachment 795092 [details] [diff] [review]:
-----------------------------------------------------------------

So this is really cool, and it behaves pretty well, but there are some things I'd like to see.

Most importantly, we can't do the whole setTimeout thing. We have to use transitionend event listeners - that way, we *know* they'll run as soon as the transition finishes.

I also think we should make the transitions pref-offable, like Firefox does. This way, Thunderbird isn't forced to be janky on weakers systems.

Let me know if you have any questions on any of that. I really dig what this patch does - just a little hammering, and we'll get it through.

-Mike

::: mail/base/content/tabmail.css
@@ +7,5 @@
>  }
>  
>  .tabmail-tab {
>    -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-tab");
> +  transition: min-width .2s ease-out, max-width .25s ease-out, opacity 50ms ease-out;

Can all of these be in ms units?

@@ +8,5 @@
>  
>  .tabmail-tab {
>    -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-tab");
> +  transition: min-width .2s ease-out, max-width .25s ease-out, opacity 50ms ease-out;
> +  transition-delay: 0s, 0s, 20ms;

Same with these?

::: mail/base/content/tabmail.xml
@@ +58,5 @@
>      -     argument specifies a specific tab to be closed. It can be a tab index,
>      -     a tab info object, or a tab's DOM element. In case the second
>      -     argument is true, the closed tab can't be restored by calling
>      -     undoCloseTab().
> +    -     The third parameter is a boolean-type that will *not* animate on close

This is a bit wordy - maybe just,

"The third argument is optional, and should be set to false to skip animating the tab closing. This argument defaults to true."

@@ +65,3 @@
>      -     Please note, some tabs cannot be closed. Trying to close such tab,
>      -     will fail silently.
> +    - * finishCloseTab(aOptionalTabIndexInfoOrTabNode,aNoUndo):

If this should never be called, maybe it should be _finishCloseTab.

@@ +181,5 @@
>      -     might expect.  This allows you to place common logic code on the
>      -     tab type for use by multiple modes and to defer to it.  Any arguments
>      -     provided to the caller of tabmail.openTab will be passed to your
>      -     function as well, including background.
> +    -     The 'animate' parameter is optional, but will be set to 1 if not passed.

"The third argument is optional, and should be set to false to skip animating the tab opening. This argument defaults to true."

@@ +457,5 @@
>              let tabOpenFirstFunc = firstTab.mode.openFirstTab ||
>                                     firstTab.mode.tabType.openFirstTab;
>              tabOpenFirstFunc.call(firstTab.mode.tabType, firstTab);
>              this.setTabTitle(null);
> +            

trailing whitespace

@@ +727,3 @@
>          <body><![CDATA[
>  
> +            var animationTime = 250;

Hm, I've got a problem with this. This is fragile because if we ever decide to modify the animation time, we have to remember to do it in 2 places.

The better way is to add a transitionend event listener to the element that's transitioning: https://developer.mozilla.org/en-US/docs/Web/Reference/Events/transitionend

And be sure to remove the transitionend handler in the handler itself.

@@ +760,5 @@
> +                }
> +              }
> +            }
> +
> +            if (animate == 0) {

Booleans should be compared with true or false, not integers.

And in conditionals, you can just do:

if (animate) {
  // do animate-y thing
}

or

if (!animate) {
  // do a thing if we're not animating
}

Comment 24

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Alright. So this patch switches to using eventListeners instead of timers, and also adds an about:config pref ('mail.tabs.animate'), to disable/enable the feature. Along with some of the more minor things Mike commented on before. I also changed the 'animate' param to 'dontAnimate', which is a lot more clear I think.

Mike, do I need to write tests for this or no?

Comment 25

[Josiah Bruner [:jsbruner]]

Talked to Mike on IRC and we decided that I will go ahead and port the Firefox tests. Clearing needinfo.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 811756 [details] [diff] [review]
Patch.

Review of attachment 811756 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really, really good Josiah. Just a few more notes.

::: mail/base/content/tabmail.xml
@@ +451,5 @@
>  
>              this.tabInfo[0] = this.currentTabInfo = firstTab;
>  
> +            let [iTab, tab, tabNode] =
> +            this._getTabContextForTabbyThing(firstTab, true);

Indent this by two spaces, since it is a continuation.

@@ +475,5 @@
>        </method>
>        <method name="openTab">
>          <parameter name="aTabModeName"/>
>          <parameter name="aArgs"/>
> +        <parameter name="dontAnimate"/>

Arguments need to start with an "a", so let's make this "aNoAnimate" (since aDontAnimate looks strange with that contraction-minus-apostraphe in there)

@@ +604,5 @@
> +            // and then adjustTabstrip() again.
> +
> +            t.addEventListener("transitionend", function tabOpenTransitionFinished() {
> +              this.tabContainer.adjustTabstrip();
> +              t.removeEventListener("transitioned", tabOpenTransitionFinished(), false);

This won't remove the event listener for two reasons:

1) Needs to remove for "transitionend" not "transitioned"
2) This will try to remove what tabOpenTransitionFinished() returns (which is nothing).

So it should look like this:

t.removeEventListener("transitionend", tabOpenTransitionFinished);

(you don't need to supply the third "false" argument to add/removeEventListener, since it defaults to false).

@@ +720,5 @@
>        </method>
>        <method name="closeTab">
>          <parameter name="aOptTabIndexNodeOrInfo"/>
>          <parameter name="aNoUndo" />
> +        <parameter name="dontAnimate" />

aNoAnimate

@@ +731,4 @@
>                return;
> +            }
> +
> +            let tabNode = this.tabContainer.childNodes[iTab];

Wait, wait wait...remind me again why we need to overwrite tabNode? Didn't _getTabContextForTabbyThing give us the index (iTab) and the node (tabNode) already?

@@ +732,5 @@
> +            }
> +
> +            let tabNode = this.tabContainer.childNodes[iTab];
> +            let tabIndex = Array.indexOf(this.tabContainer.childNodes, tabNode);
> +            let selectedIndex = this.tabContainer.selectedIndex;

selectedIndex is only used in the scope of lines 750-752, as far as I can tell. Let's define it within that block.

@@ +733,5 @@
> +
> +            let tabNode = this.tabContainer.childNodes[iTab];
> +            let tabIndex = Array.indexOf(this.tabContainer.childNodes, tabNode);
> +            let selectedIndex = this.tabContainer.selectedIndex;
> +            let lastTabOpenerIndex = this.tabInfo.indexOf(this.mLastTabOpener);

lastTabOpenerIndex can probably be defined inside of the block started at 739.

@@ +735,5 @@
> +            let tabIndex = Array.indexOf(this.tabContainer.childNodes, tabNode);
> +            let selectedIndex = this.tabContainer.selectedIndex;
> +            let lastTabOpenerIndex = this.tabInfo.indexOf(this.mLastTabOpener);
> +
> +            if (selectedIndex != tabIndex) {

Instead of just tossing everything we want to do into the else block, let's just do:

if (selectedIndex == tabIndex) {
  // Do all of the things
}

@@ +740,5 @@
> +              // Don't do anything
> +            } else {
> +              if (this.mLastTabOpener && (lastTabOpenerIndex != -1)) {
> +                this.tabContainer.selectedIndex = this.tabInfo.indexOf(this.mLastTabOpener);
> +                } else {

Indentation is off here.

@@ +742,5 @@
> +              if (this.mLastTabOpener && (lastTabOpenerIndex != -1)) {
> +                this.tabContainer.selectedIndex = this.tabInfo.indexOf(this.mLastTabOpener);
> +                } else {
> +                if (tabIndex == (this.tabContainer.childNodes.length - 1)) {
> +                  tabNode.setAttribute("fadein", "false");

Why are we not animating this last tab?

@@ +743,5 @@
> +                this.tabContainer.selectedIndex = this.tabInfo.indexOf(this.mLastTabOpener);
> +                } else {
> +                if (tabIndex == (this.tabContainer.childNodes.length - 1)) {
> +                  tabNode.setAttribute("fadein", "false");
> +                  animationTime = 0;

animationTime is not defined or used anywhere else.

@@ +755,5 @@
> +
> +            if (dontAnimate || !Services.prefs.getBoolPref("mail.tabs.animate")) {
> +              tabNode.setAttribute("fadein", "false");
> +              this._finishCloseTab(aOptTabIndexNodeOrInfo, aNoUndo);
> +            } else {            

Trailing whitespace.

@@ +759,5 @@
> +            } else {            
> +              tabNode.removeAttribute("fadein");
> +              tabNode.addEventListener("transitionend", function tabCloseTransitionEnded(event) {
> +                if (event.propertyName === "max-width") {
> +                  tabNode.removeEventListener("transitioned", tabCloseTransitionEnded, false);

"transitionend", not "transitioned", and don't need third "false" arg.

This also won't remove the event listener because tabCloseTransitionEnded is not *exactly* what was passed to addEventListener. What was passed was

tabCloseTransitionEnded.bind(this).

You could get around this by using a closure - so, outside of the addEventListener call, define a variable "self" that points to "this", like this:

let self = this;
// stuff
tabNode.addEventListener("transitionend", function someFunc(aEvent) {
  tabNode.removeEventListener("transitionend", someFunc);
  self._finishCloseTab(blah, blah);
  // Blah blah
});

Ping me if you have questions about this stuff.

@@ +773,5 @@
> +        <parameter name="aNoUndo" />
> +        <body>
> +          <![CDATA[
> +            let [iTab, tab, tabNode] = this._getTabContextForTabbyThing(aOptTabIndexNodeOrInfo, true);
> +            Services.console.logStringMessage("finishCloseTab");

Please remove debug message

@@ +787,3 @@
>  
>              for each (let [i, tabMonitor] in Iterator(this.tabMonitors)) {
> +            if ("onTabClosing" in tabMonitor)

Please fix indentation of these two lines.

@@ +823,5 @@
>  
>                // Ensure current tab is still selecte and displayed in the
>                // panelContainer.
>                this.panelContainer.selectedPanel =
> +              this.currentTabInfo.panel || this.currentTabInfo.mode.tabType.panel;

Please revert indentation change.

@@ +828,3 @@
>              }
>              if (this.tabContainer.childNodes.length == 1 &&
> +                                this.tabContainer.mAutoHide) {

Please revert indentation change.

Comment 27

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Thanks Mike! Sorry this took so long, was really busy last week.
Anyway, here's the updated patch that addresses your feedback.

Comment 28

[Josiah Bruner [:jsbruner]]

(In reply to Mike Conley (:mconley) from comment #26)
> Why are we not animating this last tab?

Because fading and sliding a tab without any other tabs to follow behind it looks a little peculiar in my opinion. In addition, Firefox does not animate the last tab either. (You have to remove the new tab button to observe that though).

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 823367 [details] [diff] [review]
Patch.

Review of attachment 823367 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Josiah,

This looks great, and works really well on OS X. Testing on Windows and Linux next. I'm just concerned about one block of code being a little unclear. Might just need some documentation to clear up. See below.

-Mike

::: mail/base/content/tabmail.xml
@@ +64,4 @@
>      -     Please note, some tabs cannot be closed. Trying to close such tab,
>      -     will fail silently.
> +    _
> +    - * _finishCloseTab(aOptionalTabIndexInfoOrTabNode, aNoUndo):

I'm not even sure we should include this in the documentation since it's supposed to be a private method. I think we should just remove it from here - the text can be moved into the method definition itself, since the documentation is handy.

@@ +733,5 @@
> +
> +            let tabIndex = Array.indexOf(this.tabContainer.childNodes, tabNode);
> +            let selectedIndex = this.tabContainer.selectedIndex;
> +
> +            if (selectedIndex == tabIndex) {

I think we need to document these blocks to explain what's going on here, because it's not really obvious. I'm still kinda guessing.

@@ +739,5 @@
> +              if (this.mLastTabOpener && (lastTabOpenerIndex != -1)) {
> +                this.tabContainer.selectedIndex = this.tabInfo.indexOf(this.mLastTabOpener);
> +              } else {
> +                if (tabIndex == (this.tabContainer.childNodes.length - 1)) {
> +                  tabNode.setAttribute("fadein", "false");

So, this looks like the case where the selected tab is the one being closed, and we don't have a last tab opener, and the selected tab is the last tab. Why aren't we animating it closed?

@@ +741,5 @@
> +              } else {
> +                if (tabIndex == (this.tabContainer.childNodes.length - 1)) {
> +                  tabNode.setAttribute("fadein", "false");
> +                }
> +                if (selectedIndex > 0 && (tabIndex == selectedIndex)) {

if tabIndex == selectedIndex is superfluous, because this must have been true if we entered the block at 737.

@@ +782,5 @@
>  
>              for each (let [i, tabMonitor] in Iterator(this.tabMonitors)) {
>                if ("onTabClosing" in tabMonitor)
>                  tabMonitor.onTabClosing(tab);
> +              }

Indentation here is wrong.

Comment 30

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Applied feedback. Resetting review flag.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 827626 [details] [diff] [review]
Patch.

Let's do it! Thanks JosiahOne!

Comment 32

[Josiah Bruner [:jsbruner]]

https://hg.mozilla.org/comm-central/rev/49cccc8fa53c

\o/

Comment 33

[Josiah Bruner [:jsbruner]]

https://hg.mozilla.org/comm-central/rev/d4c74466ca81

Backed out for suspected test failures -> https://tbpl.mozilla.org/?tree=Thunderbird-Trunk&rev=49cccc8fa53c

Comment 34

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch.

Updated for trunk. Carrying review flags...

Comment 35

[Josiah Bruner [:jsbruner]]

ux-feature-wanted-31 -> ux-feature-wanted-38

Comment 36

[Jorg K (CEST = GMT+2)]

I've taken the liberty to cancel the remaining tasks on
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5e185f89a260ba2fa98e1b4467ccae6d859bbfe1
since there are heaps of Mozmill failures on Windows and Linux already.

Comment 37

[Richard Marti (:Paenglab)]

Attachment: updated AnimateTabs.patch

Patch updated to tip.

Aceman, the test expert, could you look what needs to be done that the tests passes?

Comment 38

[:aceman]

Sure, do you wait in all the tests until the animation finishes?

Sample failure:

function test_closeComposeWindowAndTab() {
  composeHelper.close_compose_window(gComposeWin);

  mc.tabmail.closeTab(gNewTab);

  if (mc.tabmail.tabContainer.childNodes.length != gPreCount)
    throw new Error("The content tab didn't close");
}

tabmail.closeTab() is called and immediately we check if number of number of tabs lowered.

Is the tab considered closed when .closeTab() returns? Is the animation just some css effect. It seem so, but maybe while the anumation is still there, the tab is in the tabContainer.childNodes.

We had to workaround this also for the notification box, where the notifications started animating and broke all tests that didn't wait for that. So we check for notificationbox._animating property.
Is there something similar for the tabs? You could also pass the aNoAnimate=true argument in the tests, but that is only if you open/close the tabs directly (using the tabmail methods), not e.g. as a result of a click/keypress.

Comment 39

[Richard Marti (:Paenglab)]

The tab animation is also steered by the toolkit.cosmeticAnimations.enabled pref. Disabling this one during the tests would also be allowed?

Comment 40

[:aceman]

Yes, try to put the variable in mail/test/mozmill/runtest.py ThunderTestProfile() so that all tests get it. We do not need any test to be slowed down any more by the animation.

Comment 41

[Richard Marti (:Paenglab)]

Attachment: Bug612577-testsDisableAnimations.patch

With disabled animations there are still test failures. See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=278180957ad49b9f09cb4a21377b30a5fcdd10ff.

Aceman, please can you look - and hopefully fix them?

Comment 42

[:aceman]

It seems some of the tests would pass when run standalone.
But they are affected by previous test fails.
The first failing test is:
SUMMARY-UNEXPECTED-FAIL | test-tabs-simple.js | test-tabs-simple.js::test_close_message_a
  EXCEPTION: tab is undefined
    at: tabmail.xml line 1239
       updateCurrentTab tabmail.xml:1239 15
       _finishCloseTab tabmail.xml:839 15
       closeTab tabmail.xml:773 15
       close_tab test-folder-display-helpers.js:861 3
       test_close_message_a test-tabs-simple.js:123 3
       Runner.prototype.wrapper frame.js:589 9
       Runner.prototype._runTestModule frame.js:659 9
       Runner.prototype.runTestModule frame.js:705 3
       Runner.prototype.runTestFile frame.js:538 3
       runTestFile frame.js:717 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:280 3
       AsyncRead.prototype.onDataAvailable server.js:88 3

It seems to me there may be some bug in the change to tabmail.closeTab() . Can you see if there were updates to this code in m-c since this patch was done (4 years ago)?

Comment 43

[Richard Marti (:Paenglab)]

I'm sorry, I can't say what changed in m-c as it looks they do this differently. No <method name="closeTab"> in tabbrowser.xml. I think there was already a bug in the change as it also broke tests 4 years ago. Josiah added a <method name="_finishCloseTab"> to finish the tab closing. Maybe this looses the tab and the test fails.

You know, I'm absolutely a noob in this and this is all guessing.