fix(ngAnimate): defer DOM operations for changing classes to postDigest #9283

caitp · 2014-09-25T23:28:41Z

When ngAnimate is used, it will defer changes to classes until postDigest.
Previously, AngularJS (when ngAnimate is not loaded) would always immediately
perform these DOM operations.

Now, even when the ngAnimate module is not used, if $rootScope is in the midst
of a digest, class manipulation is deferred. This helps reduce jank in
browsers such as IE11.

/CC @tbosch --- alternative/more general fix here

Closes #8234
Closes #9263

caitp · 2014-09-25T23:42:43Z

So I personally think the other fix is tinier and able to work for local digests, so that's kind of a good thing... this obviously is basically taking a bunch of meat out of ngAnimate and moving it to core, so it grows the code maybe more than it should.

But both solutions appear to fix the jank, so yay. It's possible that this could be reworked better to have less code duplication between ng and ngAnimate. /cc @matsko

tbosch · 2014-09-30T16:48:00Z

I think we need more tests for this change. Could you copy the corresponding tests from ngAnimate over?

Also, as ngAnimate just decorates $animate from core we should be able to not duplicate the defer logic but just put it into the core and have ngAnimate use it. Could you try this?

caitp · 2014-09-30T16:57:07Z

will do

caitp · 2014-09-30T16:58:00Z

Also, as ngAnimate just decorates $animate from core we should be able to not duplicate the defer logic but just put it into the core and have ngAnimate use it. Could you try this?

The way this is organized, it will be a bit of effort to remove code duplication in a way that doesn't break one module or the other --- I want to do this and was hoping Matias could help, but it might not get done for this release. I don't think this should block landing it

caitp · 2014-09-30T17:33:25Z

test/ng/animateSpec.js

@@ -100,4 +100,118 @@ describe("$animate", function() {
      inject();
    });
  });
+
+  describe('class API', function() {


ping @tbosch --- I didn't see any similar tests to this in ngAnimate, so we might want to move these there too. Let me know if these look good to you

As ngAnimate decorates $animate from core, the functionality should only be in core, and the tests for this as well. Looking at the tests...

caitp · 2014-09-30T17:37:52Z

... I think the toHaveClass matchers don't really work correctly for SVG :(

tbosch · 2014-09-30T17:38:53Z

test/ng/animateSpec.js

+    });
+
+    it('should defer class manipulation until end of digest', inject(function($rootScope, $animate) {
+      element = jqLite('<p>test</p>');


Could you add an initial class that is removed during the digest and check that it is not really removed until the end of the digest?

tbosch · 2014-09-30T20:48:32Z

@caitp After our talk in the meeting, please make the following changes:

Tests: Check how often jqLiteAddClass and jqLiteRemoveClass class has been called and also their order for the following cases:
- add a class twice in the same digest
- add a class via $animate and in the same digest directly via jqLiteAddClass -> should not call jqLiteAddClass again (see resolveElementClasses)
- add two different classes within the same digest -> order should be preserved
Tests: Add tests for the promise that is now returned by addClass, removeClass and setClass
Docs: This is a breaking change, please add a note in the commit message

tbosch · 2014-09-30T20:54:16Z

src/ng/animate.js

+        return this.setClass(element, className, []);
+      },
+
+      $$addClass : function(element, className) {


Can we rename this to $$addClassImmediately?

tbosch · 2014-09-30T20:55:53Z

For the tests, you can actually spy on $$addClass / $$removeClass to see how often and in which order they have been called...

caitp · 2014-09-30T20:56:23Z

of these comments:
- jqLiteAddClass/jqLiteRemoveClass are hard to mock since they are global functions (yes, $$addClass/$$removeClass do this sort of, and those are already mocked)
- acknowledged
- acknowledged
What exactly is this testing? The cancel function would be exposed, but it wouldn't really do anything
I'm not sure in exactly what way this is a breaking change, other than classes not always being added immediately (which will be documented). Are there any other obvious breaks?

caitp · 2014-09-30T21:47:25Z

I've added a bunch of new tests, I am sure I could come up with 50 more if I need to, but I'm not totally sure what I'm verifying (particularly in the promise case).

How does this look?

caitp · 2014-09-30T22:21:13Z

So, since functionality is not totally shared between the two, copy tests into ngAnimate?

Is the promise functionality being tested enough for you here?

Is the ordering tested enough? Does this look alright?

PTAL --- If it looks good, will fix the commit message to include a breaking change notice and land

tbosch · 2014-09-30T22:38:48Z

Tests look good. For the promise case this is enough (just to make sure the promise is actually resolved...).

Re breaking change: Yes, the fact that classes are not always being added immediately any more.

Yes, good idea, let's add the tests also to ngAnimate to make sure ngAnimate has the same behavior!

matsko · 2014-10-07T08:37:04Z

@caitp just a heads up, I changed the way the classes are added and removed internally. It should be an easy patch for you: #9458.

Please hold off the merge until that PR is in (Igor should put it in later today).

IgorMinar · 2014-10-08T00:02:53Z

#9458 has landed.

@caitp can you please update this PR asap and address all the remaining issues?

caitp · 2014-10-08T00:26:45Z

yep --- mentioned in the chat, I'll try to have this updated tonight or early tomorrow

caitp · 2014-10-08T02:46:12Z

Okay --- everything should be addressed now. PTAL

caitp · 2014-10-08T02:47:40Z

src/ng/animate.js

+        var createdCache = false;
+        element = jqLite(element);
+
+        if (runSynchronously) {


Unfortunately, ngClass has a test which fails if core ngAnimate doesn't work synchronously. I spent some time trying to fix the test, but I couldn't really decide how it should work.

This is bad because it's a definite difference in behaviour for one special case, so I think we'll want to fix this --- but maybe it can wait a bit?

obviously, runSynchronously is not documented since I think we should get rid of it within a week or two, but I don't think it should block landing the CL

When ngAnimate is used, it will defer changes to classes until postDigest. Previously, AngularJS (when ngAnimate is not loaded) would always immediately perform these DOM operations. Now, even when the ngAnimate module is not used, if $rootScope is in the midst of a digest, class manipulation is deferred. This helps reduce jank in browsers such as IE11. Closes angular#8234 Closes angular#9263

Always defer class manipulation DOM writes until the end of digest on the root scope, even when animations are disabled. BREAKING CHANGE: The $animate class API will always defer changes until the end of the next digest. This allows ngAnimate to coalesce class changes which occur over a short period of time into 1 or 2 DOM writes, rather than many. This prevents jank in browsers such as IE, and is generally a good thing. If you're finding that your classes are not being immediately applied, be sure to invoke $digest().

caitp · 2014-10-08T17:33:06Z

So can we get one last look at this before the release? I'd like to get it checked in.

As stated, there is some rubbish in there that I think we can get rid of subsequently, but I don't think it should block landing this (it could have been fixed, but I just wasn't sure how the tests should have been reworked because of it).

I'm also not positive all of the changes in the last commit are really that useful, but they were asked for. Can revert parts of it if needed

IgorMinar · 2014-10-08T18:03:36Z

src/ng/animate.js

+    function resolveElementClasses(element, cache) {
+      var toAdd = [], toRemove = [];
+      forEach(cache.classes, function(status, className) {
+        var hasClass = jqLiteHasClass(element[0], className);


this will have terrible perf when multiple classes are added. can we read the existing classes from the dom only once for render?

I mentioned on Matsko's PR that we don't really need to care if an element has the class already or not --- it could improve perf to just skip reading it. It's not very nice for tests though

I'm not sure what you mean by "read the existing classes from the dom only once for render".

Do you mean like just do `hasClass = (' ' + element.className + ' ').indexOf(' ' + className + ' ') >= 0)`` or something?

(where the class attribute would be cached)

just read out the className, split it, create a map and use that map for checking if class is already in the dom

IgorMinar · 2014-10-08T18:12:14Z

src/ngAnimate/animate.js

@@ -979,7 +979,7 @@ angular.module('ngAnimate', ['ng'])
          element = stripCommentsFromElement(element);

          if (classBasedAnimationsBlocked(element)) {
-            return $delegate.setClass(element, add, remove);
+            return $delegate.setClass(element, add, remove, true);


can't we use jqLite here instead?

oh, we need to return the promise which complicates things

If we do, when jQuery is used we'll die on SVG elements

I do want to fix this up so that it sucks less though (but I think matsko will need to help fix broken tests that fail when we don't do this synchronously)

ok. let's keep it as is for now. can we add a comment that we call it this way via undocumented api.

This also does some cleanup in $animate

…ementClasses In ngAnimate, we can't do this because the behaviour is exposed via the API. But in core, we can avoid a bit of work.

Previously we were reading DOM attributes frequently, now we can do it just once.

It was previously used for ngAnimate, but is no longer needed

caitp · 2014-10-08T18:40:42Z

src/ng/animate.js

+      });
+
+      forEach(cache.classes, function(status, className) {
+        var hasClass = hasClasses[className] === true;


=== true so that classes like toString or constructor don't cause problems

use createMap instead and then you don't need this

caitp added severity: regression type: bug component: ngAnimate needs: review labels Sep 25, 2014

caitp added this to the 1.3.0-rc.4 milestone Sep 25, 2014

caitp assigned tbosch Sep 25, 2014

mary-poppins added cla: yes and removed cla: no labels Sep 26, 2014

jeffbcross force-pushed the master branch from dbbabb9 to 27d1234 Compare September 27, 2014 00:06

tbosch mentioned this pull request Sep 30, 2014

fix(ngModel): minimize jank when toggling CSS classes during validation #9263

Closed

caitp force-pushed the issue-8234-2 branch from e0eb8ce to 66733a8 Compare September 30, 2014 17:30

caitp reviewed Sep 30, 2014
View reviewed changes

tbosch reviewed Sep 30, 2014
View reviewed changes

caitp force-pushed the issue-8234-2 branch 2 times, most recently from 8932480 to dd7691c Compare September 30, 2014 17:58

tbosch reviewed Sep 30, 2014
View reviewed changes

IgorMinar mentioned this pull request Sep 30, 2014

ng-hide="expression()" not working in 1.3-rc2 (timing issue?) #9147

Closed

caitp force-pushed the issue-8234-2 branch from dd7691c to 3dd77e0 Compare September 30, 2014 21:37

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

caitp force-pushed the issue-8234-2 branch from ce274bd to 49eba65 Compare October 8, 2014 02:33

caitp reviewed Oct 8, 2014
View reviewed changes

caitp added 4 commits October 7, 2014 23:00

test(matchers.js): make toHaveClass matcher work better for SVG+jQuery

c3ce5d4

WIP --- fixups

a6ee572

caitp force-pushed the issue-8234-2 branch from 410372e to b68cd49 Compare October 8, 2014 03:00

IgorMinar reviewed Oct 8, 2014
View reviewed changes

caitp force-pushed the issue-8234-2 branch from b68cd49 to c966b2f Compare October 8, 2014 18:08

IgorMinar reviewed Oct 8, 2014
View reviewed changes

caitp added 3 commits October 8, 2014 14:25

refactor($compile): use labaled variables to represent nodeType values

3e2ad02

This also does some cleanup in $animate

chore(ngAnimate): add TODO messages indicating desire to remove hack

bf84fd1

perf($animate): don't join classes before it's necessary in resolveEl…

e954d22

…ementClasses In ngAnimate, we can't do this because the behaviour is exposed via the API. But in core, we can avoid a bit of work.

caitp force-pushed the issue-8234-2 branch from bcbc7e5 to e954d22 Compare October 8, 2014 18:25

caitp added 2 commits October 8, 2014 14:34

perf($animate): access DOM less in resolveElementClasses

cbf7d92

Previously we were reading DOM attributes frequently, now we can do it just once.

chore(AngularPublic): remove $$hasClass from angular exports

6949e5d

It was previously used for ngAnimate, but is no longer needed

caitp reviewed Oct 8, 2014
View reviewed changes

jeffbcross force-pushed the master branch 2 times, most recently from abdaab7 to 30996f8 Compare October 8, 2014 19:47

caitp closed this Oct 8, 2014

fix(ngAnimate): defer DOM operations for changing classes to postDigest #9283

fix(ngAnimate): defer DOM operations for changing classes to postDigest #9283

Conversation

caitp commented Sep 25, 2014

caitp commented Sep 25, 2014

tbosch commented Sep 30, 2014

caitp commented Sep 30, 2014

caitp commented Sep 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitp commented Sep 30, 2014

Choose a reason for hiding this comment

tbosch commented Sep 30, 2014

Choose a reason for hiding this comment

tbosch commented Sep 30, 2014

caitp commented Sep 30, 2014

caitp commented Sep 30, 2014

caitp commented Sep 30, 2014

tbosch commented Sep 30, 2014

matsko commented Oct 7, 2014

IgorMinar commented Oct 8, 2014

caitp commented Oct 8, 2014

caitp commented Oct 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitp commented Oct 8, 2014

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

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