Skip to content

Reopen - Suggested fix for #8740 #992

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Reopen - Suggested fix for #8740 #992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions tests/unit/tooltip/tooltip_events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,23 @@ test( "focus events", function() {
element.trigger( "focusout" );
});

// http://bugs.jqueryui.com/ticket/8740
asyncTest( "content: async callback loses focus before load", function() {
expect( 1 );
var element = $( "#tooltipped1" ).tooltip({
content: function( response ) {
element.trigger( "mouseleave" );
setTimeout(function () {
response( "sometext" );
setTimeout(function () {
ok(!$( "#" + element.data( "ui-tooltip-id" ) ).is( ":visible" ), "Tooltip should not display" );
start();
}, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2nd argument can be omitted here.

}, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

}
});
element.trigger( "mouseover" );
element.tooltip( "destroy" );
});

}( jQuery ) );
19 changes: 12 additions & 7 deletions ui/jquery.ui.tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,18 +183,21 @@ $.widget( "ui.tooltip", {
that = this,
eventType = event ? event.type : null;

this._registerCloseHandlers( event, target );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bad time to bind event handlers. The content may be changed any number of times while the tooltip is open.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottgonzalez Good catch. I verified that if you update the content with this approach, close will be called multiple times. @mziech Could you see if the close handlers could be bound in _open and still fix the underlying issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input. I implemented a test case to verify that the close handler is only called once. Indeed, it fails. Let me see whether I can fix this.

I don't think moving the close handler back to _open fixes the initial problem, since we need to detect that the user left the tooltipped element before the tooltip is actually displayed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually open (without underscore) seems like a better place to register the close handlers ...


if ( typeof contentOption === "string" ) {
return this._open( event, target, contentOption );
}

content = contentOption.call( target[0], function( response ) {
// ignore async response if tooltip was closed already
if ( !target.data( "ui-tooltip-open" ) ) {
return;
}
// IE may instantly serve a cached response for ajax requests
// delay this call to _open so the other call to _open runs first
that._delay(function() {
// ignore async response if tooltip was closed already
if ( !target.data( "ui-tooltip-open" ) ) {
return;
}

// jQuery creates a special event for focusin when it doesn't
// exist natively. To improve performance, the native event
// object is reused and the type is changed. Therefore, we can't
Expand All @@ -212,7 +215,7 @@ $.widget( "ui.tooltip", {
},

_open: function( event, target, content ) {
var tooltip, events, delayedShow,
var tooltip, delayedShow,
positionOption = $.extend( {}, this.options.position );

if ( !content ) {
Expand Down Expand Up @@ -281,8 +284,10 @@ $.widget( "ui.tooltip", {
}

this._trigger( "open", event, { tooltip: tooltip } );
},

events = {
_registerCloseHandlers: function( event, target ) {
var events = {
keyup: function( event ) {
if ( event.keyCode === $.ui.keyCode.ESCAPE ) {
var fakeEvent = $.Event(event);
Expand All @@ -291,7 +296,7 @@ $.widget( "ui.tooltip", {
}
},
remove: function() {
this._removeTooltip( tooltip );
this._removeTooltip( this._find( target ) );
}
};
if ( !event || event.type === "mouseover" ) {
Expand Down