-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
View hooks for native and non-jQuery libraries #3003
Changes from all commits
02d270f
e9543ee
aacd0e1
19d82b8
846de91
54252e6
33e23df
8ab8646
31bb428
15cb64e
09d3762
f872032
7c46856
aca58b7
e14a903
be3a354
bde634f
fc95eae
3cef7fa
2b19b00
c80be41
557edc7
933c02e
6fec7a5
6433043
7eedd4b
0cdb38b
90c36ca
4e5b282
be1cba3
3369bf4
ad44cb4
5f66f9a
d78fb34
1e9cd4a
9028e33
6ebb6e1
18d17db
390275b
9868bfb
25c0bee
12abe8e
72b88b5
20935c8
5dc2d09
3edf74c
e30e6b9
446ee52
fd64254
f13181f
01f794f
a9d066a
70933e5
0fca38c
7339bed
fca75fb
6cef2f1
81809e1
7e95b9c
c5aa603
986540f
1cb19be
2711185
6d69999
9eff496
bef78ef
c62708f
d9f6c64
58c9a2a
b840669
393a209
b377e30
20ed074
be11f3a
d21fd79
d99e43b
3850d37
b59b621
291f3bc
764eef4
264bd9b
c5c0d2e
06586f3
f67c7a1
3f79ea8
4e8caf4
ad39972
b79bd78
72f7da4
155a738
7180d5c
1d6eb7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1065,21 +1065,37 @@ | |
// Remove this view by taking the element out of the DOM, and removing any | ||
// applicable Backbone.Events listeners. | ||
remove: function() { | ||
this.$el.remove(); | ||
this._removeElement(); | ||
this.stopListening(); | ||
return this; | ||
}, | ||
|
||
// Change the view's element (`this.el` property), including event | ||
// re-delegation. | ||
// Remove this view's element from the document and all event listeners | ||
// attached to it. Exposed for subclasses using an alternative DOM | ||
// manipulation API. | ||
_removeElement: function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize this check was removed due to the dislike of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking for this.el failed some tests before the test refactoring. Sent from my iPhone
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see now. |
||
this.$el.remove(); | ||
}, | ||
|
||
// Change the view's element (`this.el` property) and re-delegate the | ||
// view's events on the new element. | ||
setElement: function(element) { | ||
if (this.$el) this.undelegateEvents(); | ||
this.$el = element instanceof Backbone.$ ? element : Backbone.$(element); | ||
this.el = this.$el[0]; | ||
this.undelegateEvents(); | ||
this._setElement(element); | ||
this.delegateEvents(); | ||
return this; | ||
}, | ||
|
||
// Creates the `this.el` and `this.$el` references for this view using the | ||
// given `el` and a hash of `attributes`. `el` can be a CSS selector or an | ||
// HTML string, a jQuery context or an element. Subclasses can override | ||
// this to utilize an alternative DOM manipulation API and are only required | ||
// to set the `this.el` property. | ||
_setElement: function(el) { | ||
this.$el = el instanceof Backbone.$ ? el : Backbone.$(el); | ||
this.el = this.$el[0]; | ||
}, | ||
|
||
// Set callbacks, where `this.events` is a hash of | ||
// | ||
// *{"event selector": "callback"}* | ||
|
@@ -1093,37 +1109,40 @@ | |
// pairs. Callbacks will be bound to the view, with `this` set properly. | ||
// Uses event delegation for efficiency. | ||
// Omitting the selector binds the event to `this.el`. | ||
// This only works for delegate-able events: not `focus`, `blur`, and | ||
// not `change`, `submit`, and `reset` in Internet Explorer. | ||
delegateEvents: function(events) { | ||
if (!(events || (events = _.result(this, 'events')))) return this; | ||
this.undelegateEvents(); | ||
for (var key in events) { | ||
var method = events[key]; | ||
if (!_.isFunction(method)) method = this[events[key]]; | ||
if (!method) continue; | ||
|
||
var match = key.match(delegateEventSplitter); | ||
var eventName = match[1], selector = match[2]; | ||
method = _.bind(method, this); | ||
eventName += '.delegateEvents' + this.cid; | ||
if (selector === '') { | ||
this.$el.on(eventName, method); | ||
} else { | ||
this.$el.on(eventName, selector, method); | ||
} | ||
this.delegate(match[1], match[2], _.bind(method, this)); | ||
} | ||
return this; | ||
}, | ||
|
||
// Clears all callbacks previously bound to the view with `delegateEvents`. | ||
// Add a single event listener to the view's element (or a child element | ||
// using `selector`). This only works for delegate-able events: not `focus`, | ||
// `blur`, and not `change`, `submit`, and `reset` in Internet Explorer. | ||
delegate: function(eventName, selector, listener) { | ||
this.$el.on(eventName + '.delegateEvents' + this.cid, selector, listener); | ||
}, | ||
|
||
// Clears all callbacks previously bound to the view by `delegateEvents`. | ||
// You usually don't need to use this, but may wish to if you have multiple | ||
// Backbone views attached to the same DOM element. | ||
undelegateEvents: function() { | ||
this.$el.off('.delegateEvents' + this.cid); | ||
if (this.$el) this.$el.off('.delegateEvents' + this.cid); | ||
return this; | ||
}, | ||
|
||
// A finer-grained `undelegateEvents` for removing a single delegated event. | ||
// `selector` and `listener` are both optional. | ||
undelegate: function(eventName, selector, listener) { | ||
this.$el.off(eventName + '.delegateEvents' + this.cid, selector, listener); | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why include this if it's never called? If you're not using jQuery you'd have to override anyway, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's to complement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree. Providing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unused currently because jQuery provides a way to implement {
undelegateEvents: function() {
this.undelegate('.delegateEvents' + this.cid);
return this;
},
undelegate: function(eventName, selector, listener) {
if (this.$el) this.$el.off(eventName, selector, listener);
}
} Though we're still going back and forth between namespaces( #3003 (comment) , #3003 (comment) ) . If we end up not using them any longer, I imagine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but that's not the way it's implemented here. And further, I don't think it needs to be. The reason for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you remove just one handler that was added by delegate then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need to? Events are cleaned up when you remove the view via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For plugins. Stickit has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think @braddunbar is saying you can already track down the reference by looping the event-selector-handler combos. This is fine by me, thought it prevents efficient and direct removal with removeEventListener for native implementations. Price to pay for removing one line. Sent from my iPhone
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
// Ensure that the View has a DOM element to render into. | ||
// If `this.el` is a string, pass it through `$()`, take the first | ||
// matching element, and re-assign it to `el`. Otherwise, create | ||
|
@@ -1133,11 +1152,17 @@ | |
var attrs = _.extend({}, _.result(this, 'attributes')); | ||
if (this.id) attrs.id = _.result(this, 'id'); | ||
if (this.className) attrs['class'] = _.result(this, 'className'); | ||
var $el = Backbone.$('<' + _.result(this, 'tagName') + '>').attr(attrs); | ||
this.setElement($el); | ||
this.setElement(document.createElement(_.result(this, 'tagName'))); | ||
this._setAttributes(attrs); | ||
} else { | ||
this.setElement(_.result(this, 'el')); | ||
} | ||
}, | ||
|
||
// Set attributes from a hash on this view's element. Exposed for | ||
// subclasses using an alternative DOM manipulation API. | ||
_setAttributes: function(attributes) { | ||
this.$el.attr(attributes); | ||
} | ||
|
||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive me if I'm rehashing something that's already been discussed, but
#undelegateEvents
is covered by#_removeElement
, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems right to me. @akre54?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if the DOM lib is jQuery. You'll need to undelegate events explicitly for everyone else so _removeElement can just focus on just detaching the node. Maybe we should probably renamed it to _detach or _detachElement because that's what it's really doing.
Sent from my iPhone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DOM lib is jQuery. Everyone else can just call
undelegateEvents
in their overridden copy of_removeElement
, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this. In the default case we'd be removing events twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the debate, but if the argument is about
this.$el.remove()
vsthis.$el.detach()
I'm in theremove
camp.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akre54: @jashkenas and @braddunbar want to remove that line of
this.undelegateEvents()
insideremove
, but I'm saying I put it there for a reason because basically every other implementation will have to callundelegateEvents
to clean up at the minimum the handlers View attached. @jashkenas and @braddunbar are saying we should just use$.fn.remove
for GC.I slept on this and I still think that you just can't guarantee
View#remove
will clean up everything immediately. Here's why:While the above will still GC
el
andclickHandler
in sane browsers, you can't guarantee that event handlers attached outside ofdelegateEvents
will be detached even today and you can still triggerclickHandler
before the whole thing gets GC'ed. So I think the tests forremove
shouldn't even try to guarantee that either. You can keep $.fn.remove for backward compatibility, but the tests shouldn't try to guarantee that all event handlers will be detached immediately because it's actually not possible, especially for implementations where jQuery isn't available.Since we can only guarantee cleaning up the event handlers
delegate
attached, callingundelegateEvent
insideremove
becomes a "necessary convenience".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to clean up all event handlers, just the ones set up using
Backbone.$
onel
and it's descendants. Regardless of what's in#remove
, this will be the contract for#_removeElement
and thus the extra call is just redundant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you insist, what do you think if I removed
undelegateEvents
from#remove
, renamed_removeElement
to_remove
and fixed up its test to test for the removal of delegated handlers? I prefer not because removing 2 + 2 handlers separately = removing 4 handlers in one go, so it's just added convenience. I have no strong feeling against removing thatundelegateEvents
line otherwise.I think we should document that if you want to be forward compatible with other View classes, you should use
delegate
instead of$.fn.on
, but that's another ticket when we agree that's actually what everyone wants.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Then in that case I agree with Brad and Jeremy. I argued for this point before and I still believe it should be the responsibility of
remove
.