Swap out "return false" JS calls for preventDefault

[Viper007Bond]

While trying to hook into someone clicking the expand/collapse arrow in the nav menu UI, I realized that the existing Javascript is using return false; to abort the hyperlink click from going through.

​This is bad and incorrect.

We should be using object.preventDefault() instead so that other bound functions aren't aborted.

[azaozz]

The linked article explains this well (although in quite a few words): "Only use return false when you want both preventDefault() and stopPropagation()".

We are (should be) using return false to do both which is usually the case. Note that this doesn't stop the 'immediate propagation', i.e. if there are several 'click' handlers on the same element they all fire.

[Viper007Bond]

Replying to azaozz:

We are (should be) using return false to do both which is usually the case. Note that this doesn't stop the 'immediate propagation', i.e. if there are several 'click' handlers on the same element they all fire.

Yes, if my code does $('#foo').click( ... ) it will work, but the return false blocks $('#foo').live('click', ... ) from working (i.e. attaching the click event to subsequently added items).

I ended up having to use $('#foo').live('mouseover', ...) to hack around it.

[Viper007Bond]

Here's my code incase things are unclear:

jQuery(document).ready(function($) {
	// ID tracker field
	var IDtracker = $('<input>').attr({
		type: 'hidden',
		id: ADASIParams.idtracker,
		name: ADASIParams.idtracker,
		value: ''
	}).prependTo('#update-nav-menu');

	function adasi_add_checkbox( item ) {
		// Skip items that are known not to be hierarchical post types or that we already modified
		if ( $(item).is('.adasi-checked, .menu-item-custom, .menu-item-category, .menu-item-post_tag') ) {
			return;
		}

		// Don't check this item again
		$(item).addClass('adasi-checked');

		var itemID = parseInt( $(item).attr('id').replace('menu-item-', ''), 10 );

		// Gotta figure it out in PHP, so use an AJAX call
		jQuery.post(
			ajaxurl,
			{
				action: ADASIParams.ajaxaction,
				id: itemID
			},
			function( response ){
				if ( response && response.add ) {
					// Track IDs to check the POST for
					IDtracker.val( IDtracker.val() + ',' + itemID );

					// Add the checkbox
					var checkboxid = ADASIParams.checkboxprefix + itemID;
					$(item).find('.menu-item-actions .link-to-original').after('<p><label><input type="checkbox" id="' + checkboxid + '" name="' + checkboxid + '" value="1" /> ' + ADASIParams.checkboxdesc + '</label></p>');

					if ( response.checked ) {
						$('#' + checkboxid).prop('checked', true);
					}
				}
			},
			'json'
		);
	}

	// Try adding checkboxes to all existing menu items
	$('.menu-item').each(function(){
		adasi_add_checkbox( this );
	});

	// When hovering over a menu item added using Javascript, try adding a checkbox to it.
	// Props DD32 for mouseover hack to get around return false; inside other click bound event.
	$('.menu-item.pending').live('mouseover', function(){
		adasi_add_checkbox( this );
	});
});

[Viper007Bond]

Full plugin: ​http://wordpress.org/extend/plugins/add-descendants-as-submenu-items/

[sanchothefat]

@Viper have you tried using jQuery's namespaced event bindings?

My approach for hooking onto elements with a click handler attached is to use the syntax described here ​http://docs.jquery.com/Namespaced_Events

For example you would use:

$( '#foo' ).live( 'click.myplugin', function() { ... } );

Essentially this approach sandboxes your plugin code from the default code. There is a big advantage to this in that it means that people can unbind and rebind specific handlers - useful for plugin/theme authors who need a little extra control or a different behaviour. Think of it as the action/filter API but for jquery events. In some cases I unbind the standard wordpress click handler and add some of my own code before it eg. for pre-processing some data before widgets are saved.

[nacin]

Replying to Viper007Bond:

I ended up having to use $('#foo').live('mouseover', ...) to hack around it.

For that matter, we also should kill all uses of .live().

I would not mind an audit of current return false statements in our JS.

[robmiller]

Current uses of return false in JS

[robmiller]

Attached is an audit of current uses of return false; it naturally doesn't attempt to make a distinction between logical uses and ones where it's being used to suppress default event handlers. I'm going to nip through and grab the low-hanging fruit (i.e. the most obvious uses where preventDefault is the correct option).

[robmiller]

Replace obvious candidates for preventDefault

[robmiller]

Aaand here's a patch of the low-hanging fruit. I've not changed anything where there was even ambiguity about whether the return false was necessary, but testing would still be good.

[adamsilverstein]

18590.diff​:

Refresh against trunk. Some instances already fixed; also may have missed new instances.

Would like feedback from @azaozz - where should we be returning false?

[azaozz]

Replying to adamsilverstein:

18590.diff looks good. The only place where it may be better to keep return false is in wp-includes/js/comment-reply.js as we already use "DOM level 0" stuff like cancel.onclick = function()..., etc.

[adamsilverstein]

In 18590.2.diff​:
remove changes for wp-includes/js/comment-reply.js

[azaozz]

In 34977:

JS: in event callbacks replace the very outdated return false with preventDefault().

Props adamsilverstein.
Fixes #18590.

[afercia]

Please check the edit Permalink "Cancel" link, e.html(revert_e) is broken now since e is now the event and no more e = $('#editable-post-name').
Edit: also the saving is broken, same reason :)

[afercia]

Small patch to avoid variable names conflicts.

[wonderboymusic]

In 35014:

Admin JS: after [34977], avoid variable names conflicts with e.

Props afercia.
Fixes #18590.

[azaozz]

Replying to afercia:

Yeah, should have used event instead of e. One more reason to always use good, descriptive, readable names for everything :)

[adamsilverstein]

Good catch, thank you!

[iseulde]

In 35198:

Fix close button plugin modal after [18590]

See #18590.

[iseulde]

Sorry, should have been [34977].