Skip to content

Event: Warn and fill event aliases #243

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions src/event.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,20 @@ jQuery.each( [ "load", "unload", "error" ], function( _, name ) {

} );

jQuery.each( ( "blur focus focusin focusout resize scroll click dblclick " +
"mousedown mouseup mousemove mouseover mouseout mouseenter mouseleave " +
"change select submit keydown keypress keyup contextmenu" ).split( " " ),
function( i, name ) {

// Handle event binding
jQuery.fn[ name ] = function( data, fn ) {
migrateWarn( "jQuery.fn." + name + "() event shorthand is deprecated" );
return arguments.length > 0 ?
Copy link
Member

Choose a reason for hiding this comment

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

Can we remember the old functions and call them instead duplicating logic in core?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? This code should work even if event shorthands are removed from Core.

Copy link
Member

Choose a reason for hiding this comment

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

As far as i know we don't plan to remove it. And if/when we remove it, warning should be changed regardless

Copy link
Member

Choose a reason for hiding this comment

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

It will be missing if someone compiles the deprecated module out. @dmethvin does Migrate support such a scenario?

Copy link
Member

Choose a reason for hiding this comment

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

It seems reimplementing is more preferable, judging by the rest of the source anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

I think our approach for 3.0 has been to implement the deprecated functionality even if it's still in core. It does handle the scenario of custom builds better i guess.

this.on( name, null, data, fn ) :
this.trigger( name );
};
} );

// Trigger "ready" event only once, on document ready
jQuery( function() {
jQuery( window.document ).triggerHandler( "ready" );
Expand Down Expand Up @@ -107,5 +121,9 @@ jQuery.fn.extend( {
return arguments.length === 1 ?
this.off( selector, "**" ) :
this.off( types, selector || "**", fn );
},
hover: function( fnOver, fnOut ) {
migrateWarn( "jQuery.fn.hover() is deprecated" );
return this.on( "mouseenter", fnOver ).on( "mouseleave", fnOut || fnOver );
}
} );
33 changes: 33 additions & 0 deletions test/event.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,39 @@ QUnit.test( ".delegate() and .undelegate()", function( assert ) {
} );
} );

QUnit.test( "Event aliases", function( assert) {
assert.expect( 14 );

var $div = jQuery( "<div />" );

"scroll click submit keydown".split( " " ).forEach( function( name ) {
expectWarning( "." + name + "()", 1, function() {
$div[ name ]( function( event ) {
assert.equal( event.type, name, name );
$div.off( event );
} )[ name ]();
} );
} );

expectWarning( ".hover() one-arg", 1, function() {
$div.hover( function( event ) {
assert.ok( /mouseenter|mouseleave/.test( event.type ), event.type );
$div.off( event );
} ).trigger( "mouseenter" ).trigger( "mouseleave" );
} );

expectWarning( ".hover() two-arg", 1, function() {
$div.hover(
function( event ) {
assert.equal( "mouseenter", event.type, event.type );
},
function( event ) {
assert.equal( "mouseleave", event.type, event.type );
}
).trigger( "mouseenter" ).trigger( "mouseleave" );
} );
} );

test( "custom ready", function( assert ) {
assert.expect( 2 );

Expand Down
12 changes: 12 additions & 0 deletions warnings.md
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,15 @@ See jQuery-ui [commit](https://github.com/jquery/jquery-ui/commit/c0093b599fcd58
**Cause:** The calling code has attempted to attach a `load` event to `window` after the page has already loaded. That means the handler will never run and so is probably not what the caller intended. This can occur when the event attachment is made too late, for example, in a jQuery ready handler. It can also occur when a file is loaded dynamically with jQuery after the page has loaded, for example using the `$.getScript()` method.

**Solution:** If a function `fn` does not actually depend on all page assets being fully loaded, switch to a ready handler `$( fn )` which runs earlier and will aways run `fn` even if the script that contains the code loads long after the page has fully loaded. If `fn` actually does depend on the script being fully loaded, check `document.readyState`. If the value is `"complete"` run the function immediately, otherwise use `$(window).on( "load", fn )`.

### JQMIGRATE: jQuery.fn.click() event shorthand is deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Why not generalize this header?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to figure out the best way to do this. My concern with generalizing the header was that it wouldn't be an exact match. The most common shorthand (I think) is click so i figured this approach would yield the best results with the others in the text. Another way is to duplicate the exact messages but that would get out of hand with so many on the list now. Or I can change it to something like METHOD. What do you think?


**Cause:** The `.on()` and `.trigger()` methods can set an event handler or generate an event for any event type, and should be used instead of the shortcut methods. This message also applies to the other event shorthands, including: blur, focus, focusin, focusout, resize, scroll, dblclick, mousedown, mouseup, mousemove, mouseover, mouseout, mouseenter, mouseleave, change, select, submit, keydown, keypress, keyup, and contextmenu.

**Solution:** Instead of `.click(fn)` use `.on("click", fn)`. Instead of `.click()` use `.trigger("click")`.

### JQMIGRATE: jQuery.fn.hover() is deprecated

**Cause:** The `.hover()` method is a shorthand for the use of the `mouseover`/`mouseout` events. It is often a poor user interface choice because it does not allow for any small amounts of delay between when the mouse enters or exits an area and when the event fires. This can make it quite difficult to use with UI widgets such as drop-down menus. For more information on the problems of hovering, see the [hoverIntent plugin](http://cherne.net/brian/resources/jquery.hoverIntent.html).

**Solution:** Review uses of `.hover()` to determine if they are appropriate, and consider use of plugins such as `hoverIntent` as an alternative. The direct replacement for `.hover(fn1, fn2)`, is `.on("mouseenter", fn1).on("mouseleave", fn2)`.