WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 6 years ago

Last modified 6 years ago

#18590 closed enhancement (fixed)

Swap out "return false" JS calls for preventDefault

Reported by: Viper007Bond Owned by: adamsilverstein
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.3
Component: Administration Keywords: needs-testing has-patch commit
Focuses: javascript Cc:

Description

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.

Attachments (5)

18590.audit.txt (42.5 KB) - added by robmiller 9 years ago.
Current uses of return false in JS
preventdefault-18590.diff (17.6 KB) - added by robmiller 9 years ago.
Replace obvious candidates for preventDefault
18590.diff (9.3 KB) - added by adamsilverstein 6 years ago.
18590.2.diff (8.5 KB) - added by adamsilverstein 6 years ago.
18590.3.diff (1.7 KB) - added by afercia 6 years ago.

Download all attachments as: .zip

Change History (32)

#1 @demetris
10 years ago

  • Cc dkikizas@… added

#2 follow-up: @azaozz
10 years ago

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.

#3 in reply to: ↑ 2 ; follow-up: @Viper007Bond
10 years ago

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.

#4 @Viper007Bond
10 years ago

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 );
	});
});

#6 @sanchothefat
10 years ago

@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.

#7 @solarissmoke
10 years ago

  • Cc solaris.smoke@… added

#8 in reply to: ↑ 3 @nacin
10 years ago

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
9 years ago

Current uses of return false in JS

#9 @robmiller
9 years ago

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
9 years ago

Replace obvious candidates for preventDefault

#10 @robmiller
9 years ago

  • Cc r@… added
  • Keywords needs-testing added

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.

#11 @robmiller
9 years ago

  • Keywords has-patch added; needs-patch removed

#12 @ocean90
8 years ago

  • Focuses javascript added

#13 @chriscct7
6 years ago

  • Keywords needs-refresh added

#14 follow-up: @adamsilverstein
6 years ago

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?

#15 @adamsilverstein
6 years ago

  • Keywords dev-feedback added; needs-refresh removed

#16 @adamsilverstein
6 years ago

  • Owner set to adamsilverstein
  • Status changed from new to reviewing

#17 in reply to: ↑ 14 @azaozz
6 years ago

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.

#18 @adamsilverstein
6 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to 4.4

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

#19 @azaozz
6 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 34977:

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

Props adamsilverstein.
Fixes #18590.

#20 follow-up: @afercia
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 :)

Last edited 6 years ago by afercia (previous) (diff)

#21 follow-up: @afercia
6 years ago

Small patch to avoid variable names conflicts.

@afercia
6 years ago

This ticket was mentioned in Slack in #core by afercia. View the logs.


6 years ago

#23 @wonderboymusic
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 35014:

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

Props afercia.
Fixes #18590.

#24 in reply to: ↑ 20 @azaozz
6 years ago

Replying to afercia:

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

#25 in reply to: ↑ 21 @adamsilverstein
6 years ago

Good catch, thank you!

#26 @iseulde
6 years ago

In 35198:

Fix close button plugin modal after [18590]

See #18590.

#27 @iseulde
6 years ago

Sorry, should have been [34977].

Note: See TracTickets for help on using tickets.