fix(ngRepeat): trigger move animation #15072

pondermatic · 2016-08-30T22:19:23Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
bug fix

What is the current behavior? (You can also link to an open issue here)
#15068

ngRepeat does not trigger a move animation for items that come after inserted or deleted items. The documentation says that a move animation occurs "when an adjacent item is filtered out causing a reorder or when the item contents are reordered". With this fix, items that are moved can use an animation, such as changing the border or background color, to show a change. This will fix issue angular#15068.

…mparison operators with type.

austinoneil · 2016-09-01T03:07:05Z

src/ng/directive/ngRepeat.js

-  };
+  // var getBlockStart = function(block) {
+  //   return block.clone[0];
+  // };


Could you please remove this commented-out code?

The previous Travis test failed because of a network issue with saucelabs.com.

Narretz · 2016-09-12T15:08:49Z

src/ng/directive/ngRepeat.js

-            for (var itemKey in collection) {
-              if (hasOwnProperty.call(collection, itemKey) && itemKey.charAt(0) !== '$') {
-                collectionKeys.push(itemKey);
+            for (collectionKey in collection) {


Why this change? Key of the current item makes more sense to me than key of collection.

You are probably right. I went back and forth on several names and don't remember why I settled on collectionKey. Possibly for consistency in how I named other keys. Let me know if you want me to change it.

Narretz · 2016-09-12T15:27:43Z

Hi @pondermatic, thanks for this PR. This is a much requested feature, and I'd be happy if we can get this in. Since ngRepeat is a performance critical part of the code, we'll have to review it thoroughly, though (and run some benchmarks).
I've looked at the changes, and I have the feeling that some of them aren't really related to the PR. could you maybe comment on these sections that are essential to the PR, or even create a commit that only includes the changes need to fix this?

pondermatic · 2016-09-12T15:53:05Z

The diff is pretty ugly because I changed several variable names. I'll work on a version that preserves as many names as possible.
I would appreciate performance testing. I timed the unit tests in Windows Chrome between the previous version and this version of ngRepeat and didn't see a noticeable difference. But it's critical that this directive run as fast as possible.

Narretz · 2016-09-12T15:56:49Z

I've made some tests with the orderBy benchmark (after some fixes), rendering 500 ngRepeated spans from scratch, and found that the new version is about 15% slower than the current master. I'm not sure how much truth is in these numbers, but the difference was consistent (still in the millisecond area, though)
This is a simple ngRepeat that renders 5000 spans, without any animations.

The unit tests won't say much about performance as they usually have repeats with small numbers.

pondermatic · 2016-09-12T16:42:07Z

I've made a version that minimizes the changes, but the diff is still hard to follow. It'll probably be better to explain the key changes.

I'm going to do some profiling and see if I can speed things up.

Narretz · 2016-09-12T17:04:53Z

src/ng/directive/ngRepeat.js

+                  keyIdentifier, nextBlockMap[nextKey].key, nextLength);
+
+                delete nextBlockMap[nextKey].key;
+                delete nextBlockMap[nextKey].value;


This is new, right? Is this necessary? The tests do not fail without this.
If we are doing this for every item, this might actually impact performance.

The previous version did not store key and value in nextBlockMap. I am deleting them to keep them from needlessly using memory when nextBlockMap is assigned to lastBlockMap.
The orderby-bp benchmark ran about the same with or without deleting the properties.

pondermatic · 2016-09-12T20:53:36Z

I've been running the orderby-bp benchmark, updated by @Narretz today 0784977.

number of samples to collect and average: 20 (default)
number of ngRepeats: 5000 (default)
Loop 25x
Windows Chrome v53
One incognito window with five tabs, each tab with a different test
added up mean test time from setup and $apply in all five tests
each pass opened in new incognito window

c54f7a9 before pondermatic
pass 1 = 1058.66 ms
pass 2 = 1067.40 ms

263be6b after pondermatic
pass 1 = 1110.78 ms
pass 2 = 1127.65 ms

My version is 5.29% slower. :(

When I removed the lines that delete the collection item's key and value from nextBlockMap, pass 1 was faster but pass 2 was much slower. Either way, the last collection is retained in memory which is not good.

Then I put the key/value in its own array so that deleting is unnecessary. I also tried an array of just the keys, which should use less memory than 263be6b, especially if the values are large objects. The mean of those benchmark passes was 1111.31.

I plan on committing revised code with this memory tweak and another tweak.

In the ngRepeatAction function: * The collection keys and values are accessed directly instead of copying them to nextBlockMap temporarily. * Checking the collectionIsLikeArray boolean value is faster than comparing value and type of collectionKeys and collection.

…angular.js into ngRepeat-animations

pondermatic · 2016-09-12T22:44:50Z

src/ng/directive/ngRepeat.js

-  var getBlockStart = function(block) {
-    return block.clone[0];
-  };
-


getBlockStart function is no longer used.

pondermatic · 2016-09-12T23:25:46Z

Commit 7a98172 ran the orderby-bp benchmark in 1037.09 ms for pass 1 and 1079.71 ms for pass 2. ngRepeat is now as fast as or a little bit faster than it was before.

…an integer Merge changes from Narretz@7dbf428.

pondermatic · 2017-03-21T17:23:42Z

Hi @Narretz. Thank you for catching that bug! Your fix and unit test work perfectly.

Narretz

LGTM, except a few nitpicks. I want someone else from the team to look at this before merging as ngRepeat is an important directive.

Narretz · 2017-03-23T08:10:26Z

test/ng/directive/ngRepeatSpec.js

+      expect(item.event).toBe('leave');
+      expect(item.element.text()).toBe('2');
+    })
+  );


Please put a newline between the two its

Narretz · 2017-03-23T08:10:52Z

test/ng/directive/ngRepeatSpec.js

+        $rootScope.items = [items[0], items[1], items[3]];
+        $rootScope.$digest();
+
+        // The leaving item should maintain it's position until it is removed


it's -> its (I know, I added this test ;))

Narretz · 2017-03-23T08:13:18Z

Here's a plunker with ngRepeat and the fixed version to play around with: http://plnkr.co/edit/fu2SDoNfxCrUZPA0v0zm?p=preview

petebacondarwin · 2017-03-23T09:57:51Z

test/ng/directive/ngRepeatSpec.js

+      if (item.event === 'leave') {
+        break;
+      }
+    }


A comment to explain why this is needed would be helpful.

I am going to revert this code. The ngRepeat tests assume the animation queue is in a certain order. During refactoring of ngRepeat, the order was different in this test so I altered the test. My latest version of ngRepeat, c969ae2, passes the original version of this test.

Should tests assume that animation events are queued in a certain order? If not, then perhaps the queue should be tested like this:

// Test each queue item's event type while ($animate.queue.length) { item = $animate.queue.shift(); switch (item.element.text()) { case '2': expect(item.event).toBe('leave'); break; case '3': expect(item.event).toBe('move'); break; default: expect(item).toBeUndefined(); } }

Maybe we should add a Jasmine matcher in the "ngRepeat Animations" block that compares item.event and item.text() values between two arrays? Should this be new GitHub issue?

Apparently, the animation queue order IS different than it used to be.
Options:

use a while loop to search for the leave event, as in commit 2331c42

shift the queue array a second time, assuming that the leave event is the second item in the queue

add a Jasmine matcher that searches the queue for a item with a specific element text and event

As far as I can tell, tests should not assume the order of events in the queue. So I propose the following matcher be added to the 'ngRepeat animations' group of tests:

beforeEach(function() { jasmine.addMatchers({ toContainQueueItem: function() { return { compare: generateCompare(false), negativeCompare: generateCompare(true) }; function generateCompare(isNot) { /** * Matcher that checks that the expected element text is in the actual Array of * animation queue items and that the event matches. * @param {array} actual * @param {string} expectedElementText * @param {string} expectedEvent optional if isNot = true * @returns {{pass: boolean, message: message}} */ return function(actual, expectedElementText, expectedEvent) { if (expectedEvent === undefined) { expectedEvent = ''; } var actualLength = actual.length; var i; var item; var message = valueFn( 'Expected animation queue to ' + (isNot ? 'not ' : '') + 'contain an item where the element\'s text is "' + expectedElementText + '"' + (isNot ? '' : ' and the event is "' + expectedEvent + '"') ); var pass = isNot; for (i = 0; i < actualLength; i++) { item = actual[i]; if (item.element.text() === expectedElementText) { pass = item.event === expectedEvent; break; } } return {'pass': pass, 'message': message}; }; } } }); });

The tests should use the matcher like this:

expect($animate.queue).not.toContainQueueItem('1'); expect($animate.queue).toContainQueueItem('2', 'leave'); expect($animate.queue).toContainQueueItem('3', 'move'); $animate.queue = [];

What does everyone think?

petebacondarwin · 2017-03-23T10:00:10Z

test/ng/directive/ngRepeatSpec.js

+    })
+  );
+
+  it('should fire off the move animation for filtered items',


This description could be more clear. Perhaps:

"should fire the move animation for items that shifted due to removal of previous items"

Or something like that.

How about this description?
should fire off the move animation for items that change position when other items are filtered out

petebacondarwin

should the tests be checking the previousNode value that is passed to the move animation?

petebacondarwin · 2017-03-23T10:03:57Z

Generally looks like a good refactoring. Thanks @pondermatic

In the 'should fire off the leave animation' test, revert change in how the animation queue is shifted to find item '2'. At one time during the refactoring of ngRepeat, the order of events in the queue changed. Improve title of the spec that checks for 'move' animation events when filtering items changes other items positions.

Improve title of the spec that checks for 'move' animation events when filtering items changes other items positions. Remove unused item variable in two tests.

petebacondarwin

LGTM - except for the failing tests :-)

The 'ngRepeat animations' test group should not assume the order of items in the animation queue. This matcher searches the animation queue for an item with an expected element text and event.

pondermatic · 2017-03-25T15:23:30Z

I can't believe I pushed that change without running tests. :)

pondermatic · 2017-03-27T20:32:39Z

test/ng/directive/ngRepeatSpec.js

@@ -1424,15 +1424,58 @@ describe('ngRepeat animations', function() {
    };
  }));

+  beforeEach(function() {


Tests should not rely on the order of items in the animation queue. This Jasmine matcher looks for a queued item with a given element text and matches against that item's event.

Narretz · 2017-03-30T09:46:44Z

I decided to do some more tests, and it looks like we created a performance problem with the new behavior when ngAnimate is used. See this updated plnkr: http://plnkr.co/edit/fu2SDoNfxCrUZPA0v0zm?p=preview

The second repeat renders 10000 elements (I know that you generally should avoid these huge lists)

Now remove the second item in the array. Only Chrome handles this gracefully. Firefox and Edge have a noticable delay. become unresponsive and might even display "page not responding" warnings
This behavior occurs even when you remove the animation styles, because ngAnimate will still check each item for animatable styles. Only when you disable animations or remove ngAnimate, perf returns to normal
The bottleneck is not the ngRepeat computation or the dom operation
ngAnimate caches the transitions / animations, so the problem mainly lies with the application / rendering of the styles to the elements (essentially the browser engine).
with 1000 items, the performance is okay

@matsko had done some interesting changes with regard to caching animation styles here: #14166 With ngAnimate enabled and no actual animation styles, the whole process is aborted earlier with this PR, but there is no noticable performance gain.

So the question is, are we okay with the perf dropoff with large repeats? There are many ways to disable animations on perf critical parts of the page, but this might still bite people whose apps will suddenly perform worse.

I suggest we should at least look into making ngAnimate faster when no actual css animations are present.

Consistenly slow is checking when animations are allowed, because we have to traverse the node parent again again and again:

angular.js/src/ngAnimate/animateQueue.js

Line 621 in f403925

function areAnimationsAllowed(node, parentNode, event) {

What's worse, we basically do the same later on to determine the animation order:

angular.js/src/ngAnimate/animation.js

Line 51 in f403925

function processNode(entry) {

maybe this could be combined, or cached?

pondermatic added 2 commits August 30, 2016 16:41

style(ngRepeat) eslint does not like unused functions and requires co…

263be6b

…mparison operators with type.

googlebot added the cla: yes label Aug 30, 2016

austinoneil reviewed Sep 1, 2016
View reviewed changes

pondermatic added 2 commits August 31, 2016 23:04

style(ngRepeat): getBlockStart function is no longer needed

f3c6062

style(ngRepeat): kick off Travis-CI

47bd3a4

The previous Travis test failed because of a network issue with saucelabs.com.

gkalpak added type: bug frequency: moderate needs: review component: ngRepeat severity: broken expected use labels Sep 8, 2016

gkalpak added this to the Backlog milestone Sep 8, 2016

Narretz reviewed Sep 12, 2016
View reviewed changes

Narretz self-assigned this Sep 12, 2016

pondermatic added 2 commits September 12, 2016 17:03

Merge branch 'ngRepeat-animations' of https://github.com/pondermatic/…

d8320a1

…angular.js into ngRepeat-animations

pondermatic reviewed Sep 12, 2016
View reviewed changes

pondermatic added 2 commits March 21, 2017 11:31

Merge branch 'master' into ngRepeat-animations

dcc80c1

fix(ngRepeat): correctly re-create last block order when track by is …

c969ae2

…an integer Merge changes from Narretz@7dbf428.

Narretz suggested changes Mar 23, 2017

View reviewed changes

petebacondarwin reviewed Mar 23, 2017

View reviewed changes

pondermatic added 2 commits March 24, 2017 12:41

test(ngRepeat): Update based on feedback.

4d6feb9

Improve title of the spec that checks for 'move' animation events when filtering items changes other items positions. Remove unused item variable in two tests.

petebacondarwin approved these changes Mar 25, 2017

View reviewed changes

petebacondarwin suggested changes Mar 25, 2017

View reviewed changes

test(ngRepeat): Add 'toContainQueueItem' Jasmine matcher

3b17d5d

The 'ngRepeat animations' test group should not assume the order of items in the animation queue. This matcher searches the animation queue for an item with an expected element text and event.

pondermatic commented Mar 27, 2017

View reviewed changes

petebacondarwin approved these changes Mar 28, 2017

View reviewed changes

Narretz modified the milestones: 1.6.4, 1.6.5 Mar 31, 2017

Narretz modified the milestones: 1.6.5, 1.6.x Apr 19, 2017

Narretz modified the milestones: 1.6.x, 1.7.x Apr 12, 2018

petebacondarwin modified the milestones: 1.7.x, 1.7.x - won't fix Sep 19, 2018

fix(ngRepeat): trigger move animation #15072

fix(ngRepeat): trigger move animation #15072

pondermatic commented Aug 30, 2016

austinoneil Sep 1, 2016

Narretz Sep 12, 2016

pondermatic Sep 12, 2016

Narretz commented Sep 12, 2016

pondermatic commented Sep 12, 2016

Narretz commented Sep 12, 2016 •

edited

pondermatic commented Sep 12, 2016

Narretz Sep 12, 2016

pondermatic Sep 12, 2016 •

edited

pondermatic commented Sep 12, 2016

pondermatic Sep 12, 2016

pondermatic commented Sep 12, 2016

pondermatic commented Mar 21, 2017

Narretz left a comment

Narretz Mar 23, 2017

Narretz Mar 23, 2017

Narretz commented Mar 23, 2017

petebacondarwin Mar 23, 2017

pondermatic Mar 24, 2017

pondermatic Mar 24, 2017

pondermatic Mar 25, 2017

petebacondarwin Mar 23, 2017

pondermatic Mar 24, 2017

petebacondarwin Mar 25, 2017

petebacondarwin left a comment

petebacondarwin commented Mar 23, 2017

petebacondarwin left a comment

pondermatic commented Mar 25, 2017

pondermatic Mar 27, 2017

Narretz commented Mar 30, 2017 •

edited

fix(ngRepeat): trigger move animation #15072

Are you sure you want to change the base?

fix(ngRepeat): trigger move animation #15072

Conversation

pondermatic commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Sep 12, 2016

pondermatic commented Sep 12, 2016

Narretz commented Sep 12, 2016 • edited

pondermatic commented Sep 12, 2016

Choose a reason for hiding this comment

pondermatic Sep 12, 2016 • edited

Choose a reason for hiding this comment

pondermatic commented Sep 12, 2016

Choose a reason for hiding this comment

pondermatic commented Sep 12, 2016

pondermatic commented Mar 21, 2017

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin commented Mar 23, 2017

petebacondarwin left a comment

Choose a reason for hiding this comment

pondermatic commented Mar 25, 2017

Choose a reason for hiding this comment

Narretz commented Mar 30, 2017 • edited

Narretz commented Sep 12, 2016 •

edited

pondermatic Sep 12, 2016 •

edited

Narretz commented Mar 30, 2017 •

edited