969397 - Update openTab() and closeTab() from tabs.js in Metro library to wait for the tab animation event

Status: RESOLVED FIXED

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

Description

[Daniel Gherasim]

As we see in our library, in tabs.js , the events TabOpen and TabClose doesn't actually waits for the tab animation to finish. What we need here is to find an event to wait for, or another solution for this.

https://hg.mozilla.org/qa/mozmill-tests/file/cbd1d6fa1f51/metrofirefox/lib/ui/tabs.js#l222
https://hg.mozilla.org/qa/mozmill-tests/file/cbd1d6fa1f51/metrofirefox/lib/ui/tabs.js#l283

Comment 1

[Daniel Gherasim]

Attachment: waitForAnimation_Tabs_v1.patch

Details about the patch:

For waiting for the animation we used the animationend that triggers in both close & open tabs cases, as we can see here:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#475

Comment 2

[Andrei Eftimie]

Comment on attachment 8373214 [details] [diff] [review]
waitForAnimation_Tabs_v1.patch

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

Great work Daniel.

Just 3 small nits:
Please update the commit message with a reviewer (r=aeftimie should be fine in this case)

::: metrofirefox/lib/ui/tabs.js
@@ +21,5 @@
>      assert.fail("A valid controller must be specified");
>    }
>  
>    this._controller = aController;
> +  this._tabs = this.getElement({type: "tabsContainer"});

Wondering if we shouldn't name this _tabsContainer to avoid any potential confusion.
Also since we are always using the node, we could cache the node directly.

So this might be named _tabsContainerNode

@@ +229,5 @@
> +      animationend: false
> +    };
> +
> +    function checkTabOpened() { self.opened = true; }
> +    function checkTabAnimationend() { self.animationend = true; }

I do have 1 nit. Please update the name so its properly camelcased: checkTabAnimationEnd

Comment 3

[Daniel Gherasim]

Attachment: waitForAnimation_Tabs_v1.1.patch

Thanks, 
I updated the patch with the requested changes.

Comment 4

[Andrei Eftimie]

Comment on attachment 8373349 [details] [diff] [review]
waitForAnimation_Tabs_v1.1.patch

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

Looks good.

Henrik, do you want to take a quick look to this patch?

Comment 5

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

Comment on attachment 8373349 [details] [diff] [review]
waitForAnimation_Tabs_v1.1.patch

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

That looks fine and I'm happy to see this fixed soon. Thanks.

Comment 6

[Andrei Eftimie]

Comment on attachment 8373349 [details] [diff] [review]
waitForAnimation_Tabs_v1.1.patch

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/0748f762468e (default)

Comment 7

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

This bug is not completely fixed. We have to backport the patch up to beta.

Comment 8

[Daniel Gherasim]

For now we are not going to backport this.