revert: fix($animateCss): remove animation end event listeners on close #13504

IgorMinar · 2015-12-11T02:06:23Z

This reverts commit c98e08f.

This commit was identified as incompatible with ng-material at Google
and is causing broken builds there. Proper fix to be investigated once
the immediate regression is addressed.

Previously, the animate queue would only detect pinned elements when they were the same element as the to-be-animated element. Related angular#12617 Closes angular#13466

Closes angular#13491

This reverts commit c98e08f. This commit was identified as incompatible with ng-material at Google and is causing broken builds there. Proper fix to be investigated once the immediate regression is addressed.

… unit tests

IgorMinar · 2015-12-11T04:49:15Z

do not merge anything into 1.4.x branch including this PR.

IgorMinar · 2015-12-11T04:50:23Z

I'll merge this in when we are ready.

@petebacondarwin, @Narretz, @gkalpak please cherry pick the test fix to master and verify why the lint check didn't catch ddescribe during CI.

gkalpak · 2015-12-11T06:53:55Z

Looking into the ddescribe issue.
There's no ddecribe in master, btw.

gkalpak · 2015-12-11T07:34:55Z

(Indeed, it was merged without ddecribe in master (6858caf), but with ddescribe in v1.4.x (85e392f)).

For some reason, I can't seem to find any travis jobs for the commits between 20604e7 and 8f0b482 (on master) and between 33cc75e and 1329e0f (on v1.4.x). If I am not missing something, this means that there was no "CI" for the following commits:

master: 6858caf, bc41ad8, 6428ed5
v1.4.x: 7caf913, 0c1b54f, 85e392f, c98e08f (these include the commit that introduced the ddescribe)

The 2 builds on v1.4.x after the missed/skipped commits should have caught the ddescribe error, but they didn't get to run the ci-checks task, because they errored one step before that (on the test:unit task) because of timing-out issues.

Maybe it is related to this Travis issue: https://www.traviscistatus.com/incidents/b3xhcrr7g46d
Note, that there were disconnections due to timeouts (causing test:unit to fail) on both Saucelabs and BrowserStack, which indicate it might indeed be a Travis issue (or so I say 😛).

gkalpak · 2015-12-11T07:42:41Z

@IgorMinar, by "please cherry pick the test fix to master" I assume you only meant the ddescribe test fix, not the reverted $animateCss tests. If you meant something else, just lmk 😉

gkalpak · 2015-12-11T08:00:28Z

src/ngAnimate/animateCss.js

@@ -749,11 +747,6 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) {
          options.onDone();
        }

-        // Remove the transitionend / animationend listener(s)
-        if (events) {
-          element.off(events.join(' '), onAnimationProgress);


Not sure if it is related to the "breakage", but this line can cause trouble if events is an empty array. This is because of how jqLiteOn and jqLiteOff work (I didn't look into jQuery's implementation).

With an empty events array, we will be calling element.on('', onAnimationProgress) and element.off('', onAnimationProgress). Due to how .on() and .off() are implemented, these two calls will result in:

.on(...): Register a listener for the '' (empty string) event (won't ever fire - but that's fine).

.off(...): Deregister all listeners, because: .off('', ...) <==> .off().
jqLiteOff only check if the event is truthy (and '' isn't) and if not, it thinks it is a no-argument call and deregisters all listeners from the element.

If that is indeed the issue, I think it makes more sense to fix jqLiteOff instead of fixing it here.
BTW, prior to v1.3.0-beta.19, jqLiteOff did the right thing (using isUndefined(type) instead of !type), until someone "optimized" it ™ 😛

DISCLAIMER: I haven't investigated if events can be empty (or if it will always have atleast one entry), but there's a good chance it can, since someone already noticed it: #13502.

petebacondarwin · 2015-12-11T08:12:59Z

Good work @gkalpak .

Narretz · 2015-12-11T09:02:38Z

Thanks @gkalpak. If jquery also removes all events when argument is '' then there's no use in fixing it in jqlite. I'll check.

gkalpak · 2015-12-11T09:07:09Z

Yes, we needs to verify what jQuery does (but I would be very surprised if they did the same as jqLiteOff).

Narretz · 2015-12-11T10:14:13Z

The results are in ... jquery doesn't remove any events if .off is called with empty string. So we need to fix jqlite, I'm gonna do that now. We can of course also make the code in animateCss more explicit. Wdyt?

petebacondarwin · 2015-12-11T10:19:07Z

@Narretz thanks

Narretz · 2015-12-11T10:56:08Z

Scratch that, actually, jQuery also removes all events when called with '', so the fix must go directly into animateCss

IgorMinar · 2015-12-11T17:50:30Z

ok. this is good to be merged now, but there are some commits in the v1.4.x branch that might need to be reverted before we land this PR. commits to revert (in this order): 194ade2, 1329e0f.

I talked to @petebacondarwin about this and either him or I will do this cleanup next week. We are not under time pressure to do this as long as v1.4.x remains frozen.

Narretz · 2015-12-11T18:06:24Z

@IgorMinar I have a fix for the animation issue, so it doesn't necessarily have to be reverted. Or is there another reason why we can't just fix it?

gkalpak · 2015-12-11T18:44:54Z

@IgorMinar, are you sure 194ade2 is the one that needs to be reverted ?

IgorMinar · 2015-12-11T18:59:00Z

yes - the two commits listed are in v1.4.x branch but not in google3. g3
now contains an emergency rollback (revert) of the animation commit in this
PR.

Do not touch v1.4.x - pete or me will do the cleanup next week.

On Fri, Dec 11, 2015 at 10:45 AM Georgios Kalpakas notifications@github.com
wrote:

@IgorMinar https://github.com/IgorMinar, are you sure 194ade2
194ade2
is the one that needs to be reverted ?

—
Reply to this email directly or view it on GitHub
#13504 (comment).

IgorMinar · 2015-12-14T06:18:20Z

landed as 194ade2...bab3069 (I had to force push to get g3 and github in sync)

googlebot · 2015-12-14T06:18:22Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

gkalpak · 2015-12-14T09:09:10Z

@IgorMinar, @petebacondarwin, is v1.4.x still "frozen" ?

Narretz and others added 3 commits December 10, 2015 15:20

fix($animate): allow animations when pinned element is parent element

1329e0f

Previously, the animate queue would only detect pinned elements when they were the same element as the to-be-animated element. Related angular#12617 Closes angular#13466

docs(select): fix typo

194ade2

Closes angular#13491

revert: fix($animateCss): remove animation end event listeners on close

b98dcfa

This reverts commit c98e08f. This commit was identified as incompatible with ng-material at Google and is causing broken builds there. Proper fix to be investigated once the immediate regression is addressed.

googlebot added the cla: yes label Dec 11, 2015

test(ngOptionsSpec): remove ddescribe which caused CI to skip most of…

701e9fc

… unit tests

IgorMinar changed the title ~~revert: fix($animateCss): remove animation end event listeners on close~~ DO NOT MERGE: revert: fix($animateCss): remove animation end event listeners on close Dec 11, 2015

gkalpak reviewed Dec 11, 2015
View reviewed changes

IgorMinar force-pushed the v1.4.x branch from 194ade2 to bab3069 Compare December 14, 2015 06:17

IgorMinar changed the title ~~DO NOT MERGE: revert: fix($animateCss): remove animation end event listeners on close~~ revert: fix($animateCss): remove animation end event listeners on close Dec 14, 2015

googlebot added cla: no and removed cla: yes labels Dec 14, 2015

IgorMinar closed this Dec 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert: fix($animateCss): remove animation end event listeners on close #13504

revert: fix($animateCss): remove animation end event listeners on close #13504

IgorMinar commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

gkalpak commented Dec 11, 2015

gkalpak commented Dec 11, 2015

gkalpak commented Dec 11, 2015

gkalpak Dec 11, 2015

gkalpak Dec 11, 2015

gkalpak Dec 11, 2015

petebacondarwin commented Dec 11, 2015

Narretz commented Dec 11, 2015

gkalpak commented Dec 11, 2015

Narretz commented Dec 11, 2015

petebacondarwin commented Dec 11, 2015

Narretz commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

Narretz commented Dec 11, 2015

gkalpak commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

IgorMinar commented Dec 14, 2015

googlebot commented Dec 14, 2015

gkalpak commented Dec 14, 2015

revert: fix($animateCss): remove animation end event listeners on close #13504

revert: fix($animateCss): remove animation end event listeners on close #13504

Conversation

IgorMinar commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

gkalpak commented Dec 11, 2015

gkalpak commented Dec 11, 2015

gkalpak commented Dec 11, 2015

gkalpak Dec 11, 2015

Choose a reason for hiding this comment

gkalpak Dec 11, 2015

Choose a reason for hiding this comment

gkalpak Dec 11, 2015

Choose a reason for hiding this comment

petebacondarwin commented Dec 11, 2015

Narretz commented Dec 11, 2015

gkalpak commented Dec 11, 2015

Narretz commented Dec 11, 2015

petebacondarwin commented Dec 11, 2015

Narretz commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

Narretz commented Dec 11, 2015

gkalpak commented Dec 11, 2015

IgorMinar commented Dec 11, 2015

IgorMinar commented Dec 14, 2015

googlebot commented Dec 14, 2015

gkalpak commented Dec 14, 2015