Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

perf($animate): listen for document visibility changes #14071

Merged
merged 4 commits into from
Apr 26, 2016
Merged

perf($animate): listen for document visibility changes #14071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d056cb
perf($animate): listen for document visibility changes
Narretz Feb 17, 2016
25bddfa
remove when rootScope is destroyed, add + fix tests
Narretz Apr 26, 2016
6a0adfb
fixup
Narretz Apr 26, 2016
8faa004
fix test exepcts for jq 2.1
Narretz Apr 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/AngularPublic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
$ControllerProvider,
$DateProvider,
$DocumentProvider,
$$IsDocumentHiddenProvider,
$ExceptionHandlerProvider,
$FilterProvider,
$$ForceReflowProvider,
Expand Down Expand Up @@ -227,6 +228,7 @@ function publishExternalAPI(angular) {
$cacheFactory: $CacheFactoryProvider,
$controller: $ControllerProvider,
$document: $DocumentProvider,
$$isDocumentHidden: $$IsDocumentHiddenProvider,
$exceptionHandler: $ExceptionHandlerProvider,
$filter: $FilterProvider,
$$forceReflow: $$ForceReflowProvider,
Expand Down
10 changes: 3 additions & 7 deletions src/ng/animateRunner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ var $$AnimateAsyncRunFactoryProvider = function() {
};

var $$AnimateRunnerFactoryProvider = function() {
this.$get = ['$q', '$sniffer', '$$animateAsyncRun', '$document', '$timeout',
function($q, $sniffer, $$animateAsyncRun, $document, $timeout) {
this.$get = ['$q', '$sniffer', '$$animateAsyncRun', '$$isDocumentHidden', '$timeout',
function($q, $sniffer, $$animateAsyncRun, $$isDocumentHidden, $timeout) {

var INITIAL_STATE = 0;
var DONE_PENDING_STATE = 1;
Expand Down Expand Up @@ -81,11 +81,7 @@ var $$AnimateRunnerFactoryProvider = function() {

this._doneCallbacks = [];
this._tick = function(fn) {
var doc = $document[0];

// the document may not be ready or attached
// to the module for some internal tests
if (doc && doc.hidden) {
if ($$isDocumentHidden()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this in every tick (and for every runner), coudn't we listen for an event and update the function as necessary ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tick is only called when the animation completes, so only once per runner. I'm not sure how costly one function call is. Event listener is also possible. but I don't want to add another one over the one we have in $$isDocumentHidden. So maybe we could move the actual async tick into this service where it will be updated by the listener. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I confused this with the $$rAF's tick. Since it's indeed called one, I guess it's fine as it is.

timeoutTick(fn);
} else {
rafTick(fn);
Expand Down
26 changes: 26 additions & 0 deletions src/ng/document.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,29 @@ function $DocumentProvider() {
return jqLite(window.document);
}];
}


/**
* @private
* Listens for document visibility change and makes the current status accessible.
*/
function $$IsDocumentHiddenProvider() {
this.$get = ['$document', '$rootScope', function($document, $rootScope) {
var doc = $document[0];
var hidden = doc && doc.hidden;

$document.on('visibilitychange', changeListener);

$rootScope.$on('$destroy', function() {
$document.off('visibilitychange', changeListener);
});

function changeListener() {
hidden = doc.hidden;
}

return function() {
return hidden;
};
}];
}
6 changes: 4 additions & 2 deletions src/ngAnimate/animateQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,10 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {

this.$get = ['$$rAF', '$rootScope', '$rootElement', '$document', '$$HashMap',
'$$animation', '$$AnimateRunner', '$templateRequest', '$$jqLite', '$$forceReflow',
'$$isDocumentHidden',
function($$rAF, $rootScope, $rootElement, $document, $$HashMap,
$$animation, $$AnimateRunner, $templateRequest, $$jqLite, $$forceReflow) {
$$animation, $$AnimateRunner, $templateRequest, $$jqLite, $$forceReflow,
$$isDocumentHidden) {

var activeAnimationsLookup = new $$HashMap();
var disabledElementsLookup = new $$HashMap();
Expand Down Expand Up @@ -367,7 +369,7 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) {

var isStructural = ['enter', 'move', 'leave'].indexOf(event) >= 0;

var documentHidden = $document[0].hidden;
var documentHidden = $$isDocumentHidden();

// this is a hard disable of all animations for the application or on
// the element itself, therefore there is no need to continue further
Expand Down
11 changes: 5 additions & 6 deletions test/ng/animateRunnerSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,14 +163,13 @@ describe("$$AnimateRunner", function() {
}));

it("should use timeouts to trigger async operations when the document is hidden", function() {
var doc;
var hidden = true;

module(function($provide) {
doc = jqLite({
body: document.body,
hidden: true

$provide.value('$$isDocumentHidden', function() {
return hidden;
});
$provide.value('$document', doc);
});

inject(function($$AnimateRunner, $rootScope, $$rAF, $timeout) {
Expand All @@ -184,7 +183,7 @@ describe("$$AnimateRunner", function() {
$timeout.flush();
expect(spy).toHaveBeenCalled();

doc[0].hidden = false;
hidden = false;

spy = jasmine.createSpy();
runner = new $$AnimateRunner();
Expand Down
43 changes: 21 additions & 22 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7245,22 +7245,22 @@ describe('$compile', function() {
});

inject(function($compile, $rootScope) {
expect(jqLiteCacheSize()).toEqual(0);
var cacheSize = jqLiteCacheSize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and a few other tests assume that the cacheSize is 0 after bootstrap. The listener for visibilitychange on the document adds a cache entry. So I thought it makes more sense to not assume a specific cacheSize on bootstrap.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, makes sense.


element = $compile('<div><div ng-repeat="x in xs" ng-if="x==1">{{x}}</div></div>')($rootScope);
expect(jqLiteCacheSize()).toEqual(1);
expect(jqLiteCacheSize()).toEqual(cacheSize + 1);

$rootScope.$apply('xs = [0,1]');
expect(jqLiteCacheSize()).toEqual(2);
expect(jqLiteCacheSize()).toEqual(cacheSize + 2);

$rootScope.$apply('xs = [0]');
expect(jqLiteCacheSize()).toEqual(1);
expect(jqLiteCacheSize()).toEqual(cacheSize + 1);

$rootScope.$apply('xs = []');
expect(jqLiteCacheSize()).toEqual(1);
expect(jqLiteCacheSize()).toEqual(cacheSize + 1);

element.remove();
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(cacheSize + 0);
});
});

Expand All @@ -7277,22 +7277,22 @@ describe('$compile', function() {
});

inject(function($compile, $rootScope) {
expect(jqLiteCacheSize()).toEqual(0);
var cacheSize = jqLiteCacheSize();

element = $compile('<div><div ng-repeat="x in xs" ng-if="x==1">{{x}}</div></div>')($rootScope);
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(cacheSize);

$rootScope.$apply('xs = [0,1]');
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(cacheSize);

$rootScope.$apply('xs = [0]');
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(cacheSize);

$rootScope.$apply('xs = []');
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(cacheSize);

element.remove();
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(cacheSize);
});
});

Expand All @@ -7308,26 +7308,26 @@ describe('$compile', function() {
});

inject(function($compile, $rootScope) {
expect(jqLiteCacheSize()).toEqual(0);
var cacheSize = jqLiteCacheSize();
element = $compile('<div><div ng-repeat="x in xs" ng-if="val">{{x}}</div></div>')($rootScope);

$rootScope.$apply('xs = [0,1]');
// At this point we have a bunch of comment placeholders but no real transcluded elements
// So the cache only contains the root element's data
expect(jqLiteCacheSize()).toEqual(1);
expect(jqLiteCacheSize()).toEqual(cacheSize + 1);

$rootScope.$apply('val = true');
// Now we have two concrete transcluded elements plus some comments so two more cache items
expect(jqLiteCacheSize()).toEqual(3);
expect(jqLiteCacheSize()).toEqual(cacheSize + 3);

$rootScope.$apply('val = false');
// Once again we only have comments so no transcluded elements and the cache is back to just
// the root element
expect(jqLiteCacheSize()).toEqual(1);
expect(jqLiteCacheSize()).toEqual(cacheSize + 1);

element.remove();
// Now we've even removed the root element along with its cache
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(cacheSize + 0);
});
});

Expand Down Expand Up @@ -7364,6 +7364,7 @@ describe('$compile', function() {
});

inject(function($compile, $rootScope, $httpBackend, $timeout, $templateCache) {
var cacheSize = jqLiteCacheSize();
$httpBackend.whenGET('red.html').respond('<p>red.html</p>');
var template = $compile(
'<div ng-controller="Leak">' +
Expand All @@ -7378,7 +7379,7 @@ describe('$compile', function() {
$timeout.flush();
$httpBackend.flush();
expect(linkFn).not.toHaveBeenCalled();
expect(jqLiteCacheSize()).toEqual(2);
expect(jqLiteCacheSize()).toEqual(cacheSize + 2);

$templateCache.removeAll();
var destroyedScope = $rootScope.$new();
Expand Down Expand Up @@ -8161,9 +8162,7 @@ describe('$compile', function() {

it('should not leak memory with nested transclusion', function() {
inject(function($compile, $rootScope) {
var size;

expect(jqLiteCacheSize()).toEqual(0);
var size, initialSize = jqLiteCacheSize();

element = jqLite('<div><ul><li ng-repeat="n in nums">{{n}} => <i ng-if="0 === n%2">Even</i><i ng-if="1 === n%2">Odd</i></li></ul></div>');
$compile(element)($rootScope.$new());
Expand All @@ -8177,7 +8176,7 @@ describe('$compile', function() {
expect(jqLiteCacheSize()).toEqual(size);

element.remove();
expect(jqLiteCacheSize()).toEqual(0);
expect(jqLiteCacheSize()).toEqual(initialSize);
});
});
});
Expand Down
26 changes: 26 additions & 0 deletions test/ng/documentSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,29 @@ describe('$document', function() {
});
});
});


describe('$$isDocumentHidden', function() {
it('should listen on the visibilitychange event', function() {
var doc;

var spy = spyOn(document, 'addEventListener').and.callThrough();

inject(function($$isDocumentHidden, $document) {
expect(spy.calls.mostRecent().args[0]).toBe('visibilitychange');
expect(spy.calls.mostRecent().args[1]).toEqual(jasmine.any(Function));
expect($$isDocumentHidden()).toBeFalsy(); // undefined in browsers that don't support visibility
});

});

it('should remove the listener when the $rootScope is destroyed', function() {
var spy = spyOn(document, 'removeEventListener').and.callThrough();

inject(function($$isDocumentHidden, $rootScope) {
$rootScope.$destroy();
expect(spy.calls.mostRecent().args[0]).toBe('visibilitychange');
expect(spy.calls.mostRecent().args[1]).toEqual(jasmine.any(Function));
});
});
});
25 changes: 12 additions & 13 deletions test/ngAnimate/animateSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,12 @@ describe("animations", function() {
}));

it("should skip animations entirely if the document is hidden", function() {
var doc;
var hidden = true;

module(function($provide) {
doc = jqLite({
body: document.body,
hidden: true
$provide.value('$$isDocumentHidden', function() {
return hidden;
});
$provide.value('$document', doc);
});

inject(function($animate, $rootScope) {
Expand All @@ -173,7 +171,7 @@ describe("animations", function() {
expect(capturedAnimation).toBeFalsy();
expect(element[0].parentNode).toEqual(parent[0]);

doc[0].hidden = false;
hidden = false;

$animate.leave(element);
$rootScope.$digest();
Expand Down Expand Up @@ -2503,18 +2501,19 @@ describe("animations", function() {


describe('because the document is hidden', function() {
beforeEach(module(function($provide) {
var doc = jqLite({
body: document.body,
hidden: true
var hidden = true;

beforeEach(function() {
module(function($provide) {
$provide.value('$$isDocumentHidden', function() {
return hidden;
});
});
$provide.value('$document', doc);
}));
});

it('should trigger callbacks for an enter animation',
inject(function($animate, $rootScope, $rootElement, $document) {

var callbackTriggered = false;
var spy = jasmine.createSpy();
$animate.on('enter', jqLite($document[0].body), spy);

Expand Down