From b9389b26ba2cf6aa70372fa32a7b28c62d174bf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Go=C5=82e=CC=A8biowski?= Date: Tue, 5 Aug 2014 14:43:06 +0200 Subject: [PATCH] fix(jQuery): cooperate with other libraries monkey-patching jQuery.cleanData Some libraries (like jQuery UI) patch jQuery.cleanData as well. This commit makes Angular work correctly even if such external patching was done after the Angular one. Fixes #8471 --- src/.jshintrc | 2 ++ src/Angular.js | 24 +++++++++---- src/ng/compile.js | 14 +++++--- test/helpers/testabilityPatch.js | 5 +++ test/ng/compileSpec.js | 61 ++++++++++++++++++++++++-------- 5 files changed, 79 insertions(+), 27 deletions(-) diff --git a/src/.jshintrc b/src/.jshintrc index 2e2b32250df6..255ad208cb1d 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -88,6 +88,8 @@ "getBlockElements": false, "VALIDITY_STATE_PROPERTY": false, + "skipDestroyOnNextJQueryCleanData": true, + /* filters.js */ "getFirstThursdayOfYear": false, diff --git a/src/Angular.js b/src/Angular.js index d1d4d70f3b31..4039d8c9ebdc 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -1454,8 +1454,15 @@ function snake_case(name, separator) { }); } +var bindJQueryFired = false; +var skipDestroyOnNextJQueryCleanData; function bindJQuery() { var originalCleanData; + + if (bindJQueryFired) { + return; + } + // bind to jQuery if present; jQuery = window.jQuery; // Use jQuery if it exists with proper functionality, otherwise default to us. @@ -1472,25 +1479,28 @@ function bindJQuery() { inheritedData: JQLitePrototype.inheritedData }); - originalCleanData = jQuery.cleanData; - // Prevent double-proxying. - originalCleanData = originalCleanData.$$original || originalCleanData; - // All nodes removed from the DOM via various jQuery APIs like .remove() // are passed through jQuery.cleanData. Monkey-patch this method to fire // the $destroy event on all removed nodes. + originalCleanData = jQuery.cleanData; jQuery.cleanData = function(elems) { - for (var i = 0, elem; (elem = elems[i]) != null; i++) { - jQuery(elem).triggerHandler('$destroy'); + if (!skipDestroyOnNextJQueryCleanData) { + for (var i = 0, elem; (elem = elems[i]) != null; i++) { + jQuery(elem).triggerHandler('$destroy'); + } + } else { + skipDestroyOnNextJQueryCleanData = false; } originalCleanData(elems); }; - jQuery.cleanData.$$original = originalCleanData; } else { jqLite = JQLite; } angular.element = jqLite; + + // Prevent double-proxying. + bindJQueryFired = true; } /** diff --git a/src/ng/compile.js b/src/ng/compile.js index 1d6c62650327..175efc13211e 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2043,11 +2043,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { 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. Note that we need to use the original method here and not the one monkey-patched by Angular - // since the patched method emits the $destroy event causing the scope to be trashed and we do need - // the very same scope to work with the new element. - jQuery.cleanData.$$original([firstElementToRemove]); + // 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]); } for (var k = 1, kk = elementsToRemove.length; k < kk; k++) { diff --git a/test/helpers/testabilityPatch.js b/test/helpers/testabilityPatch.js index 723eb6449d33..709f34781db8 100644 --- a/test/helpers/testabilityPatch.js +++ b/test/helpers/testabilityPatch.js @@ -112,6 +112,11 @@ function dealoc(obj) { function cleanup(element) { element.off().removeData(); + if (window.jQuery) { + // jQuery 2.x doesn't expose the cache storage; ensure all element data + // is removed during its cleanup. + jQuery.cleanData([element]); + } // Note: We aren't using element.contents() here. Under jQuery, element.contents() can fail // for IFRAME elements. jQuery explicitly uses (element.contentDocument || // element.contentWindow.document) and both properties are null for IFRAMES that aren't attached diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index d955835f6117..83e899345607 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4118,26 +4118,57 @@ describe('$compile', function() { }); if (jQuery) { - it('should clean up after a replaced element', inject(function ($compile) { - var privateData, firstRepeatedElem; + describe('cleaning up after a replaced element', function () { + var $compile, xs; + beforeEach(inject(function (_$compile_) { + $compile = _$compile_; + xs = [0, 1]; + })); - element = $compile('
{{x}}
')($rootScope); + function testCleanup() { + var privateData, firstRepeatedElem; - $rootScope.$apply('xs = [0,1]'); - firstRepeatedElem = element.children('.ng-scope').eq(0); + element = $compile('
{{x}}
')($rootScope); - expect(firstRepeatedElem.data('$scope')).toBeDefined(); - privateData = jQuery._data(firstRepeatedElem[0]); - expect(privateData.events).toBeDefined(); - expect(privateData.events.$destroy).toBeDefined(); - expect(privateData.events.$destroy[0]).toBeDefined(); + $rootScope.$apply('xs = [' + xs + ']'); + firstRepeatedElem = element.children('.ng-scope').eq(0); - $rootScope.$apply('xs = null'); + expect(firstRepeatedElem.data('$scope')).toBeDefined(); + privateData = jQuery._data(firstRepeatedElem[0]); + expect(privateData.events).toBeDefined(); + expect(privateData.events.$destroy).toBeDefined(); + expect(privateData.events.$destroy[0]).toBeDefined(); - expect(firstRepeatedElem.data('$scope')).not.toBeDefined(); - privateData = jQuery._data(firstRepeatedElem[0]); - expect(privateData && privateData.events).not.toBeDefined(); - })); + $rootScope.$apply('xs = null'); + + expect(firstRepeatedElem.data('$scope')).not.toBeDefined(); + privateData = jQuery._data(firstRepeatedElem[0]); + expect(privateData && privateData.events).not.toBeDefined(); + } + + it('should work without external libraries (except jQuery)', testCleanup); + + it('should work with another library patching jQuery.cleanData after Angular', function () { + var cleanedCount = 0; + var currentCleanData = jQuery.cleanData; + jQuery.cleanData = function (elems) { + cleanedCount += elems.length; + // Don't return the output and expicitly pass only the first parameter + // so that we're sure we're not relying on either of them. jQuery UI patch + // behaves in this way. + currentCleanData(elems); + }; + + testCleanup(); + + // The initial ng-repeat div is dumped after parsing hence we expect cleanData + // count to be one larger than size of the iterated array. + expect(cleanedCount).toBe(xs.length + 1); + + // Restore the previous jQuery.cleanData. + jQuery.cleanData = currentCleanData; + }); + }); }