return false; is overused in several places. And is really acting the hard way whereas event.preventDefault() would be sufficient most of the time.

return false; is actually doing three very separate things when you call it:

  1. event.preventDefault();
  2. event.stopPropagation();
  3. Stops callback execution and returns immediately when called.

So it's making JS devs work difficult in case they would need to bind other events on top of core ones.

CommentFileSizeAuthor
#6 contextual-1749782-6.patch525 bytestim.plunkett
#3 core_js-event_preventDefault_vs_return_false-1749782-3.patch7.53 KBpascalduez
#2 core_js-event_preventDefault_vs_return_false-1749782-2.patch8.08 KBpascalduez

Comments

pascalduez’s picture

pascalduez commented
Assigned: Unassigned » pascalduez
pascalduez’s picture

pascalduez commented
StatusFileSize
new core_js-event_preventDefault_vs_return_false-1749782-2.patch8.08 KB

Deliberately leaved tabledrag.js untouched since :

  1. it *really* needs a re-write... #1524414: Rewrite tabledrag.js to use jQuery UI
  2. I couldn't get the key events to work, thus test.
pascalduez’s picture

pascalduez commented
Assigned: pascalduez » Unassigned
Status: Active » Needs review
StatusFileSize
new core_js-event_preventDefault_vs_return_false-1749782-3.patch7.53 KB

Re-rolled against latest HEAD...

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Works great :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This looks pretty straight-forward to me!

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Fixed » Needs review
StatusFileSize
new contextual-1749782-6.patch525 bytes

This broke contextual links.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

and that's a very good example of how return false; does too much and probably not what you'd think it does.

Fixes the problem.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Category: task » bug
Priority: Normal » Critical
Status: Reviewed & tested by the community » Fixed

:(

Committed and pushed to 8.x.

I think I'm officially done committing anymore JS clean-up patches until we have some basic sanity-check tests in place to make sure various JS is being loaded when it ought to be. This is getting a little ridiculous.

nod_’s picture

nod_
Primary language French
Location Lille
commented

The JS was loaded, it was just missing a line.

So if we look at the last couple of weeks, dependency patch got 2 follow-ups for incomplete dependencies definitions and was breaking views, both follow-ups were 1 liners. The preventDefault patch had 1 follow-up which is this issue.

On the other hand, All of the JS is passing JSHint without errors, tableheader got rewritten, really nasty and stupid bugs were fixed, news JS was added. The JS we have it's like D6 php, no tests and commit-and-hope-reviewers-did-a-good-job. So really, what you need to compare is how bad it was from D6 to D7 without the testbot to the current occasional JS breakage (which have pretty much been one liners).

So is it me getting sloppy with reviews? Maybe, on the other hand those patches needed to get in and were big. My co-maintainer is having some unforseen trouble at the moment and can't really take care of this for a while. So he won't be able to compensate for my blind spots during reviews.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

markhalliwell’s picture

markhalliwell
Primary language English
commented

Updated the jQuery coding standards to reference this issue: https://drupal.org/node/1720586#event-delegation