Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
- event.preventDefault();
- event.stopPropagation();
- 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.
Comment | File | Size | Author |
---|---|---|---|
#6 | contextual-1749782-6.patch | 525 bytes | tim.plunkett |
#3 | core_js-event_preventDefault_vs_return_false-1749782-3.patch | 7.53 KB | pascalduez |
#2 | core_js-event_preventDefault_vs_return_false-1749782-2.patch | 8.08 KB | pascalduez |
Comments
Comment #1
pascalduez CreditAttribution: pascalduez commentedComment #2
pascalduez CreditAttribution: pascalduez commentedDeliberately leaved tabledrag.js untouched since :
Comment #3
pascalduez CreditAttribution: pascalduez commentedRe-rolled against latest HEAD...
Comment #4
nod_Works great :)
Comment #5
webchickThis looks pretty straight-forward to me!
Committed and pushed to 8.x. Thanks!
Comment #6
tim.plunkettThis broke contextual links.
Comment #7
nod_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.
Comment #8
webchick:(
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.
Comment #9
nod_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.
Comment #11
markhalliwellUpdated the jQuery coding standards to reference this issue: https://drupal.org/node/1720586#event-delegation