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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pascalduez’s picture

Assigned: Unassigned » pascalduez
pascalduez’s picture

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

Assigned: pascalduez » Unassigned
Status: Active » Needs review
FileSize
7.53 KB

Re-rolled against latest HEAD...

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Works great :)

webchick’s picture

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

Status: Fixed » Needs review
FileSize
525 bytes

This broke contextual links.

nod_’s picture

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

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

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

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