From 96288d02d316ff1508b2e0673f34291f9abd1c7f Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 9 Jun 2015 01:07:50 -0700 Subject: [PATCH] refactor($compile): remove skipDestroyOnNextJQueryCleanData, remove jq data of all replaced nodes Closes: #12094 --- src/.jshintrc | 2 - src/Angular.js | 13 ++--- src/jqLite.js | 9 ++- src/ng/compile.js | 40 ++++++------- test/ng/compileSpec.js | 126 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 153 insertions(+), 37 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index e5ce17e43234..07fcb9162fd6 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -94,8 +94,6 @@ "VALIDITY_STATE_PROPERTY": false, "reloadWithDebugInfo": false, - "skipDestroyOnNextJQueryCleanData": true, - "NODE_TYPE_ELEMENT": false, "NODE_TYPE_ATTRIBUTE": false, "NODE_TYPE_TEXT": false, diff --git a/src/Angular.js b/src/Angular.js index 288dadc05508..3eae1367afe2 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -1678,7 +1678,6 @@ function snake_case(name, separator) { } var bindJQueryFired = false; -var skipDestroyOnNextJQueryCleanData; function bindJQuery() { var originalCleanData; @@ -1712,15 +1711,11 @@ function bindJQuery() { originalCleanData = jQuery.cleanData; jQuery.cleanData = function(elems) { var events; - if (!skipDestroyOnNextJQueryCleanData) { - for (var i = 0, elem; (elem = elems[i]) != null; i++) { - events = jQuery._data(elem, "events"); - if (events && events.$destroy) { - jQuery(elem).triggerHandler('$destroy'); - } + for (var i = 0, elem; (elem = elems[i]) != null; i++) { + events = jQuery._data(elem, "events"); + if (events && events.$destroy) { + jQuery(elem).triggerHandler('$destroy'); } - } else { - skipDestroyOnNextJQueryCleanData = false; } originalCleanData(elems); }; diff --git a/src/jqLite.js b/src/jqLite.js index 80680ac55469..361d865202f1 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -189,6 +189,12 @@ function jqLiteHasData(node) { return false; } +function jqLiteCleanData(nodes) { + for (var i = 0, ii = nodes.length; i < ii; i++) { + jqLiteRemoveData(nodes[i]); + } +} + function jqLiteBuildFragment(html, context) { var tmp, tag, wrap, fragment = context.createDocumentFragment(), @@ -571,7 +577,8 @@ function getAliasedAttrName(name) { forEach({ data: jqLiteData, removeData: jqLiteRemoveData, - hasData: jqLiteHasData + hasData: jqLiteHasData, + cleanData: jqLiteCleanData }, function(fn, name) { JQLite[name] = fn; }); diff --git a/src/ng/compile.js b/src/ng/compile.js index ecdcd979fba0..8b6491091de7 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2634,9 +2634,14 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { parent.replaceChild(newNode, firstElementToRemove); } - // TODO(perf): what's this document fragment for? is it needed? can we at least reuse it? + // Append all the `elementsToRemove` to a fragment. This will... + // - remove them from the DOM + // - allow them to still be traversed with .nextSibling + // - allow a single fragment.qSA to fetch all elements being removed var fragment = document.createDocumentFragment(); - fragment.appendChild(firstElementToRemove); + for (i = 0; i < removeCount; i++) { + fragment.appendChild(elementsToRemove[i]); + } if (jqLite.hasData(firstElementToRemove)) { // Copy over user data (that includes Angular's $scope etc.). Don't copy private @@ -2644,31 +2649,18 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // event listeners (which is the main use of private data) wouldn't work anyway. jqLite.data(newNode, jqLite.data(firstElementToRemove)); - // Remove data of the replaced element. We cannot just call .remove() - // on the element it since that would deallocate scope that is needed - // for the new node. Instead, remove the data "manually". - if (!jQuery) { - delete jqLite.cache[firstElementToRemove[jqLite.expando]]; - } else { - // jQuery 2.x doesn't expose the data storage. Use jQuery.cleanData to clean up after - // the replaced element. The cleanData version monkey-patched by Angular would cause - // the scope to be trashed and we do need the very same scope to work with the new - // element. However, we cannot just cache the non-patched version and use it here as - // that would break if another library patches the method after Angular does (one - // example is jQuery UI). Instead, set a flag indicating scope destroying should be - // skipped this one time. - skipDestroyOnNextJQueryCleanData = true; - jQuery.cleanData([firstElementToRemove]); - } + // Remove $destroy event listeners from `firstElementToRemove` + jqLite(firstElementToRemove).off('$destroy'); } - for (var k = 1, kk = elementsToRemove.length; k < kk; k++) { - var element = elementsToRemove[k]; - jqLite(element).remove(); // must do this way to clean up expando - fragment.appendChild(element); - delete elementsToRemove[k]; - } + // Cleanup any data/listeners on the elements and children. + // This includes invoking the $destroy event on any elements with listeners. + jqLite.cleanData(fragment.querySelectorAll('*')); + // Update the jqLite collection to only contain the `newNode` + for (i = 1; i < removeCount; i++) { + delete elementsToRemove[i]; + } elementsToRemove[0] = newNode; elementsToRemove.length = 1; } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 934cc1b0f69f..da016f74c5ac 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -5844,7 +5844,9 @@ describe('$compile', function() { testCleanup(); - expect(cleanedCount).toBe(xs.length); + // The ng-repeat template is removed/cleaned (the +1) + // and each clone of the ng-repeat template is also removed (xs.length) + expect(cleanedCount).toBe(xs.length + 1); // Restore the previous jQuery.cleanData. jQuery.cleanData = currentCleanData; @@ -8587,4 +8589,126 @@ describe('$compile', function() { expect(element.hasClass('fire')).toBe(true); })); }); + + describe('element replacement', function() { + it('should broadcast $destroy only on removed elements, not replaced', function() { + var linkCalls = []; + var destroyCalls = []; + + module(function($compileProvider) { + $compileProvider.directive('replace', function() { + return { + multiElement: true, + replace: true, + templateUrl: 'template123' + }; + }); + + $compileProvider.directive('foo', function() { + return { + priority: 1, // before the replace directive + link: function($scope, $element, $attrs) { + linkCalls.push($attrs.foo); + $element.on('$destroy', function() { + destroyCalls.push($attrs.foo); + }); + } + }; + }); + }); + + inject(function($compile, $templateCache, $rootScope) { + $templateCache.put('template123', '

'); + + $compile( + '
' + + '
' + + '
' + )($rootScope); + + expect(linkCalls).toEqual(['2', '3']); + expect(destroyCalls).toEqual([]); + $rootScope.$apply(); + expect(linkCalls).toEqual(['2', '3', '1']); + expect(destroyCalls).toEqual(['2', '3']); + }); + }); + + function getAll($root) { + // check for .querySelectorAll to support comment nodes + return [$root[0]].concat($root[0].querySelectorAll ? sliceArgs($root[0].querySelectorAll('*')) : []); + } + + function testCompileLinkDataCleanup(template) { + inject(function($compile, $rootScope) { + var toCompile = jqLite(template); + + var preCompiledChildren = getAll(toCompile); + forEach(preCompiledChildren, function(element, i) { + jqLite.data(element, 'foo', 'template#' + i); + }); + + var linkedElements = $compile(toCompile)($rootScope); + $rootScope.$apply(); + linkedElements.remove(); + + forEach(preCompiledChildren, function(element, i) { + expect(jqLite.hasData(element)).toBe(false, "template#" + i); + }); + forEach(getAll(linkedElements), function(element, i) { + expect(jqLite.hasData(element)).toBe(false, "linked#" + i); + }); + }); + } + it('should clean data of element-transcluded link-cloned elements', function() { + testCompileLinkDataCleanup('
'); + }); + it('should clean data of element-transcluded elements', function() { + testCompileLinkDataCleanup('
'); + }); + + function testReplaceElementCleanup(dirOptions) { + var template = '
'; + module(function($compileProvider) { + $compileProvider.directive('theDir', function() { + return { + multiElement: true, + replace: dirOptions.replace, + transclude: dirOptions.transclude, + template: dirOptions.asyncTemplate ? undefined : template, + templateUrl: dirOptions.asyncTemplate ? 'the-dir-template-url' : undefined + }; + }); + }); + inject(function($templateCache, $compile, $rootScope) { + $templateCache.put('the-dir-template-url', template); + + testCompileLinkDataCleanup( + '
' + + '
' + + '
' + + '
' + + '
' + ); + }); + } + it('should clean data of elements removed for directive template', function() { + testReplaceElementCleanup({}); + }); + it('should clean data of elements removed for directive templateUrl', function() { + testReplaceElementCleanup({asyncTmeplate: true}); + }); + it('should clean data of elements transcluded into directive template', function() { + testReplaceElementCleanup({transclude: true}); + }); + it('should clean data of elements transcluded into directive templateUrl', function() { + testReplaceElementCleanup({transclude: true, asyncTmeplate: true}); + }); + it('should clean data of elements replaced with directive template', function() { + testReplaceElementCleanup({replace: true}); + }); + it('should clean data of elements replaced with directive templateUrl', function() { + testReplaceElementCleanup({replace: true, asyncTemplate: true}); + }); + }); });