890181 - Test failure "Tab has been closed" when trying to close a tab with close button

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Description

[Daniela Petrovici]

This was discovered while investigating bug 880135. Steps performed by the test are:
- Open a new tab through the menu
- Wait for tab length to be 2
- Close the tab

RESULT: Tab should be closed, but it isn't

NOTE: I will attach a minimized test case for this issue. The bug reproduces with both mozmill 1.5 and mozmill 2.0

The issue does not seem to reproduce if we wait for display CSS Property to be !== "none".
Also checked attributes and JS Properties, but they are the same before and after we click on the close tab button

Comment 1

[Daniela Petrovici]

Attachment: minimized test case

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Because tab is either not displayed yet or is still animating we have to wait for property _fullyOpen to become true, I also reversed the assertions in testTabbedBrowsing/testCloseTab.js for the bug to be reproducible in tests.

http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ef12727
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ef12316
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ef0f093

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 794016 [details] [diff] [review]
patch_v1.0

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

Interesting and good catch. But I'm indifferent in using a private property of the tabbrowser object. Can you please ask Dao Gottwald for his input on this? Thanks.

::: lib/tabs.js
@@ +195,5 @@
>      // Disable tab closing animation for default behavior
>      prefs.preferences.setPref(PREF_TABS_ANIMATE, false);
>  
> +    assert.waitFor(function () {
> +     return this.getTab().getNode()._fullyOpen === true;

No need to compare to '=== true' if the value is already a boolean.

@@ +196,5 @@
>      prefs.preferences.setPref(PREF_TABS_ANIMATE, false);
>  
> +    assert.waitFor(function () {
> +     return this.getTab().getNode()._fullyOpen === true;
> +    }, "The tab has been loaded", undefined, undefined, this);

I would propose you define ´var self = this´ above, and not make use of the last parameter. We failed a couple of times in the past on that.

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +29,5 @@
>      return tabBrowser.length === 5;
>    }, "5 tabs have been opened");
>  
> +  // Close a tab via the close button on the tab itself
> +  tabBrowser.closeTab("closeButton");

Why have you exchanged 'shortcut' and 'closeButton'? I don't see why that is necessary.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Hi Dao,  would it be ok to wait for tab's property _fullyOpen, before closing it?
The issue is that if we hit over close button while animating it will catch only the mousedown event. 
Can we determine in other way if the animation has ended ?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Cosmin Malutan from comment #4)
> Hi Dao,  would it be ok to wait for tab's property _fullyOpen, before
> closing it?

No, it's an implementation detail, you shouldn't depend on it.

> The issue is that if we hit over close button while animating it will catch
> only the mousedown event. 
> Can we determine in other way if the animation has ended ?

You could listen to the transitionend event.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Comment on attachment 794016 [details] [diff] [review]
> ::: tests/functional/testTabbedBrowsing/testCloseTab.js
> @@ +29,5 @@
> >      return tabBrowser.length === 5;
> >    }, "5 tabs have been opened");
> >  
> > +  // Close a tab via the close button on the tab itself
> > +  tabBrowser.closeTab("closeButton");
> 
> Why have you exchanged 'shortcut' and 'closeButton'? I don't see why that is
> necessary.
I exchanged them because I wanted to test closing the tab with close button first so the issue to be reproducible in testrun.

I added an sleep of 0 after the tab has been opened so it will spin the event loop, and the tab will be updated.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19c7c5b1
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19c65bfb
http://mozmill-crowd.blargon7.com/#/functional/report/6d53b7d749a60e65551b4b1d19c58685

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 795987 [details] [diff] [review]
patch_v2.0

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

::: lib/tabs.js
@@ +439,5 @@
>      }
>      finally {
>        this._controller.window.removeEventListener("TabOpen", checkTabOpened, false);
>        prefs.preferences.clearUserPref(PREF_TABS_ANIMATE);
> +      this._controller.sleep(0);

sleep(0) should never be used if other alternatives exist. Why has the suggestion from Dao not been implemented?

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +28,5 @@
>    assert.waitFor(function () {
>      return tabBrowser.length === 5;
>    }, "5 tabs have been opened");
>  
> +  // Close a tab via the close button on the tab itself:

If it should also cover that situation in the future you should add a bit more content to the comment and explain why the button has to be used first for the test.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Comment on attachment 795987 [details] [diff] [review]
> patch_v2.0
> 
> Review of attachment 795987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/tabs.js
> @@ +439,5 @@
> >      }
> >      finally {
> >        this._controller.window.removeEventListener("TabOpen", checkTabOpened, false);
> >        prefs.preferences.clearUserPref(PREF_TABS_ANIMATE);
> > +      this._controller.sleep(0);
> 
> sleep(0) should never be used if other alternatives exist. Why has the
> suggestion from Dao not been implemented?
 I couldn't get it working, because I couldn't attach the event listener on tab. I will try again to implement Dao's suggestion or find something else.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

I wait for close button visibility and display properties to be ready before we click on it.
I also close the tab with close button first in testTabbedBrowsing/testCloseTab.js, so the issue would be reproducible in testrun.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1d6edad
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1d6d8b2
http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a1d6e375

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 798757 [details] [diff] [review]
patch v3.0

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

Why is the review only up for me? We also have Andreea and Andrei who can help to get this reviewed earlier. Please think about that in the future.

::: lib/tabs.js
@@ +209,5 @@
> +        assert.waitFor(function () {
> +          var cs = buttonNode.ownerDocument.defaultView.getComputedStyle(buttonNode,
> +                                                                         null);
> +          return cs.visibility === "visible" && cs.display !== "none";
> +        }, "Close button is available");

My question still stands, why does the transistionend event not work? You didn't give any details about that but came up with a workaround by checking the CSS state.

I don't like that path compared to a way cleaner event handling method.

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +28,5 @@
>    assert.waitFor(function () {
>      return tabBrowser.length === 5;
>    }, "5 tabs have been opened");
>  
> +  // Close a tab via the close button on the tab itself:

Also I have mentioned that the comment has to be updated, so we are getting reminded why that order is important. I don't see an update for that.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: minimizedTC.js

(In reply to Henrik Skupin (:whimboo) from comment #10)
> My question still stands, why does the transistionend event not work? You
> didn't give any details about that but came up with a workaround by checking
> the CSS state.
> 
> I don't like that path compared to a way cleaner event handling method.

Hi Henrik, "transitionend" event get's fired only if pref browser.tabs.animate is set to true if is set to false the state of button will be updated asynchronous, so we have to add a sleep of 0 before we click on it 
	http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1466
if we want to avoid this to we have to wait for close button display and visibility properties as is done at 
	http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/button.xml#86

In this new minimized test case, it shows that it passes if we animate the tabs and wait for transitionend event, or if we don't animate them but we add a sleep of 0 or if we wait for close button css state.

I will come with a patch where we wait for transitionend event shortly.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Cosmin Malutan from comment #11)
> In this new minimized test case, it shows that it passes if we animate the
> tabs and wait for transitionend event, or if we don't animate them but we
> add a sleep of 0 or if we wait for close button css state.

We disabled animation because of failures in detecting the opening and closing of tabs. If that transitionend event fixes it if animation is enabled, then lets do it! This is closer to the user settings.

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

Reports : 
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c1aec91
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c1b8a0d
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c1b05ac
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c1b8394
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c1d0545
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c1bfd82

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.1

I added an indentation.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 806635 [details] [diff] [review]
patch v4.1

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

::: lib/tabs.js
@@ +435,4 @@
>  
>      try {
>        assert.waitFor(function () {
> +        return self.opened && self.transionend;

Do we really still need the TabOpen event?

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

I would keep the TabOpen event to, to be sure that a new tab has been indeed opened and not just a tab has been moved/dragged/closed etc.

Comment 17

[Andrei Eftimie]

Comment on attachment 806635 [details] [diff] [review]
patch v4.1

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

Shouldn't we also do the same for closing a tab?
Please check if removing the preference, and setting the same observer help us there as well.

I feel we should handle opening and closing the tabs in a similar fashion.

::: lib/tabs.js
@@ +412,2 @@
>      // Add event listener to wait until the tab has been opened
> +    var self = { opened: false, transionend: false };

typo: transitioned, transitionend ?

Check all instances :)

@@ +435,4 @@
>  
>      try {
>        assert.waitFor(function () {
> +        return self.opened && self.transionend;

I actually like that we wait for both. Should be more robust/reliable

@@ +496,5 @@
>      }
> +    // When the animation ends it sets the flag to true
> +    this._tabs.getNode().addEventListener("transitionend", function () {
> +      self.transionend = true;
> +    }, false);

Make this a named function so you can remove the eventListener once we finish up, similar to checkTabOpened

Comment 18

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.2

(In reply to Andrei Eftimie from comment #17)
> Comment on attachment 806635 [details] [diff] [review]
> patch v4.1
> 
> Review of attachment 806635 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't we also do the same for closing a tab?
> Please check if removing the preference, and setting the same observer help
> us there as well.
No it doesn't,  no matter how I try to implement this for closing the tab I get some weird errors.
(it passes in some testes and fails in others, and it would take to much time to investigate it properly)
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cff8a22
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf529e8


Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee69721902941b
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee6972190217f7
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219019781
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee69721901934f
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219026568
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee69721901a46e

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Cosmin Malutan from comment #18)
> No it doesn't,  no matter how I try to implement this for closing the tab I
> get some weird errors.
> (it passes in some testes and fails in others, and it would take to much
> time to investigate it properly)

I don't tread it as excuse not doing an investigation here. We always prefer consistency as close as current users are operating. So it will be very helpful to also have the animation for closing a tab. If it cannot be implemented in this bug, we might want to spun off a new one. But please don't say it takes too much time to investigate and stop working on it. Thanks.

Comment 20

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> I don't tread it as excuse not doing an investigation here. We always prefer
> consistency as close as current users are operating. So it will be very
> helpful to also have the animation for closing a tab. If it cannot be
> implemented in this bug, we might want to spun off a new one. But please
> don't say it takes too much time to investigate and stop working on it.
> Thanks.

I had failures in different tests  when implementing waiting for animation after closing a tab. I filed bug 921445 for implementing this to don't block this bug.

Comment 21

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 808587 [details] [diff] [review]
patch v4.2

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

::: lib/tabs.js
@@ +427,5 @@
>          break;
>        default:
>          throw new Error(arguments.callee.name + ": Unknown event type - " + type);
>      }
> +    function checkAnimationEnded() { self.transioned = true; }

transioned?

@@ +429,5 @@
>          throw new Error(arguments.callee.name + ": Unknown event type - " + type);
>      }
> +    function checkAnimationEnded() { self.transioned = true; }
> +    // When the animation ends it sets the flag to true
> +    this._tabs.getNode().addEventListener("transitionend", checkAnimationEnded, false);

Blank lines before function checkAnimationEnded() and always comments have blank line above them.

@@ +495,5 @@
>          throw new Error(arguments.callee.name + ": Unknown event type - " + type);
>      }
> +    function checkAnimationEnded() { self.transioned = true; }
> +    // When the animation ends it sets the flag to true
> +    this._tabs.getNode().addEventListener("transitionend", checkAnimationEnded, false);

Same applies here.

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +30,5 @@
>    }, "5 tabs have been opened");
>  
> +
> +  // We close the tab via close button first so bug 890181 will be reproducible
> +  // in testrun

I would like more "Closing the tab via close button first makes bug 890181 reproducible in testrun"

@@ +36,3 @@
>    assert.waitFor(function () {
>      return tabBrowser.length === 4;
> +  }, "One tab has been closed using the close button");

Let's be consistent in all messages for assert.waitFor in using either "via" or "using". I see "via" is mainly used.

@@ +58,5 @@
>  
>  /**
>   * Map test functions to litmus tests
>   */
>  // testOpenInBackgroundTab.meta = {litmusids : [8094]};

Please also remove this or file a separate bug for our contributors to do so as we no longer use litmus. We still have many tests using litmusids.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: p

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.3

Here is the updated patch.
Thanks

Comment 24

[Andrei Eftimie]

Comment on attachment 812516 [details] [diff] [review]
patch v4.3

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

Only 2 things left and I think we can land this.

::: lib/tabs.js
@@ +431,5 @@
>  
> +    function checkAnimationEnded() { self.transitioned = true; }
> +
> +    // When the animation ends it sets the flag to true
> +    this._tabs.getNode().addEventListener("transitionend", checkAnimationEnded, false);

The 3rd parameter used here `userCapture` will default to false if not specified.
You can safely remove it.
(see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener )

Please also check other instances where this is the case.

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +56,5 @@
>  
>  /**
>   * Map test functions to litmus tests
>   */
>  // testOpenInBackgroundTab.meta = {litmusids : [8094]};

I see you haven't removed this as asked in the last review.

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.4

(In reply to Andrei Eftimie from comment #24)
> The 3rd parameter used here `userCapture` will default to false if not
> specified.
> You can safely remove it.
I removed it.

> >  // testOpenInBackgroundTab.meta = {litmusids : [8094]};
> 
> I see you haven't removed this as asked in the last review.
I filed bug 922587 for this.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 812600 [details] [diff] [review]
patch_v4.4

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

Pushed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/7ed552391d0a (default)

Please set the affected flags.

Comment 27

[Andrei Eftimie]

Backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/b3f578c0111d (default)

See bug 923645 for more information.
I'm guessing this was to blame:
http://hg.mozilla.org/qa/mozmill-tests/rev/b3f578c0111d#l1.82

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v5.0

So the failures on endurance tests for which my previous patch where responsible, happened because after we open enough tabs tabs will start to scroll and they don't have an animation anymore when opening a new one.
So as a solution for this I added an mutation observer on tabs scroll-down-button, and when it's visible I don't wait for animation anymore.
I runned endurance and functional testruns without failures:

Reports:
http://mozmill-crowd.blargon7.com/#/endurance/report/ec449c026814fd64783d738c02966951
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c029a4e2f
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c029951ed
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c02995fdc
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c029a6dea
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c0299eb20
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c029938a1

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 817173 [details] [diff] [review]
patch v5.0

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

Good work with the MutationObserver. Seems to be the right way. But I still have a couple of comments on the current implementation.

::: lib/tabs.js
@@ +84,5 @@
>   */
>  function tabBrowser(controller) {
>    this._controller = controller;
>    this._tabs = this.getElement({type: "tabs"});
> +  this._animated = true;

Using the past tense for boolean flags is somewhat misleading. Also I would prefer to see a 'is' or 'active' prefix which indicates that it is a flag.

@@ +87,5 @@
>    this._tabs = this.getElement({type: "tabs"});
> +  this._animated = true;
> +
> +
> +  // After tabs become scrollable they don't animate anymore, so we add an mutation observer

nit: just 'a mutation'.

@@ +88,5 @@
> +  this._animated = true;
> +
> +
> +  // After tabs become scrollable they don't animate anymore, so we add an mutation observer
> +  // to set the animated boolean so we now if we wait for transitioned event

nit: s/boolean/flag and 'know if we have to wait'

@@ +98,5 @@
> +    mutations.forEach(function (mutation) {
> +      self._animated = mutation.target.hasAttribute('collapsed');
> +    });
> +  });
> +  obs.observe(scrollButtonDown.getNode(), config);

I think that we leak memory here given that the observer will not be released. So I don't think we can make this global for the whole class.

@@ +204,5 @@
>     *   Index of the tab to close (only used for middleClick)
>     */
>    closeTab : function tabBrowser_closeTab(aEventType, aIndex) {
>      var type = aEventType || "menu";
> +    var index = aIndex || this.selectedIndex;

Please don't revert this late change.

@@ +432,2 @@
>  
> +    var context = this;

Please define this where it is used.

@@ +432,4 @@
>  
> +    var context = this;
> +    if (this._animated) {
> +      // When the animation ends it sets the flag to true

the comment is not necessary

@@ +434,5 @@
> +    if (this._animated) {
> +      // When the animation ends it sets the flag to true
> +      function checkAnimationEnded() {
> +        self.transitioned = true;
> +      }

Please define this method outside of the if condition.

@@ +438,5 @@
> +      }
> +      this._tabs.getNode().addEventListener("transitionend", checkAnimationEnded, false);
> +    }
> +
> +    function checkTabOpened() { self.opened = true; }

Hm, instead of defining a dozen of callback methods lets define a single one which has a switch on the event type.

@@ +462,5 @@
>        assert.waitFor(function () {
> +        // It wait for length property to get updated otherwise colseAllTabs method
> +        // will be called twice sometimes, and transionend will not trigger second time
> +        if (context._animated) {
> +          return self.opened && self.transitioned && (self.length !== context.length);

Please check your spelling and grammar of the comment. Also I don't see why closeAllTabs can be called multiple times. Please explain.

@@ +467,5 @@
> +        }
> +        else {
> +          return self.opened;
> +        }
> +      }, "New tab has been opened", 10000);

Why are you adding a hard-coded value here, which is even not the default timeout we use across the place?

Comment 31

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v6.0

(In reply to Henrik Skupin (:whimboo) from comment #29)
> 
> @@ +98,5 @@
> > +    mutations.forEach(function (mutation) {
> > +      self._animated = mutation.target.hasAttribute('collapsed');
> > +    });
> > +  });
> > +  obs.observe(scrollButtonDown.getNode(), config);
> 
> I think that we leak memory here given that the observer will not be
> released. So I don't think we can make this global for the whole class.
https://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#garbage-collection
Because registered observers have a weak reference to the nodes, theirs nodes will be handled by the Garbage-Collector when the only remained reference is to observers(Here when we close the browser).  

> @@ +462,5 @@
> >        assert.waitFor(function () {
> > +        // It wait for length property to get updated otherwise colseAllTabs method
> > +        // will be called twice sometimes, and transionend will not trigger second time
> > +        if (context._animated) {
> > +          return self.opened && self.transitioned && (self.length !== context.length);
>  Also I don't see why closeAllTabs can be called multiple times. Please explain.
When tabs are animated the tabs.length doesn't get updated right away, because the animated tab gets animated/moved outside of visible space (here we get the transitionend event) and only then removed. 
Method closeAllTabs() will call closeTab() when tabs.length > 1, if the tabs length gets updated later (after we closed tab 2), we are calling closeTab() when we have only one tab, resulting in failures.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7d483983
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7d48349c
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7d48509f

Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/f54ca65f73fd56845c30b7ab7d46dc77
http://mozmill-crowd.blargon7.com/#/endurance/report/f54ca65f73fd56845c30b7ab7d46bd76

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Cosmin Malutan from comment #31)
> When tabs are animated the tabs.length doesn't get updated right away,
> because the animated tab gets animated/moved outside of visible space (here
> we get the transitionend event) and only then removed. 

So if that is the case and the length gets updated at the very last, why do we need transitionend at all? Why can't we still only listening for the TabClose event and the number of tabs decreased?

> Method closeAllTabs() will call closeTab() when tabs.length > 1, if the tabs
> length gets updated later (after we closed tab 2), we are calling closeTab()
> when we have only one tab, resulting in failures.

You thinking is wrong here. closeAllTabs() calls closeTab(), that's true. But closeTab() should never return before the tab has been closed. That's what the method is for. So that doesn't play into account here.

Comment 33

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #32)
> So if that is the case and the length gets updated at the very last, why do
> we need transitionend at all? Why can't we still only listening for the
> TabClose event and the number of tabs decreased?
That should work but only for closing tabs. So I will refactor that.
For OpwtTab we still have to animate because the tab gets added and only then animated in to visible space.

> You thinking is wrong here. closeAllTabs() calls closeTab(), that's true.
> But closeTab() should never return before the tab has been closed. That's
> what the method is for. So that doesn't play into account here.
That's why we had to wait for length to change when closing a tab.

Comment 34

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v6.1

>So if that is the case and the length gets updated at the very last, why do
> we need transitionend at all? Why can't we still only listening for the
> TabClose event and the number of tabs decreased?

I made the changes, and now we wait for closing tabs only for TabClose event and the number of tabs.
We still need transitionend event for opening a new tab.
I tested the patch again on all platforms running endurance and functional testruns. 

Reports
http://mozmill-crowd.blargon7.com/#/endurance/report/f54ca65f73fd56845c30b7ab7d7aaefa
http://mozmill-crowd.blargon7.com/#/endurance/report/f54ca65f73fd56845c30b7ab7d7ce32c
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7d6a2c29
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7d69c12a
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7d69e314

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Cosmin Malutan from comment #34)
> We still need transitionend event for opening a new tab.

Why? An explanation is always helpful as only giving a statement. Thanks.

Comment 36

[Cosmin Malutan, [:cosmin-malutan]]

Because, when we open a new tab, the tab node gets added to the tabs and only then is moved into the visible space, where we can take action on it(click the close button for instance). So the length property will be updated first in that situation and only after that we will have the transitionend event.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So I don't understand why we have to compare the length property then. If we are waiting for the OpenTab event we know that a new tab has been opened, and with 'transitionend' we know it is ready to work with. With the above explanation I see the length checks as obsolete.

Comment 38

[Andrei Eftimie]

Comment on attachment 819034 [details] [diff] [review]
patch_v6.1

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

I would still like better organisation of the animationObserver methods/properties, se more inline.

In regards to Henrik's comment:
I see we do only check for `length` when closing tabs. This should lead to simpler code, and should be reliable.
And no reference to length when opening, there we wait for it to be `open` and we wait for the `transition` to end when transitions are used.

::: lib/tabs.js
@@ +228,4 @@
>        assert.waitFor(function () {
> +        // When removing a tab it animates outside of visible space ond only the
> +        // it's being removed, so the length property gets updated the latest
> +        return self.closed && (self.length !== context.length);

You mention that the length is changed last.
In the case of closing a tab, wouldn't it suffice to wait for the length property?

We only need to store the length at the start of the method.
Then wait for it to decrease by 1.

This should be simple and reliable.

@@ +349,5 @@
>    /**
> +   * Initialisation of tabs observer, wo will set "isAnimated" flag when
> +   * scroll-button collapses
> +   */
> +  initObserver : function tabBrowser_initObserver() {

This method is still a bit confusing.
I like that we now grouped all functionality together, but we can still better organise it:

I would create an object, not a function. Not sure how to call it.
`animationObserver` might work, but the whole object is not an observer per se.
Let this object have its init method.
Bind the isAnimated flag to the parent object as well.
Bint the observe() and disconnect() methods to the parent object as well.

This way:
- all related methods and properties will be held together in the same 'namespace' object
- we'll have a better architecture of this part (now we have all of them declared in the init() function which is not very clean
- a cleaner API:

tabbrowser.animationObserver.init()
tabbrowser.animationObserver.isAnimated
tabbrowser.animationObserver.observe()
tabbrowser.animationObserver.disconnect()

@@ +353,5 @@
> +  initObserver : function tabBrowser_initObserver() {
> +    var scrollButtonDown = this.getElement({type: "tabs_scrollButton", subtype: "down"});
> +    var config = { attributes: true, attributeFilter: ["collapsed"]};
> +
> +    this.animationObserver = {      

nit: space

@@ +488,5 @@
> +          return self.opened && self.transitioned;
> +        }
> +        else {
> +          return self.opened;
> +        }

You could rewrite this with a ternary operator:
> return self.opened &&
>        (context.animationObserver.isAnimated) ? self.transitioned : true;

Comment 39

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.0

I updated the patch.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/95c9c51e8c2d9f14917fc749dd61ee15
http://mozmill-crowd.blargon7.com/#/functional/report/95c9c51e8c2d9f14917fc749dd61fbb6
http://mozmill-crowd.blargon7.com/#/functional/report/95c9c51e8c2d9f14917fc749dd61e775

Comment 40

[Andrei Eftimie]

Comment on attachment 822185 [details] [diff] [review]
patch_v7.0

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

This looks really good now.

Just one more update and we can land this, we can still simplify some code, and have a short list of nits to fix.

Thanks Cosmin.

::: lib/tabs.js
@@ +219,5 @@
> +    var self = this;
> +    assert.waitFor(function () {
> +      // When removing a tab it animates outside of visible space ond only after that
> +      // it's being removed, so the length property gets updated last
> +      return length !== self.length;

Two things here:
1) Please move the comment right before the whole block
2) We should not only check that the lenghts are different but validate that we closed 1 and only 1 tab.
The check should be: 
> return self.length === length - 1;

@@ +337,5 @@
>    /**
> +   * Initialisation of tabs observer, who will set "isAnimated" flag when
> +   * scroll-button collapses
> +   */
> +  animationObserver : {

Please move this further up so we preserve alphabetical ordering (just after the getters and setters).

@@ +338,5 @@
> +   * Initialisation of tabs observer, who will set "isAnimated" flag when
> +   * scroll-button collapses
> +   */
> +  animationObserver : {
> +    _obs: {

You don't need to initialize this here, as you are overwriting _obs in init()

@@ +350,5 @@
> +     *        Element to observe
> +     */
> +    init: function (aElement) {
> +      var win = aElement.getNode().ownerDocument.defaultView;
> +      var self =  this;

nit: 2 spaces

@@ +360,5 @@
> +      this._obs.element = aElement;
> +      this.observe();
> +    },
> +    // Public method to start observing
> +    observe: function () {

These wrappers have no real use, you can call these directly:
> this._obs.observe(aElement.getNode(),
>                   {attributes: true, attributeFilter: ["collapsed"]});

And since _obs is not really private, we can drop the underscore.

@@ +444,2 @@
>  
> +    function flagsSetterCallback(e) {

I still don't like this at all.
We're overcomplicating things here.

Instead of 11 lines of code we could have just two 1-line functions, one for each event.
> function tabOpenCallback() { self.opened = true; }
> function transitionEndCallback() { self.transitioned = true; }

I'll ask Henrik's opinion here since he requested to have 1 function with a switch.

@@ +567,4 @@
>        }, "New tab has been opened");
>      }
>      finally {
> +      this._controller.window.removeEventListener("TabOpen", flagsSetterCallback);

nit: for consistency, can we please call this "tabOpen", and the other event should then be called "transitionEnd" (in both functions)

Comment 41

[Andrei Eftimie]

Henrik, I'd like your input here (I've also noted this in my last review).

You requested in comment 29:
> Hm, instead of defining a dozen of callback methods lets define a single one which has a
> switch on the event type.

But code-wise right now we have:
> function flagsSetterCallback(e) {
>   switch (e.type) {
>     case "TabOpen":
>       self.opened = true;
>       break;
>     case "transitionend":
>       self.transitioned = true;
>       break;
>     default:
>       break;
>   }
> }

Which looks really clumsy to do for 2 events.
I would still like it much better to have 2 separate functions here, it would look like this:
> function tabOpenCallback() { self.opened = true; }
> function transitionEndCallback() { self.transitioned = true; }

But I'll leave it to 1 function with a switch if y ou feel strongly about it.
(If we're ever going to need lots of callback functions here, that would be the way to go, but for 2 I find it is an overkill)

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 822185 [details] [diff] [review]
patch_v7.0

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

::: lib/tabs.js
@@ +85,5 @@
>  function tabBrowser(controller) {
>    this._controller = controller;
>    this._tabs = this.getElement({type: "tabs"});
> +  this.animationObserver.init(this.getElement({type: "tabs_scrollButton",
> +                                               subtype: "down"}));

nit: please add a blank line before.

@@ +337,5 @@
>    /**
> +   * Initialisation of tabs observer, who will set "isAnimated" flag when
> +   * scroll-button collapses
> +   */
> +  animationObserver : {

Why has this to be part of the class itself? I would expect to see it globally declared.

@@ +444,2 @@
>  
> +    function flagsSetterCallback(e) {

Well, so lets leave it as is and make sure we get bug 808586 fixed anytime soon.

Comment 43

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.1

Here are the latest changes.
I moved the observer object in to the global space, and I fixed the nits.
Thanks

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8e8ae0a
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8e8a0af
http://mozmill-crowd.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8e8bcb9

Comment 44

[Andrei Eftimie]

Comment on attachment 825162 [details] [diff] [review]
patch_v7.1

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

The patch doesn't apply fully anymore, please update it.

::: lib/tabs.js
@@ +30,5 @@
> +        self.isAnimated = mutation.target.hasAttribute("collapsed");
> +      });
> +    });
> +    this.obs.element = aElement;
> +    this.obs.observe(this.obs.element.getNode(),

Why aren't you calling here directly `aElement.getNode()`?

I don't think we need to cache a reference to the used Element,
if we'll ever need that, we'll implement it then.

Comment 45

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.2

I updated the patch.

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 825796 [details] [diff] [review]
patch_v7.2

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

::: lib/tabs.js
@@ +11,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +/**
> + * Initialisation of tabs observer, who will set "isAnimated" flag when

s/who/which/

@@ +15,5 @@
> + * Initialisation of tabs observer, who will set "isAnimated" flag when
> + * scroll-button collapses
> + */
> +var animationObserver = {
> +  isAnimated: true,

isAnimated or hasAnimated? I would prefer the latter if we are setting it to true after the button has been finished the animation for collapse.

@@ +24,5 @@
> +   */
> +  init: function (aElement) {
> +    var win = aElement.getNode().ownerDocument.defaultView;
> +    var self = this;
> +    this.obs = new win.MutationObserver(function (mutations) {

'obs' is the shortcut for 'observer service'. So please do not mix-up those terms.

@@ +30,5 @@
> +        self.isAnimated = mutation.target.hasAttribute("collapsed");
> +      });
> +    });
> +    this.obs.observe(aElement.getNode(),
> +                     { attributes: true, attributeFilter: ["collapsed"] });

Please use a style like in waitFor()

@@ +440,5 @@
> +          break;
> +        default:
> +          break;
> +      }
> +    }

Why hasn't this been reverted as instructed in one of the former comments?

@@ +463,5 @@
>          throw new Error(arguments.callee.name + ": Unknown event type - " + type);
>      }
>  
>      try {
> +      var context = this;

This can be removed, right?

@@ +468,3 @@
>        assert.waitFor(function () {
> +        return self.opened &&
> +               (animationObserver.isAnimated) ? self.transitioned : true;

Can't we already initialize self.transitioned to true if no animation should happen?

@@ +510,5 @@
> +          break;
> +        default:
> +          break;
> +      }
> +    }

Same as above.

Comment 47

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.3

(In reply to Henrik Skupin (:whimboo) from comment #46)
> isAnimated or hasAnimated? I would prefer the latter if we are setting it to
> true after the button has been finished the animation for collapse.
this flag would say if the tab will animate from the moment is set on.
the flag that says if it has animated is transitioned.

I updated the patch, and I made new test reports, functional and endurance.
Windows:
http://mozmill-crowd.blargon7.com/#/endurance/report/21341f02f219acb032f628de85703a02
http://mozmill-crowd.blargon7.com/#/functional/report/21341f02f219acb032f628de856c2891
http://mozmill-crowd.blargon7.com/#/functional/report/21341f02f219acb032f628de8567b5e7
Mac:
http://mozmill-crowd.blargon7.com/#/endurance/report/21341f02f219acb032f628de8582a82f
http://mozmill-crowd.blargon7.com/#/functional/report/21341f02f219acb032f628de855c40ef
http://mozmill-crowd.blargon7.com/#/functional/report/21341f02f219acb032f628de855c3a0a
Linux
http://mozmill-crowd.blargon7.com/#/endurance/report/21341f02f219acb032f628de85825045
http://mozmill-crowd.blargon7.com/#/functional/report/21341f02f219acb032f628de853c5003
http://mozmill-crowd.blargon7.com/#/functional/report/21341f02f219acb032f628de853c1231

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 830822 [details] [diff] [review]
patch_v7.3

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

With the nits fixed you get my r+. For the next version please ask Andreea and Andrei only. Thanks.

::: lib/tabs.js
@@ +30,5 @@
> +        self.isAnimated = mutation.target.hasAttribute("collapsed");
> +      });
> +    });
> +    this.mutationObserver.observe(aElement.getNode(), {attributes: true,
> +                                                       attributeFilter: ["collapsed"]});

It's not the waitFor style I have mentioned last time but well, fine for now except Andreea or Andrei want to see it improved.

@@ +110,5 @@
>    this._controller = controller;
>    this._tabs = this.getElement({type: "tabs"});
> +
> +  animationObserver.init(this.getElement({type: "tabs_scrollButton",
> +                                          subtype: "down"}));

I would have done this in two separate expressions.

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +46,5 @@
>    assert.waitFor(function () {
>      return tabBrowser.length === 2;
>    }, "One tab has been closed via middle click");
>  
> +  // Close a tab by pressing the keyboard shortcut:

I think we can omit this comment.

@@ -56,5 @@
> -
> -/**
> - * Map test functions to litmus tests
> - */
> -// testOpenInBackgroundTab.meta = {litmusids : [8094]};

This should not be around anymore, right? The patch most likely fails to apply here.

Comment 49

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.4

(In reply to Henrik Skupin (:whimboo) from comment #48)
> With the nits fixed you get my r+. For the next version please ask Andreea
> and Andrei only. Thanks.
Thanks Henrik, can't wait to see this landed.

Comment 50

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.5

I updated the patch due to landing of bug 927397.

Comment 51

[Andrei Eftimie]

Comment on attachment 8336794 [details] [diff] [review]
patch_v7.5

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

Looks good to me.

Lets get this in:
http://hg.mozilla.org/qa/mozmill-tests/rev/13f1a0e4fde5 (default)

Comment 52

[Andrei Eftimie]

We should keep an eye out for any tab-related failures since this touches the whole tab opening & tab closing functionality.

Cosmin, do we need backports here?

Comment 53

[Cosmin Malutan, [:cosmin-malutan]]

Patch applies cleanly on aurora.
For other branches I will wait for metro structure(bug 927397) to get landed before I get backports.

Comment 54

[Cosmin Malutan, [:cosmin-malutan]]

The patch applies cleanly on beta and release too. 

Reports:
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a04ea11
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a042db2
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a04f0b3
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a048c9e
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a04b4c0
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a05095d
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a057267
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a05534b
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a057360
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a055af2
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a057d9d
mozmill-crowd.blargon7.com/#/functional/reports/b99421c0f132c68dec1548288a05688f

Comment 55

[Andrei Eftimie]

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/5d3d8600d89e (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/859ee85dc24d (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/f881379c7f6b (mozilla-release)

Only the mozilla-esr24 branch remaining.

Comment 56

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v7.5[esr24]

Here is the patch that applies on esr24.

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1d71e5
http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1d5d8c
http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1d5f55
http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1db7c5
http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1dcfcd
http://mozmill-crowd.blargon7.com/#/functional/report/b99421c0f132c68dec1548288a1d96de

Comment 57

[Andrei Eftimie]

Comment on attachment 8339290 [details] [diff] [review]
patch_v7.5[esr24]

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

Looks good, landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/db6ce6015f7a (mozilla-esr24)

Comment 58

[Andrei Eftimie]

I think we're done here.

Thanks Cosmin for all the work!