Skip to content
Browse files

Add `Element.purge` for cleaning up event listeners and element stora…

…ge keys on elements that will be removed from the page. Make `Element.update` perform similar cleanup automatically. (Andrew Dupont, Tobie Langel)
  • Loading branch information...
1 parent 38e85b8 commit c9d6c3ee14747179a1038bc21055f2d6ccd492c4 @savetheclocktower savetheclocktower committed May 7, 2010
Showing with 83 additions and 7 deletions.
  1. +6 −4 CHANGELOG
  2. +47 −3 src/dom/dom.js
  3. +30 −0 test/unit/dom_test.js
View
10 CHANGELOG
@@ -1,10 +1,12 @@
-Fix issue where `Element.Layout#get` would fail to interpret negative pixel values. (Sebastien Gruhier, Andrew Dupont)
+* Add `Element.purge` for cleaning up event listeners and element storage keys on elements that will be removed from the page. Make `Element.update` perform similar cleanup automatically. (Andrew Dupont, Tobie Langel)
-Fix bugs in layout.js. Add tests for `Element.Layout#toCSS`, `#toObject`, and `#toHash`. (RStankov, Andrew Dupont)
+* Fix issue where `Element.Layout#get` would fail to interpret negative pixel values. (Sebastien Gruhier, Andrew Dupont)
-Add `Element.Layout#toObject` and `Element.Layout.toHash`. (Andrew Dupont)
+* Fix bugs in layout.js. Add tests for `Element.Layout#toCSS`, `#toObject`, and `#toHash`. (RStankov, Andrew Dupont)
-Make `Element.Layout#toCSS` return camelized property names, as expected by `Element.setStyle`. [#1021 state:resolved] (njakobsen, Andrew Dupont)
+* Add `Element.Layout#toObject` and `Element.Layout.toHash`. (Andrew Dupont)
+
+* Make `Element.Layout#toCSS` return camelized property names, as expected by `Element.setStyle`. [#1021 state:resolved] (njakobsen, Andrew Dupont)
*1.7_rc1* (April 1, 2010)
View
50 src/dom/dom.js
@@ -189,6 +189,19 @@ if (!Node.ELEMENT_NODE) {
Element.idCounter = 1;
Element.cache = { };
+// Performs cleanup on an element before it is removed from the page.
+// See `Element#purge`.
+function purgeElement(element) {
+ // Must go first because it relies on Element.Storage.
+ Element.stopObserving(element);
+
+ var uid = element._prototypeUID;
+ if (uid) {
+ element._prototypeUID = void 0;
+ delete Element.Storage[uid];
+ }
+}
+
@RStankov
RStankov added a note May 12, 2010

Since the event handlers are stored in Element.Storage, if element have event handlers it will have uid, so Element.stopObserving(element); could be moved after if (uid){} before element._prototypeUID = void 0;. And in this way it will speedup this method for elements who don't have storage, for them it will be just a empty call

@savetheclocktower
Collaborator

Works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
/**
* mixin Element.Methods
*
@@ -449,6 +462,11 @@ Element.Methods = {
* Note that this method allows seamless content update of table related
* elements in Internet Explorer 6 and beyond.
*
+ * Any nodes replaced with `Element.update` will first have event
+ * listeners unregistered and storage keys removed. This frees up memory
+ * and prevents leaks in certain versions of Internet Explorer. (See
+ * [[Element.purge]]).
+ *
* ##### Examples
*
* language: html
@@ -551,7 +569,14 @@ Element.Methods = {
function update(element, content) {
element = $(element);
-
+
+ // Purge the element's existing contents of all storage keys and
+ // event listeners, since said content will be replaced no matter
+ // what.
+ var descendants = Element.select(element, '*'),
+ i = descendants.length;
+ while (i--) purgeElement(descendants[i]);
+
@RStankov
RStankov added a note May 12, 2010

I'm wondering if here we could move this into a method purgeElementDescendants, because here and in purge method are these 3 lines.
Plus for speed I think you could element.getElementsByTagName('*') so it won't pass through selector engine and whole bunch of unneeded stuff ( + on IE it will extend all element, and this is not needed here, since they will be removed any way)

@savetheclocktower
Collaborator

Works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if (content && content.toElement)
content = content.toElement();
@@ -3715,8 +3740,8 @@ Element.addMethods({
uid = 0;
} else {
if (typeof element._prototypeUID === "undefined")
- element._prototypeUID = [Element.Storage.UID++];
- uid = element._prototypeUID[0];
+ element._prototypeUID = Element.Storage.UID++;
@kangax
Collaborator
kangax added a note May 8, 2010

Doesn't making "_prototypeUID" a primitive affects html representation in IE (outerHTML, etc.)?

@tobie
Collaborator
tobie added a note May 8, 2010

It was essentially an issue with cloning, if I remember correctly.

@kangax
Collaborator
kangax added a note May 8, 2010

Well IIRC, cloneNode in IE clones elements in such way that expandos reference original objects, so that could definitely be a problem. How about making Element#clone clean-up "_prototypeUID"?

@tobie
Collaborator
tobie added a note May 8, 2010

I pretty much think that's what it does already.

@kangax
Collaborator
kangax added a note May 8, 2010

Ok. So there are no issues with leaving property as non-primitive then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ uid = element._prototypeUID;
}
if (!Element.Storage[uid])
@@ -3786,5 +3811,24 @@ Element.addMethods({
}
}
return Element.extend(clone);
+ },
+
+ /**
+ * Element.purge(@element) -> null
+ *
+ * Removes all event listeners and storage keys from an element.
+ *
+ * To be used just before removing an element from the page.
+ **/
+ purge: function(element) {
+ if (!(element = $(element))) return;
+ purgeElement(element);
+
+ var descendants = Element.select(element, '*'),
+ i = descendants.length;
+
+ while (i--) purgeElement(descendants[i]);
+
+ return null;
@RStankov
RStankov added a note May 25, 2010

Here should be "return element", In most of my code I expect to do $(element).purge().remove();

@savetheclocktower
Collaborator

My thinking here was that return null would prevent chaining on purpose, as if to emphasize that you shouldn't be calling any methods on this element after it's purged. So you'd do $(element).remove().purge(). But then that means the documentation is wrong and should say "To be used just after removing an element from the page."

I'm happy to change it so that it returns the element, though. I'll check with Tobie and Sam and see if they have an opinion.

@madrobby
Collaborator
madrobby added a note May 25, 2010

+1 for chaining in this case. The order really shouldn't matter.

Maybe we can add a destroy() convenience method to purge and remove at the same time?

@tobie
Collaborator
tobie added a note May 25, 2010

I think returning the element is more consistant. Calling a method on the element after having purged it doesn't have any side effects, so it's ok to do that. And yes, adding Element#destroy was sort of planned for a future release.

@savetheclocktower
Collaborator

Adding destroy was discussed, too, but because this happened so late in the 1.7 release cycle Tobie and I decided to add as few methods as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
});
View
30 test/unit/dom_test.js
@@ -1501,6 +1501,36 @@ new Test.Unit.Runner({
this.assert(deepClone.firstChild);
this.assertEqual('SPAN', deepClone.firstChild.nodeName.toUpperCase());
this.assert(!deepClone.down('span')._prototypeUID);
+ },
+
+ testElementPurge: function() {
+ var element = new Element('div');
+ element.store('foo', 'bar');
+
+ var uid = element._prototypeUID;
+ this.assert(uid in Element.Storage, "newly-created element's uid should exist in `Element.Storage`");
+
+ element.purge();
+
+ this.assert(!(uid in Element.Storage), "purged element's UID should no longer exist in `Element.Storage`");
+ this.assert(!(Object.isNumber(element._prototypeUID)), "purged element's UID should no longer exist in `Element.Storage`");
+
+ // Should purge elements replaced via innerHTML.
+ var parent = new Element('div');
+ var child = new Element('p').update('lorem ipsum');
+
+ parent.insert(child);
+ child.store('foo', 'bar');
+ child.observe('test:event', function(event) { event.stop(); });
+ var childUID = child._prototypeUID;
+
+ parent.update("");
+
+ // At this point, `child` should have been purged.
+ this.assert(!(childUID in Element.Storage), "purged element's UID should no longer exist in `Element.Storage`");
+
+ var event = child.fire('test:event');
+ this.assert(!event.stopped, "fired event should not have been stopped");
}
});

0 comments on commit c9d6c3e

Please sign in to comment.
Something went wrong with that request. Please try again.