Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Tooltip: Only bind remove handler for delegated tooltips #1156

Closed
wants to merge 1 commit into from

3 participants

@scottgonzalez

The remove handler was being added for every tooltip, but only removed for
delegated tooltips. The default destroy behavior already handles non-delegated
tooltips, so the handler should only be added for delegated tooltips.

Fixes #9531

I'm not sure if there's a reasonable way to test this. We don't generally test for memory leaks.

@scottgonzalez scottgonzalez Tooltip: Only bind remove handler for delegated tooltips
The remove handler was being added for every tooltip, but only removed for
delegated tooltips. The default destroy behavior already handles non-delegated
tooltips, so the handler should only be added for delegated tooltips.

Fixes #9531
853902b
@tjvantoll
Collaborator

This looks good to me. Is it worth having an _isDelegated() method to handle the duplicated target[ 0 ] !== this.element[ 0 ] check?

@scottgonzalez

Perhaps. It's a simple check, so it's unlikely to be done wrong and I'm not sure if we'll save any bytes. I'll go with whatever is smaller.

@robotdan

You beat me to the punch. :-) I have some unit tests that could be added to this if that is helpful. I'm using $._data to get the remove events, perhaps that isn't an ok thing to do in a unit test since that is an internal structure.

@scottgonzalez

Yeah, I don't want to look at $._data.

@scottgonzalez scottgonzalez closed this pull request from a commit
@scottgonzalez scottgonzalez Tooltip: Only bind remove handler for delegated tooltips
The remove handler was being added for every tooltip, but only removed for
delegated tooltips. The default destroy behavior already handles non-delegated
tooltips, so the handler should only be added for delegated tooltips.

Fixes #9531
Closes gh-1156
a8ff773
@scottgonzalez scottgonzalez deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 2, 2014
  1. @scottgonzalez

    Tooltip: Only bind remove handler for delegated tooltips

    scottgonzalez authored
    The remove handler was being added for every tooltip, but only removed for
    delegated tooltips. The default destroy behavior already handles non-delegated
    tooltips, so the handler should only be added for delegated tooltips.
    
    Fixes #9531
This page is out of date. Refresh to see the latest.
Showing with 11 additions and 4 deletions.
  1. +11 −4 ui/jquery.ui.tooltip.js
View
15 ui/jquery.ui.tooltip.js
@@ -310,11 +310,17 @@ $.widget( "ui.tooltip", {
fakeEvent.currentTarget = target[0];
this.close( fakeEvent, true );
}
- },
- remove: function() {
- this._removeTooltip( tooltip );
}
};
+
+ // Only bind remove handler for delegated targets. Non-delegated
+ // tooltips will handle this in destroy.
+ if ( target[ 0 ] !== this.element[ 0 ] ) {
+ events.remove = function() {
+ this._removeTooltip( tooltip );
+ };
+ }
+
if ( !event || event.type === "mouseover" ) {
events.mouseleave = "close";
}
@@ -352,8 +358,9 @@ $.widget( "ui.tooltip", {
target.removeData( "ui-tooltip-open" );
this._off( target, "mouseleave focusout keyup" );
+
// Remove 'remove' binding only on delegated targets
- if ( target[0] !== this.element[0] ) {
+ if ( target[ 0 ] !== this.element[ 0 ] ) {
this._off( target, "remove" );
}
this._off( this.document, "mousemove" );
Something went wrong with that request. Please try again.