Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
See: http://blog.jquery.com/2011/11/03/jquery-1-7-released/ for more information.
After reading that issue I think it's save to just rename .bind to .on and .unbind to .off
Comment | File | Size | Author |
---|---|---|---|
#67 | core-js-jquery-on-1342198-67.patch | 34.67 KB | nod_ |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedWow, That's awesome aspilicious!
I'll test this soon.
Comment #2
cosmicdreams CreditAttribution: cosmicdreams commentedtesting this one. Obviously there is a lot this patche touches and therefore a lot to test. We shouldn't pass off this testing to other patches as our goal in this issue is to ensure that making this change doesn't break stuff.
Comment #3
cosmicdreams CreditAttribution: cosmicdreams commentedI'm noticing that the overlay isn't working. I think this issue is caused by jQuery 1.7's exclusion of the .isNaN() function and will eventually be fixed by #1342336: Remove isNaN() from our javascripts
Comment #4
cosmicdreams CreditAttribution: cosmicdreams commentedUploading a file doesn't work. This issue might be resolved by #1203532: jQuery Form Plugin update to latest stable release
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commentedBut on the plus side:
Menu actions
Block actions
Dashboard actions
All work.
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commentedComment #7
cosmicdreams CreditAttribution: cosmicdreams commentedSpoke too soon. This patch seems to be responsible for breaking Overlay
Comment #8
RobLoachComment #9
nod_This needs a big reroll.
Comment #10
klonos;)
Comment #11
RobLoachComment #12
cosmicdreams CreditAttribution: cosmicdreams commentedI can't see anything wrong with the patch but it needs to be manually tested to ensure that it doesn't break any of the javascript heavy features.
Comment #13
nod_Ok previous patch worked (did not try the overlay)
And I put in more changes, no more .click, .change and so on, everything go through .on() check out the jquery doc http://api.jquery.com/on/ to see what's up with the strange parameters in a few .on calls.
Comment #14
nod_need reroll
Comment #15
nod_adding tag, it's an easy enough patch. 3 files needs attention; vertical-tabs, tabledrag and states. The rest apply just fine.
Comment #16
pascalduez CreditAttribution: pascalduez commentedOkay, trying a reroll.
Comment #17
pascalduez CreditAttribution: pascalduez commentedWe kind of agreed that this one should happens more after the JShint stuff, so we don't have to reroll it several times. #14 does not apply at all anymore, or producing conflicts with --3way.
Comment #18
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch attempts to reroll #13 and mostly succeeds. However, I had to pretty much revert everything the patch tried to modify with tabledrag.js as everything it tried to change became broken and disjointed. If I had more time, I might be able to figure out the intent of the author and do a better job of fixing that part of the patch.
Please review this patch closely as my choice may not properly reflect the intent of the author.
Comment #19
cosmicdreams CreditAttribution: cosmicdreams commentedoops, I mean this one.
Comment #20
sunLooks good to me otherwise.
Did not test the patch manually though and especially some more fancy stuff like Ajax, file uploads, overlay, and stuff should definitely be tested.
Comment #21
nod_Great work :)
Still some .bind, .unbind, .click and keyup (probably other keydown and all) left in tabledrag, vertical-tab, color and overlay :) There are a few patches in the queue (the JSHint issues, that include the .on change as well; just FYI).
Comment #22
cosmicdreams CreditAttribution: cosmicdreams commentedYea, I read about the ongoing work with JSHint after I started working on this. Oh well, it was a bit of work to get this far so hopefully this is for some benefit.
I knew about the gaps in the tabledrag. I had difficulty getting my IDE to merge the incoming changes from the previous patch as what was produced seemed all wrong. My previous guidance was that someone with more knowledge of the intent of that patch should try to rewrite the fixes to tabledrag. But I can take a shot at that if that's what's holding this patch up.
I'll also look at the vertical-tab, color, and overlay to see what I missed.
Comment #23
nod_Ok complete rewrite.
I only changed the .on and .off things, I did not try to refactor things so hopefully we can just get this in and do the refactor for each separately. I used the list of event from https://github.com/jquery/jquery/blob/master/src/event-alias.js and replaced everything by .on() .off() and explicitly called .trigger()
Comment #25
nod_#23: core-js-jquery-on-1342198-23.patch queued for re-testing.
Comment #26
Jelle_SMight be safe to run the Testswarm and FAT tests on this one just to be sure ;-)
Comment #27
Jelle_SCorrect me if I'm wrong, but shouldn't this be an "on" in stead of a "trigger". I'm not entirely familiar with wat exactly jQuery.proxy does, but it seems like it's something that should be bound to the event in stead of triggering it?
EDIT:
I've just read through the jQuery docs for proxy. It seems even more to me that is should be an "on" in stead of a "trigger"
Comment #28
nod_D'oh! your right.
Fixed.
Comment #29
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like a clean conversion. have you grep'ed through the code to ensure you're not missing any?
Comment #30
nod_yep, here is what I used to track down all occurrences:
\.(blur|focus|focusin|focusout|load|resize|scroll|unload|click|dblclick|mousedown|mouseup|mousemove|mouseover|mouseout|mouseenter|mouseleave|change|select|submit|keydown|keypress|keyup|error|contextmenu)\(
From the event-alias file linked to in #23
Comment #31
Wim LeersI couldn't spot any errors.
However, I did see that the use of jQuery Namespaced Events was missing in many locations (but only where it was already missing before this conversion). Don't we want to fix that in one go in this conversion?
Comment #32
nod_nope. i don't want to be in reroll hell :p and event names need discussion
Comment #33
droplet CreditAttribution: droplet commentedMost changes are 1:1 matching game
https://github.com/jquery/jquery/blob/master/src/event-alias.js
few focus() in vertical-tabs.js are point to Drupal.verticalTab.prototype.focus(). It's fine.
#28 changes RTBC to me.
I caught 2 more changes, and the DOCs fixes.
Comment #34
nod_The two changes you made won't work. What get() returns is a DOM object and it doesn't have the .trigger method.
It should be left alone or .get() should be changed to .eq().
Comment #35
droplet CreditAttribution: droplet commentedsurprised that it has 100 lines of code for keydown function for the keyboard users.
Patch:
- fixed 2 errors in #33
- fixed 1 error in #28
Cheers.
Comment #36
nod_All good for me, thanks.
We need someone to RTBC now :)
Comment #37
Wim LeersRTBC-worthy, besides a bunch of (mostly whitespace) nitpicks. Once they're fixed (and please provide an interdiff this time), I'll RTBC this :)
Space after "function".
Same.
Same.
Same.
Same.
Lots of chaining -> indent?
Why not have all these
.off()
calls be on a separate line?Analogous.
Should be
.on('focus keyup', selectDefault)
Space after "function".
Same.
Same.
Same.
Same.
Same.
Same.
Same.
Same.
Same.
Same.
Comment #38
droplet CreditAttribution: droplet commentedNovice: Reroll and fix code standard problem, see #37.
Comment #39
droplet CreditAttribution: droplet commented#35: core-js-jquery-on-1342198-35.patch queued for re-testing.
Comment #41
droplet CreditAttribution: droplet commented- code standard fixes
- some JS move to new dir (reapply)
Comment #42
klonos...this helps ;)
Comment #43
Wim LeersAll good, but not all remarks in #37 were fixed yet.
Comment #44
droplet CreditAttribution: droplet commentedCheers.
Comment #45
nod_Sweet, thanks :)
Looks RTBC to me.
Comment #46
nod_Umm in ContextualLinkView.js event.target.trigger('click'); doesn't seem right. It's not a jQuery object, just a DOM one. Same in contextual.js and contextual.toolbar.js Removed them.
Otherwise looks good.
Comment #47
Wim Leers#46: indeed — thanks for that fix.
Patch was reviewed very carefully already, manual testing did not uncover any problems. RTBC.
Comment #48
nod_#46: core-js-jquery-on-1342198-46.patch queued for re-testing.
Comment #50
nod_reroll. The tabledrag touch issue broke this one.
Comment #51
nod_reroll after a few JSHint issues got in.
Comment #52
yannickooDouble quotes?
Comment #53
nod_Scope creep. If we start here it'll never end. Also views js needs much more work anyway.
Comment #54
yannickooSo that will be an extra issue, got it.
Comment #55
nod_#51: core-js-jquery-on-1342198-51.patch queued for re-testing.
Comment #56
alexpottGrepping the code for the methods we're replacing I've found... some of these might well have to remain but others look eminently replaceable.
From tabledrag.js
From edit.js
From views-admin.js
From ContextualLinkView.js, contextual.js, contextual.toolbar.js
From color.js
From contextual.js
Comment #57
nod_Get ready for more JS than you probably want to know about :p
Didn't reroll for the tabledrag touch event properly apparently :)
the
_.bind()
is different, it's an underscore method that is the same as nativeFunction.prototype.bind()
(since we don't use IE8, we could just use the nativeDrupal.edit.app.acceptEditorStateChange.bind(Drupal.edit.app)
btw) function, basically it sets what the value of this is within the function when it's called. Usually used in event handlers.I mentioned it to Wim that the naming inside edit module could be confusing, the
this.bind/unbind/focus()
left are not jQuery calls it's methods from a backbone model, the names are arbitrary, it's all ours. Same for contextual.The
mapping.mouseenter
needs to stay this way because it's how backbone want's it's events to be declared.For
event.target.click()
I didn't change them because there is no jQuery nearby, don't really need to add jQuery to it, not a problem to add if you want.color.js
, yes we should probably replace this one.Comment #58
droplet CreditAttribution: droplet commented#51: core-js-jquery-on-1342198-51.patch queued for re-testing.
Comment #60
nod_rerolled.
in color.js
inputs[0].focus();
that needs to be kept as-is, inputs is an array of DOM objects, so calling focus() directly is ok, it's calling the DOM method directly.fixed two other mistakes. Views and tabledrag still working fine, that's RTBC to me now.
Comment #61
nod_reroll
Comment #62
nod_61: core-js-jquery-on-1342198-61.patch queued for re-testing.
Comment #65
nod_applies fine for me but whatever.
Comment #66
nod_Comment #67
nod_Blocking #2137235: Make core JS work with a subset of jQuery.
Comment #68
nod_Reroll because overlay was removed from core \o/
Comment #69
cosmicdreams CreditAttribution: cosmicdreams commented+1 RTBC
Comment #70
catchCommitted/pushed to 8.x, thanks!