Skip to content

2nd Reopen - Suggested fix for #8740 #1053

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 7 commits into from
Closed

2nd Reopen - Suggested fix for #8740 #1053

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f3204fe
Tooltip: Register event handlers before content is loaded. Fixes #874…
mziech Aug 10, 2013
0520a49
Tooltip tests: jshint cleanup
mziech Aug 14, 2013
4996bb2
Merge remote-tracking branch 'upstream/master' into fix-8740
mziech Aug 14, 2013
3306ec5
Merge branch 'master' of git://github.com/jquery/jquery-ui into fix-8740
mziech Feb 9, 2014
45a6e90
Tooltip: Clean up after merge conflict. Fixes #8740 - Tooltip: Does n…
mziech Feb 9, 2014
556559c
Tooltip tests: Code style cleanup. Fixes #8740 - Tooltip: Does not hi…
mziech Feb 9, 2014
ebea3df
Merge branch 'master' of git://github.com/jquery/jquery-ui into fix-8740
mziech Feb 27, 2014
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
42 changes: 42 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,46 @@ test( "focus events", function() {
element.trigger( "focusout" );
});

// http://bugs.jqueryui.com/ticket/8740
asyncTest( "content: async callback loses focus before load", function() {
Copy link
Member

Choose a reason for hiding this comment

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

This test belongs in tooltip_options.js, it's not testing any events.

expect( 1 );
var element = $( "#tooltipped1" ).tooltip({
Copy link
Member

Choose a reason for hiding this comment

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

Blank line between expect() and var.

content: function( response ) {
element.trigger( "mouseleave" );
setTimeout(function () {
Copy link
Member

Choose a reason for hiding this comment

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

No space between function and ().

response( "sometext" );
setTimeout(function () {
ok(!$( "#" + element.data( "ui-tooltip-id" ) ).is( ":visible" ), "Tooltip should not display" );
Copy link
Member

Choose a reason for hiding this comment

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

This line exceeds the 100 character limit and is missing a space after ok(.

start();
});
});
}
});
element.trigger( "mouseover" );
element.tooltip( "destroy" );
Copy link
Member

Choose a reason for hiding this comment

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

Why are you destroying the tooltip before the test finishes?

});

// https://github.com/jquery/jquery-ui/pull/992/files#r5667799
asyncTest( "content: close should only be called once, even if content is set multiple times", function() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we actually need this test.

expect( 1 );
var element = $( "#tooltipped1" ).tooltip(),
closecount = 0;
element.bind( "tooltipopen", function() {
element.tooltip( "option", "content", "one" );
element.tooltip( "option", "content", "two" );
element.trigger( "mouseleave" );
});
element.bind( "tooltipclose", function() {
closecount++;
if (closecount === 1) {
setTimeout(function () {
equal( closecount, 1, "Close event handler should be called once" );
element.tooltip( "destroy" );
start();
});
}
});
element.trigger( "mouseover" );
});

}( jQuery ) );
18 changes: 11 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 @@ -174,6 +174,7 @@ $.widget( "ui.tooltip", {
});
}

this._registerCloseHandlers( event, target );
this._updateContent( target, event );
},

Expand All @@ -188,13 +189,14 @@ $.widget( "ui.tooltip", {
}

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 +214,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 +283,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 +295,7 @@ $.widget( "ui.tooltip", {
}
},
remove: function() {
this._removeTooltip( tooltip );
this._removeTooltip( this._find( target ) );
}
};
if ( !event || event.type === "mouseover" ) {
Expand Down