Comments

Wow, That's awesome aspilicious!

I'll test this soon.

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

I'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

Uploading a file doesn't work. This issue might be resolved by #1203532: jQuery Form Plugin update to latest stable release

But on the plus side:

Menu actions
Block actions
Dashboard actions

All work.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Spoke too soon. This patch seems to be responsible for breaking Overlay

Issue tags:+JavaScript

This needs a big reroll.

Title:Use .on and .off in stead off .bind, .unbind and .delegateUse .on and .off in stead of .bind, .unbind and .delegate

;)

Title:Use .on and .off in stead of .bind, .unbind and .delegateUse .on and .off instead of .bind, .unbind and .delegate
Status:Needs work» Needs review
StatusFileSize
new20.64 KB
PASSED: [[SimpleTest]]: [MySQL] 37,009 pass(es).
[ View ]

Issue tags:+Needs manual testing

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

StatusFileSize
new54.25 KB
PASSED: [[SimpleTest]]: [MySQL] 37,032 pass(es).
[ View ]

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.

Status:Needs review» Needs work

need reroll

Issue tags:+js-novice

adding tag, it's an easy enough patch. 3 files needs attention; vertical-tabs, tabledrag and states. The rest apply just fine.

Assigned:Unassigned» pascalduez

Okay, trying a reroll.

Assigned:pascalduez» Unassigned

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

Status:Needs work» Needs review
StatusFileSize
new35.94 KB
PASSED: [[SimpleTest]]: [MySQL] 40,746 pass(es).
[ View ]

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

StatusFileSize
new35.41 KB
PASSED: [[SimpleTest]]: [MySQL] 40,752 pass(es).
[ View ]

oops, I mean this one.

Status:Needs review» Needs work

+++ b/core/modules/openid/openid.js
@@ -25,7 +25,7 @@ Drupal.behaviors.openid = {
-      .click(function (e) {
+      .on('click', function () {      ¶
@@ -37,7 +37,7 @@ Drupal.behaviors.openid = {
-      .click(function (e) {
+      .on('click', function () {      ¶

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

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

Yea, 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.

Status:Needs work» Needs review
StatusFileSize
new51.19 KB
PASSED: [[SimpleTest]]: [MySQL] 54,932 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-JavaScript, -Needs manual testing, -js-novice

The last submitted patch, core-js-jquery-on-1342198-23.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+JavaScript, +Needs manual testing, +js-novice

#23: core-js-jquery-on-1342198-23.patch queued for re-testing.

Might be safe to run the Testswarm and FAT tests on this one just to be sure ;-)

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -667,7 +667,7 @@ Drupal.viewsUi.rearrangeFilterHandler.prototype.syncGroupsOperators = function (
-  this.dropdowns.change(jQuery.proxy(this, 'operatorChangeHandler'));
+  this.dropdowns.trigger('change', jQuery.proxy(this, 'operatorChangeHandler'));

Correct 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"

StatusFileSize
new51.19 KB
PASSED: [[SimpleTest]]: [MySQL] 54,819 pass(es).
[ View ]

D'oh! your right.

Fixed.

Looks like a clean conversion. have you grep'ed through the code to ensure you're not missing any?

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

Status:Needs review» Needs work

I 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?

Status:Needs work» Needs review

nope. i don't want to be in reroll hell :p and event names need discussion

StatusFileSize
new2.5 KB
new52.42 KB
PASSED: [[SimpleTest]]: [MySQL] 55,447 pass(es).
[ View ]

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

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().

StatusFileSize
new2.87 KB
new52.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-jquery-on-1342198-35_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

All good for me, thanks.

We need someone to RTBC now :)

Status:Needs review» Needs work

RTBC-worthy, besides a bunch of (mostly whitespace) nitpicks. Once they're fixed (and please provide an interdiff this time), I'll RTBC this :)

+++ b/core/misc/states.jsundefined
@@ -500,7 +500,7 @@ states.State.prototype = {
+$(document).on('state:disabled', function(e) {

Space after "function".

+++ b/core/misc/states.jsundefined
@@ -514,7 +514,7 @@ $(document).bind('state:disabled', function(e) {
+$(document).on('state:required', function(e) {

Same.

+++ b/core/misc/states.jsundefined
@@ -525,22 +525,22 @@ $(document).bind('state:required', function(e) {
+$(document).on('state:visible', function(e) {

Same.

+++ b/core/misc/states.jsundefined
@@ -525,22 +525,22 @@ $(document).bind('state:required', function(e) {
+$(document).on('state:checked', function(e) {

Same.

+++ b/core/misc/states.jsundefined
@@ -525,22 +525,22 @@ $(document).bind('state:required', function(e) {
+$(document).on('state:collapsed', function(e) {

Same.

+++ b/core/misc/tableselect.jsundefined
@@ -28,7 +28,7 @@ Drupal.tableSelect = function () {
+  $table.find('th.select-all').prepend($('<input type="checkbox" class="form-checkbox" />').attr('title', strings.selectAll)).on('click', function (event) {

Lots of chaining -> indent?

+++ b/core/modules/color/color.jsundefined
@@ -184,8 +184,8 @@ Drupal.behaviors.color = {
+        $(focused).off('keyup', farb.updateValue)
+          .off('keyup', preview).off('keyup', resetScheme)

Why not have all these .off() calls be on a separate line?

+++ b/core/modules/color/color.jsundefined
@@ -193,7 +193,7 @@ Drupal.behaviors.color = {
-      $(focused).keyup(farb.updateValue).keyup(preview).keyup(resetScheme)
+      $(focused).on('keyup', farb.updateValue).on('keyup', preview).on('keyup', resetScheme)

Analogous.

+++ b/core/modules/shortcut/shortcut.admin.jsundefined
@@ -11,7 +11,7 @@ Drupal.behaviors.newSet = {
+    $('div.form-item-new input').on('focus', selectDefault).on('keyup', selectDefault);

Should be .on('focus keyup', selectDefault)

+++ b/core/modules/translation/translation.jsundefined
@@ -4,7 +4,7 @@
+    $('#edit-node-type-language-default, #edit-node-type-language-hidden', context).on('change', function(context) {

Space after "function".

+++ b/core/modules/translation_entity/translation_entity.admin.jsundefined
@@ -23,7 +23,7 @@ Drupal.behaviors.translationEntityDependentOptions = {
+      $fields.on('change', function() {

Same.

+++ b/core/modules/views/views_ui/js/ajax.jsundefined
@@ -16,13 +16,13 @@
+      $submit_buttons.on('click', function(event) {

Same.

+++ b/core/modules/views/views_ui/js/ajax.jsundefined
@@ -16,13 +16,13 @@
+      $submit_buttons.on('mousedown', function(event) {

Same.

+++ b/core/modules/views/views_ui/js/ajax.jsundefined
@@ -87,9 +87,9 @@
+      $('input#edit-displays-live-preview', context).once('views-ajax-processed').on('click', function() {

Same.

+++ b/core/modules/views/views_ui/js/ajax.jsundefined
@@ -100,7 +100,7 @@
+      $("#views-tabset a").once('views-ajax-processed').on('click', function() {

Same.

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -346,14 +346,14 @@ Drupal.viewsUi.OptionsSearch = function ($form) {
+  this.$searchBox.on('keypress', function(event) {

Same.

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -858,7 +858,7 @@ Drupal.behaviors.viewsFilterConfigSelectAll.attach = function(context) {
+  .on('click', function() {

Same.

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -867,7 +867,7 @@ Drupal.behaviors.viewsFilterConfigSelectAll.attach = function(context) {
+    $(this).on('click', function() {

Same.

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -928,7 +928,7 @@ Drupal.behaviors.viewsUiChangeDefaultWidget.attach = function (context, settings
+  $('input[name="options[group_info][multiple]"]').on('change', function() {

Same.

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -983,12 +983,12 @@ Drupal.behaviors.viewsUiOverrideSelect.attach = function (context, settings) {
+    $(this).on('change', function() {

Same.

Issue tags:+Novice

Novice: Reroll and fix code standard problem, see #37.

Status:Needs work» Needs review
Issue tags:-JavaScript, -Novice, -Needs manual testing, -js-novice

#35: core-js-jquery-on-1342198-35.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+JavaScript, +Needs manual testing, +js-novice

The last submitted patch, core-js-jquery-on-1342198-35.patch, failed testing.

Assigned:Unassigned» droplet
Status:Needs work» Needs review
StatusFileSize
new8.85 KB
PASSED: [[SimpleTest]]: [MySQL] 55,672 pass(es).
[ View ]
new49.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,619 pass(es).
[ View ]

- code standard fixes
- some JS move to new dir (reapply)

Assigned:droplet» Unassigned

...this helps ;)

Status:Needs review» Needs work

All good, but not all remarks in #37 were fixed yet.

Status:Needs work» Needs review
StatusFileSize
new11.58 KB
new16.7 KB
new56.3 KB
PASSED: [[SimpleTest]]: [MySQL] 55,704 pass(es).
[ View ]

Cheers.

Sweet, thanks :)

Looks RTBC to me.

StatusFileSize
new54.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-jquery-on-1342198-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing, -js-novice

#46: indeed — thanks for that fix.

Patch was reviewed very carefully already, manual testing did not uncover any problems. RTBC.

Issue tags:-JavaScript

#46: core-js-jquery-on-1342198-46.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+JavaScript

The last submitted patch, core-js-jquery-on-1342198-46.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new53.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,958 pass(es).
[ View ]

reroll. The tabledrag touch issue broke this one.

StatusFileSize
new53.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-jquery-on-1342198-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

reroll after a few JSHint issues got in.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/views_ui/js/ajax.jsundefined
@@ -100,7 +100,7 @@
+      $("#views-tabset a").once('views-ajax-processed').on('click', function () {

Double quotes?

Status:Needs work» Reviewed & tested by the community

Scope creep. If we start here it'll never end. Also views js needs much more work anyway.

So that will be an extra issue, got it.

#51: core-js-jquery-on-1342198-51.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs review

Grepping the code for the methods we're replacing I've found... some of these might well have to remain but others look eminently replaceable.

  // Add event bindings to the document. The self variable is passed along
  // as event handlers do not have direct access to the tableDrag object.
  if (Modernizr.touch) {
    $(document).bind('touchmove', function (event) { return self.dragRow(event.originalEvent.touches[0], self); });
    $(document).bind('touchend', function (event) { return self.dropRow(event.originalEvent.touches[0], self); });
  }
  else {
    $(document).bind('mousemove', function (event) { return self.dragRow(event, self); });
    $(document).bind('mouseup', function (event) { return self.dropRow(event, self); });
  }

From tabledrag.js

    acceptStateChange: _.bind(Drupal.edit.app.acceptEditorStateChange, Drupal.edit.app)

From edit.js

  this.bind();
  ...
  this.unbind();
  ...
  this.bind()

From views-admin.js

      event.target.click();

From ContextualLinkView.js, contextual.js, contextual.toolbar.js

    // Focus first color.
    inputs[0].focus();

From color.js

  this.focus();
  ...
  mapping.mouseenter =  function () { this.model.focus(); };
  ...
  this.model.focus();

From contextual.js

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 native Function.prototype.bind() (since we don't use IE8, we could just use the native Drupal.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.

Issue tags:-JavaScript

#51: core-js-jquery-on-1342198-51.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+JavaScript

The last submitted patch, core-js-jquery-on-1342198-51.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new52.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,829 pass(es).
[ View ]

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.

StatusFileSize
new42.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-jquery-on-1342198-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

reroll

Status:Needs review» Needs work

The last submitted patch, 61: core-js-jquery-on-1342198-61.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 61: core-js-jquery-on-1342198-61.patch, failed testing.

Issue summary:View changes
StatusFileSize
new42.07 KB
PASSED: [[SimpleTest]]: [MySQL] 60,048 pass(es).
[ View ]

applies fine for me but whatever.

Status:Needs work» Needs review

Issue tags:+JavaScript clean-up
Related issues:+#2137235: Make core JS work with a subset of jQuery
StatusFileSize
new34.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,051 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Reroll because overlay was removed from core \o/

+1 RTBC

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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