Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

Wow, That's awesome aspilicious!

I'll test this soon.

cosmicdreams’s picture

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.

cosmicdreams’s picture

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

cosmicdreams’s picture

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

cosmicdreams’s picture

But on the plus side:

Menu actions
Block actions
Dashboard actions

All work.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community
cosmicdreams’s picture

Status: Reviewed & tested by the community » Needs work

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

RobLoach’s picture

nod_’s picture

Issue tags: +JavaScript

This needs a big reroll.

klonos’s picture

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

;)

RobLoach’s picture

Title: Use .on and .off in stead of .bind, .unbind and .delegate » Use .on and .off instead of .bind, .unbind and .delegate
Status: Needs work » Needs review
FileSize
20.64 KB
cosmicdreams’s picture

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.

nod_’s picture

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.

nod_’s picture

Status: Needs review » Needs work

need reroll

nod_’s picture

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.

pascalduez’s picture

Assigned: Unassigned » pascalduez

Okay, trying a reroll.

pascalduez’s picture

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.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
35.94 KB

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.

cosmicdreams’s picture

FileSize
35.41 KB

oops, I mean this one.

sun’s picture

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.

nod_’s picture

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

cosmicdreams’s picture

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.

nod_’s picture

Status: Needs work » Needs review
FileSize
51.19 KB

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.

nod_’s picture

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.

Jelle_S’s picture

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

Jelle_S’s picture

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

nod_’s picture

D'oh! your right.

Fixed.

cosmicdreams’s picture

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

nod_’s picture

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

Wim Leers’s picture

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?

nod_’s picture

Status: Needs work » Needs review

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

droplet’s picture

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.

nod_’s picture

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

droplet’s picture

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.

nod_’s picture

All good for me, thanks.

We need someone to RTBC now :)

Wim Leers’s picture

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.

droplet’s picture

Issue tags: +Novice

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

droplet’s picture

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.

droplet’s picture

Assigned: Unassigned » droplet
Status: Needs work » Needs review
FileSize
8.85 KB
49.04 KB

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

klonos’s picture

Assigned: droplet » Unassigned

...this helps ;)

Wim Leers’s picture

Status: Needs review » Needs work

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

droplet’s picture

Cheers.

nod_’s picture

Sweet, thanks :)

Looks RTBC to me.

nod_’s picture

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.

Wim Leers’s picture

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.

nod_’s picture

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.

nod_’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
53.66 KB

reroll. The tabledrag touch issue broke this one.

nod_’s picture

reroll after a few JSHint issues got in.

yannickoo’s picture

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?

nod_’s picture

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.

yannickoo’s picture

So that will be an extra issue, got it.

nod_’s picture

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

alexpott’s picture

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

nod_’s picture

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.

droplet’s picture

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.

nod_’s picture

Status: Needs work » Needs review
FileSize
52.81 KB

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.

nod_’s picture

reroll

nod_’s picture

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.

nod_’s picture

applies fine for me but whatever.

nod_’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Reviewed & tested by the community

Reroll because overlay was removed from core \o/

cosmicdreams’s picture

+1 RTBC

catch’s picture

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.