From d71dbb1ae50f174680533492ce4c7db3ff74df00 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20Go=C5=82e=CC=A8biowski?= <m.goleb@gmail.com>
Date: Mon, 28 Apr 2014 22:23:10 +0200
Subject: [PATCH] refactor(jqLite): stop patching individual jQuery methods

Currently Angular monkey-patches a few jQuery methods that remove elements
from the DOM. Since methods like .remove() have multiple signatures
that can change what's actually removed, Angular needs to carefully
repeat them in its patching or it can break apps using jQuery correctly.
Such a strategy is also not future-safe.

Instead of patching individual methods on the prototype, it's better to
hook into jQuery.cleanData and trigger custom events there. This should be
safe as e.g. jQuery UI needs it and uses it. It'll also be future-safe.

The only drawback is that $destroy is no longer triggered when using $detach
but:

  1. Angular doesn't use this method, jqLite doesn't implement it.
  2. Detached elements can be re-attached keeping all their events & data
     so it makes sense that $destroy is not triggered on them.
  3. The approach from this commit is so much safer that any issues with
     .detach() working differently are outweighed by the robustness of the code.

BREAKING CHANGE: the $destroy event is no longer triggered when using the
jQuery detach() method. If you want to destroy Angular data attached to the
element, use remove().
---
 src/Angular.js          | 20 ++++++++++++++-----
 src/jqLite.js           | 43 -----------------------------------------
 test/jQueryPatchSpec.js | 10 ----------
 3 files changed, 15 insertions(+), 58 deletions(-)

diff --git a/src/Angular.js b/src/Angular.js
index 57f478b5b944..a3117ee435f2 100644
--- a/src/Angular.js
+++ b/src/Angular.js
@@ -1430,8 +1430,10 @@ function snake_case(name, separator){
 }
 
 function bindJQuery() {
+  var originalCleanData;
   // bind to jQuery if present;
   jQuery = window.jQuery;
+
   // reset to jQuery or default to us.
   if (jQuery) {
     jqLite = jQuery;
@@ -1442,14 +1444,22 @@ function bindJQuery() {
       injector: JQLitePrototype.injector,
       inheritedData: JQLitePrototype.inheritedData
     });
-    // Method signature:
-    //     jqLitePatchJQueryRemove(name, dispatchThis, filterElems, getterIfNoArguments)
-    jqLitePatchJQueryRemove('remove', true, true, false);
-    jqLitePatchJQueryRemove('empty', false, false, false);
-    jqLitePatchJQueryRemove('html', false, false, true);
+
+    originalCleanData = jQuery.cleanData;
+    // Prevent double-proxying.
+    originalCleanData = originalCleanData.$$original || originalCleanData;
+
+    jQuery.cleanData = function(elems) {
+      for (var i = 0, elem; (elem = elems[i]) != null; i++) {
+        jQuery(elem).triggerHandler('$destroy');
+      }
+      originalCleanData(elems);
+    };
+    jQuery.cleanData.$$original = originalCleanData;
   } else {
     jqLite = JQLite;
   }
+
   angular.element = jqLite;
 }
 
diff --git a/src/jqLite.js b/src/jqLite.js
index d03ea67b4676..771c4312432d 100644
--- a/src/jqLite.js
+++ b/src/jqLite.js
@@ -136,49 +136,6 @@ function camelCase(name) {
     replace(MOZ_HACK_REGEXP, 'Moz$1');
 }
 
-/////////////////////////////////////////////
-// jQuery mutation patch
-//
-// In conjunction with bindJQuery intercepts all jQuery's DOM destruction apis and fires a
-// $destroy event on all DOM nodes being removed.
-//
-/////////////////////////////////////////////
-
-function jqLitePatchJQueryRemove(name, dispatchThis, filterElems, getterIfNoArguments) {
-  var originalJqFn = jQuery.fn[name];
-  originalJqFn = originalJqFn.$original || originalJqFn;
-  removePatch.$original = originalJqFn;
-  jQuery.fn[name] = removePatch;
-
-  function removePatch(param) {
-    // jshint -W040
-    var list = filterElems && param ? [this.filter(param)] : [this],
-        fireEvent = dispatchThis,
-        set, setIndex, setLength,
-        element, childIndex, childLength, children;
-
-    if (!getterIfNoArguments || param != null) {
-      while(list.length) {
-        set = list.shift();
-        for(setIndex = 0, setLength = set.length; setIndex < setLength; setIndex++) {
-          element = jqLite(set[setIndex]);
-          if (fireEvent) {
-            element.triggerHandler('$destroy');
-          } else {
-            fireEvent = !fireEvent;
-          }
-          for(childIndex = 0, childLength = (children = element.children()).length;
-              childIndex < childLength;
-              childIndex++) {
-            list.push(jQuery(children[childIndex]));
-          }
-        }
-      }
-    }
-    return originalJqFn.apply(this, arguments);
-  }
-}
-
 var SINGLE_TAG_REGEXP = /^<(\w+)\s*\/?>(?:<\/\1>|)$/;
 var HTML_REGEXP = /<|&#?\w+;/;
 var TAG_NAME_REGEXP = /<([\w:]+)/;
diff --git a/test/jQueryPatchSpec.js b/test/jQueryPatchSpec.js
index 10e46be8a499..4c588092dc57 100644
--- a/test/jQueryPatchSpec.js
+++ b/test/jQueryPatchSpec.js
@@ -30,10 +30,6 @@ if (window.jQuery) {
 
     describe('$detach event', function() {
 
-      it('should fire on detach()', function() {
-        doc.find('span').detach();
-      });
-
       it('should fire on remove()', function() {
         doc.find('span').remove();
       });
@@ -83,12 +79,6 @@ if (window.jQuery) {
 
     describe('$detach event is not invoked in too many cases', function() {
 
-      it('should fire only on matched elements on detach(selector)', function() {
-        doc.find('span').detach('.second');
-        expect(spy2).toHaveBeenCalled();
-        expect(spy2.callCount).toEqual(1);
-      });
-
       it('should fire only on matched elements on remove(selector)', function() {
         doc.find('span').remove('.second');
         expect(spy2).toHaveBeenCalled();