Title says all. Changing this only saves one function call but it saves this call hundreds of times. there is a comment in the jQuery source that assure both are equivalent. It's on the sexy 169 line.

  • If a $(domelement) is used more than once it goes in a variable with a $domelement name in the closest scope around it,
  • removed useless $() wrapping that were already jQuery objects,
  • I fixed two leaking global vars in this patch too (in tabledrag),
  • I checked everything but I'm sure sun will end up questioning that ;) be my guest for testing.

If you look closely at the patch you might wonder why context was passed as an argument to attachBehaviors instead of $(context), saving a few thousand useless calls. I'm wondering too.

And tabledrag needs love badly.

This patch will confilct with the other on about changing .size() to .length and most likely the one about eval in tableHeader.

Files: 
CommentFileSizeAuthor
#71 1419968-follow-up.patch1.08 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 35,409 pass(es).
[ View ]
#63 drupal-1419968-61.patch660 bytestim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 35,362 pass(es).
[ View ]
#61 drupal-1419968-61.patch660 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1419968-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#53 drupal-1419968-53.patch94.07 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 36,162 pass(es).
[ View ]
#49 tabledrag-49-do-not-test.patch25.59 KBdroplet
#47 core-js-select-with-find-1419968-47.patch93.91 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,913 pass(es).
[ View ]
#45 core-js-select-with-find-1419968-45.patch93.84 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,907 pass(es).
[ View ]
#41 core-js-select-with-find-1419968-41.patch93.78 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,779 pass(es).
[ View ]
#37 core-js-select-with-find-1419968-37.patch94.3 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,807 pass(es).
[ View ]
#35 core-js-select-with-find-1419968-35.patch92.86 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-select-with-find-1419968-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 core-js-select-with-find-1419968-30.patch93.48 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-select-with-find-1419968-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 1419968-28-30-interdiff.diff12.06 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1419968-28-30-interdiff.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 1419968-28-core-js-select-with-find.patch89.06 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 35,644 pass(es).
[ View ]
#27 core-js-select-with-find-1419968-27.patch84.56 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,646 pass(es).
[ View ]
#25 core-js-select-with-find-1419968-25.patch83.25 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 34,817 pass(es).
[ View ]
#21 core-js-select-with-find-1419968-21.patch83.1 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-select-with-find-1419968-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 core-js-select-with-find-1419968-16.patch83.06 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 34,324 pass(es).
[ View ]
#13 core-js-select-with-find-1419968-13.patch84.05 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 34,038 pass(es).
[ View ]
#11 core-js-select-with-find-1419968-11.patch84 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 34,031 pass(es).
[ View ]
#8 core-js-select-with-find-1419968-8.patch83.28 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 33,335 pass(es).
[ View ]
core-js-select-with-find.patch83.02 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 33,341 pass(es).
[ View ]

Comments

droplet’s picture

Status:Needs review» Needs work

For issue purpose, most of this patch looks good to me. (I've not do real test)

+++ b/core/misc/tabledrag.jsundefined
@@ -385,9 +393,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
-        var nextRow = $(self.rowObject.group).filter(':last').next('tr').get(0);
-        while (nextRow && $(nextRow).is(':hidden')) {
-          nextRow = $(nextRow).next('tr').get(0);
+        var $nextRow = $(self.rowObject.group).filter(':last').next('tr');
+        var nextRow = $nextRow.get(0);
+        while (nextRow && $nextRow.is(':hidden')) {
+          $nextRow = $(nextRow).next('tr');
+          nextRow = $nextRow.get(0);

not same ??

while (nextRow && $nextRow.is(':hidden')) {

should be

while (nextRow && nextRow.is(':hidden')) {
+++ b/core/misc/tabledrag.jsundefined
@@ -631,8 +643,9 @@ Drupal.tableDrag.prototype.findDropTargetRow = function (x, y) {
-      while ($(row).is(':hidden') && $(row).prev('tr').is(':hidden')) {
-        row = $(row).prev('tr').get(0);
+      while ($row.is(':hidden') && $row.prev('tr').is(':hidden')) {
+        $row = $row.prev('tr');
+        row = $row.get(0);

why copy again? it returned "return row;" and not use $row anymore.

-28 days to next Drupal core point release.

nod_’s picture

Status:Needs work» Needs review
Issue tags:+JavaScript clean-up

I just wanted to change the selectors, tabledrag is a huge mess and if I start cleaning it up i'll end up refactoring everything. The change you proposed will not work, try it :þ nextRow it the DOM element $nextRow is the jQuery object of the DOM element.

I'm not changing how and when elements are used even if it's stupid, just selectors. There'll be another issue for that.

sun’s picture

Issue tags:+frontend performance

The base/reasoning for this issue seems to be frontend performance.

However, looking at the referenced jQuery core lines, that code almost looks like a no-op to me.

As in PHP, the cost of a mere function call is almost unmeasurable in JS either.

So before we're going to do this (which in itself would imply kind of a new JS coding standard), and break many patches in the queue, I'd like to see that there is actually a measurable performance difference.

nod_’s picture

for perf mesurements, here is a little something : http://jsperf.com/drupal-dom-selector/2 you can play around with it I took the html tree from admin/structure/menu/manage/navigation and the selector that is used for table row styling, I'm much more interested in IE results but don't have any around to test.

I don't care about other patches, I'll reroll this one or others as needed.

nod_’s picture

With a less obnoxious selector that can be optimized with querySelectorAll, results are better: http://jsperf.com/drupal-dom-selector/4

As you can see by following the link there is a little performance difference, this is not entirely a style issue. Not by a crazy amount but still it adds up.

nod_’s picture

after a bit more testing http://jsperf.com/drupal-dom-selector/5 there is a 5–10% overhead for $('selector', domelement); depending on the browser.

droplet’s picture

It's pattern is widely use in jQuery world:
http://addyosmani.com/jqprovenperformance/

$('selector').find() always better than context

It's the worst case:
http://jsperf.com/drupal-dom-selector/6

nod_’s picture

StatusFileSize
new83.28 KB
PASSED: [[SimpleTest]]: [MySQL] 33,335 pass(es).
[ View ]

reroll

sun’s picture

It looks like this can be backported to D7?

This needs manual testing, though not really sure how to approach that.

nod_’s picture

yeah sure I can help backport if it doesn't apply properly. For testing you'd be testing jQuery so I think the review should be on making sure I haven't done anything else than create vars and replaced selectors.

nod_’s picture

StatusFileSize
new84 KB
PASSED: [[SimpleTest]]: [MySQL] 34,031 pass(es).
[ View ]

rereroll, caught a typo and keep up with the latest core changes.

droplet’s picture

@nod_,

both you & me are wrong. I made a wrong suggestion on #1

+++ b/core/misc/tabledrag.jsundefined
@@ -385,9 +393,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
-        var nextRow = $(self.rowObject.group).filter(':last').next('tr').get(0);
-        while (nextRow && $(nextRow).is(':hidden')) {
-          nextRow = $(nextRow).next('tr').get(0);
+        var $nextRow = $(self.rowObject.group).filter(':last').next('tr');
+        var nextRow = $nextRow.get(0);
+        while (nextRow && $nextRow.is(':hidden')) {
+          $nextRow = $(nextRow).next('tr');
+          nextRow = $nextRow.get(0);

If we simplify the changes, it will looks like this:
http://jsfiddle.net/wk4Qe/

Orig: check on first matched element (FALSE)
Patched: check on all matched element (TRUE)

nod_’s picture

StatusFileSize
new84.05 KB
PASSED: [[SimpleTest]]: [MySQL] 34,038 pass(es).
[ View ]

oh, good catch!

Just needed to add a .eq(0): http://jsfiddle.net/Sj9JW/

droplet’s picture

Status:Needs review» Reviewed & tested by the community

Reviewed code, and randomly do tests. It's worked.

catch’s picture

Status:Reviewed & tested by the community» Needs review

http://jsperf.com/drupal-dom-selector/6 looks good to me there's a regression in opera but a decent improvement in every other browser.

However I agree this needs at least some manual testing, and there's no sign of that here.

nod_’s picture

StatusFileSize
new83.06 KB
PASSED: [[SimpleTest]]: [MySQL] 34,324 pass(es).
[ View ]

rererereroll since lots of little fixes have been committed to JS files lately :D

What really needs to be tested are the changes in tabledrag, the rest of them are really straightforward see the comment about the comment in jquery source file. Seriously if the fixes to tabledrag are removed it's all 1 line changes or maybe two when I create a new var.

catch’s picture

Issue tags:+Novice

Tagging as novice for the tabledrag testing.

droplet’s picture

Status:Needs review» Needs work

Looks find to me.

Following can be a separated issue (remark)

+++ b/core/misc/tabledrag.jsundefined
@@ -345,9 +352,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
+        var $previousRow = $(self.rowObject.element).prev('tr').eq(0);
+        var previousRow = $previousRow.get(0);
+        while (previousRow && $previousRow.is(':hidden')) {

Few places have above pattern.

$previousRow.get(0);

equivalent to

$previousRow[0];

http://jsfiddle.net/kyJzh/
http://jsperf.com/get-0-vs-0

12 days to next Drupal core point release.

nod_’s picture

Indeed, I didn't want to change too much stuff in tabledrag so I left it as it was.

I'd say we leave that out from this patch since another issue will be open to deal with selector optimization. But meh, I don't know, that's not what I care about for this patch either way is fine with me.

nod_’s picture

Status:Needs work» Needs review

If you don't mind droplet I'm going for minimum friction, so i'll put it back to NR to keep trying to get reviewers :)

nod_’s picture

StatusFileSize
new83.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-select-with-find-1419968-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, core-js-select-with-find-1419968-21.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review

well it's a js-only patch, doesn't matter is the testbot is happy or not.

Status:Needs review» Needs work

The last submitted patch, core-js-select-with-find-1419968-21.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new83.25 KB
PASSED: [[SimpleTest]]: [MySQL] 34,817 pass(es).
[ View ]

rererererereroll

droplet’s picture

can I make it as RTBC ?

nod_’s picture

StatusFileSize
new84.56 KB
PASSED: [[SimpleTest]]: [MySQL] 35,646 pass(es).
[ View ]

a rerererererereroll was needed. I'd say it's RTBC but since I made the patch… might want to try getting someone to review it, I've had no luck.

cosmicdreams’s picture

StatusFileSize
new89.06 KB
PASSED: [[SimpleTest]]: [MySQL] 35,644 pass(es).
[ View ]

found some more instances of this issue

in ajax.js line 377,
was: $('.throbber', this.progress.element).after('<div class="message">' + this.progress.message + '</div>');
new: this.progress.element.find('.throbber').after('<div class="message">' + this.progress.message + '</div>');

in states.js line 530,
was: $('> legend a', e.target).click();
new: $(e.target).find$('> legend a').click();

in tabledrag.js, line 242,
was: var field = $('.' + group, row);
new: var field = $(row).find('.' + group);

in block.js, line 95,
was: if ($('option[value=' + regionName + ']', regionField).length == 0) {
new: if ($(regionField).find('option[value=' + regionName + ']').length == 0) {

and line 149,
was: $('tr.region-message', table).each(function () {
new: $(table).find('tr.region-message').each(function () {

in comment.js, line 12:
was: return Drupal.checkPlain($('.form-item-comment input:checked', context).next('label').text());
new: return Drupal.checkPlain($(context).find('.form-item-comment input:checked').next('label').text());

in field_ui.js, many changes:
(see patch)

in menu.js, line 5:
was: $('fieldset.menu-link-form', context).drupalSetSummary(function (context) {
new: $(context).find('fieldset.menu-link-form').drupalSetSummary(function (context) {

line 6:
was: if ($('.form-item-menu-enabled input', context).is(':checked')) {
new: if ($(context).find('.form-item-menu-enabled input').is(':checked')) {

line 7:
was: return Drupal.checkPlain($('.form-item-menu-link-title input', context).val());
new: return Drupal.checkPlain($(context).find('.form-item-menu-link-title input').val());

in user.permissions.js, line 33
was: $('input[type=checkbox]', this).not('.rid-2, .rid-1').addClass('real-checkbox').each(function () {
new: $(this).find('input[type=checkbox]').not('.rid-2, .rid-1').addClass('real-checkbox').each(function () {

line 38:
was: $('input[type=checkbox].rid-2', this)
new: $(this).find('input[type=checkbox].rid-2')

droplet’s picture

Issue tags:-Novice

#28 updates reviewed. Looks good.

@cosmicdreams,
do you review other parts ? we hope one more more people review this thread :)

cosmicdreams’s picture

@droplet, I do as much as I can.

I'll manually test this patch today if I have time. I'll go change-by-change this evening.

nod_’s picture

StatusFileSize
new12.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1419968-28-30-interdiff.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
new93.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-select-with-find-1419968-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

wow, thanks for finding those. You're right it's like I forgot field_ui.js I reviewed #28 too and it's all working fine.

I've made a few changes to avoid the redundant use of $() on elements that already are jQuery objects.

There is poor man's interdiff too (diff of two patches…) to see what I changed from #28.

man I hope this is the last, so much patches are starting to conflict… If that goes further I'll be splitting this by module.

Status:Needs review» Needs work

The last submitted patch, 1419968-28-30-interdiff.diff, failed testing.

nod_’s picture

Status:Needs work» Needs review

yeah yeah, sorry bot got the wrong filename.

JohnAlbin’s picture

nod_’s picture

StatusFileSize
new92.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-select-with-find-1419968-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok, reroll. Wasn't too hard but I'd rather wait next month to get this in.

Oh crap I patched againt D7. sorry will upload followup patch.

Status:Needs review» Needs work

The last submitted patch, core-js-select-with-find-1419968-35.patch, failed testing.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new94.3 KB
PASSED: [[SimpleTest]]: [MySQL] 35,807 pass(es).
[ View ]

ok here it is.

Very soon I will be able to give performance measurements of this kind of changes on behaviors attach time.

JohnAlbin’s picture

git apply core-js-select-with-find-1419968-37.patch reports:

core-js-select-with-find-1419968-37.patch:146: trailing whitespace.
(function (source_id, options) {
core-js-select-with-find-1419968-37.patch:1516: trailing whitespace.

warning: 2 lines add whitespace errors.

This patch touches a lot of code (by design). I'm unsure how to test it all in the browser. Is eyeball review of the js the only possible method?

nod_’s picture

yes, you need to make sure that: $('XXXselector', DOMELEMENT) if transformed properly in: $(DOMELEMENT).find('XXXselector').

I've added some variable as well to avoid redundant uses of $(this), i turned it into: $this = $(this);, and maybe a var or 2 added because it was leaking but that should be it.

edit: change is not a browser thing, don't need to test everywhere.

sun’s picture

Status:Needs review» Needs work
+++ b/core/misc/machine-name.js
@@ -28,10 +28,11 @@ Drupal.behaviors.machineName = {
+        (function (source_id, options) {      ¶

trailing whitespace

+++ b/core/misc/tabledrag.js
@@ -704,13 +722,16 @@ Drupal.tableDrag.prototype.updateField = function (changedRow, group) {
+    // TODO clean that up, It wasn't coherent compared to the rest of the function.

Should be:

// @todo Explain what is actually wrong.

The current todo doesn't actually explain what is wrong. Also make sure that the comment wraps at 80 chars. If the todo exceeds 80 chars, indent subsequent lines by two spaces (after the leading //).

+++ b/core/modules/file/file.js
@@ -75,7 +79,7 @@ Drupal.file = Drupal.file || {
-
+    ¶

Trailing white-space.

---
The manual testing that has been performed already is sufficient for D8, so this patch can land whenever it is ready.

The backport for D7 will likely require more manual testing. However, once it is in D8, we'll get some additional manual testing for free.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new93.78 KB
PASSED: [[SimpleTest]]: [MySQL] 35,779 pass(es).
[ View ]

Here it is. The comment is not really relevant, it's a comment on the code of this particular part, it uses the elements differently than the rest of the code, it's not using .get(0) at the same time. it relies on jquery whereas the rest of the code default to using DOM elements.

It's not really important and outside of the scope of this changes. Anyway tabledrag will be rewritten before D8 ships so that'll be taken care of then :)

JohnAlbin’s picture

From the patch:

--- a/core/misc/machine-name.js
+++ b/core/misc/machine-name.js
@@ -29,9 +29,10 @@ Drupal.behaviors.machineName = {
     for (var i in settings.machineName) {
       if (settings.machineName.hasOwnProperty(i)) {
         (function (source_id, options) {
-          var $source = $(source_id, context).addClass('machine-name-source');
-          var $target = $(options.target, context).addClass('machine-name-target');
-          var $suffix = $(options.suffix, context);
+          var $context = $(context);
+          var $source = $context.find(source_id).addClass('machine-name-source');

Which occurs in context here:

Drupal.behaviors.machineName = {
  attach: function (context, settings) {
    var self = this;
    for (var i in settings.machineName) {
      if (settings.machineName.hasOwnProperty(i)) {
        (function (source_id, options) {
          var $context = $(context);
          var $source = $context.find(source_id).addClass('machine-name-source');

Shouldn't you create the var $context = $(context); variable outside the for loop? It looks like we're re-creating the same jQuery object on each loop pass.

There are other places where we could optimize the code by creating a temporary variable to hold a jQuery object. Should I mention all of them? or just leave it for later optimizations since its out of scope?

I'm only about 1/4 of the way through this review (I'm looking at tabledrag at the moment). I've applied the patch and I'm reviewing the changes, line by line, in context of the actual file. I'll post back when I'm all the way though my review. (Dinner time!)

nod_’s picture

You're right, $context should be outside of the loop.

I think we should decide if this patch can go in as-is and fix the rest in followup issues otherwise nobody else will want to review this monster again (ask xjm about the #states patch :). On the other end I don't mind adding fixes to this one since the other big conflicting patches are already committed.

When I rerolled I saw a few things I overlooked before but didn't change them. I think it'll be much more efficient to separate by file/module to catch the details. By all means, take notes on what you see, it'll be useful whatever happens.

I guess it all comes down to a core process which I'm not familiar with what to do with big patches where you miss things but it doesn't break the system.

And by the way, thanks a lot for reviewing it :)

JohnAlbin’s picture

Assigned:Unassigned» nod_
Status:Needs review» Needs work
@@ -349,9 +356,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
         break;
       case 38: // Up arrow.
       case 63232: // Safari up arrow.
-        var previousRow = $(self.rowObject.element).prev('tr').get(0);
-        while (previousRow && $(previousRow).is(':hidden')) {
-          previousRow = $(previousRow).prev('tr').get(0);
+        var $previousRow = $(self.rowObject.element).prev('tr').eq(0);
+        var previousRow = $previousRow.get(0);
+        while (previousRow && $previousRow.is(':hidden')) {
+          $previousRow = $(previousRow).prev('tr').eq(0);
+          previousRow = $previousRow.get(0);

and

@@ -361,9 +370,10 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
           if ($(item).is('.tabledrag-root')) {
             // Swap with the previous top-level row.
             var groupHeight = 0;
-            while (previousRow && $('.indentation', previousRow).length) {
-              previousRow = $(previousRow).prev('tr').get(0);
-              groupHeight += $(previousRow).is(':hidden') ? 0 : previousRow.offsetHeight;
+            while (previousRow && $previousRow.find('.indentation').length) {
+              $previousRow = $(previousRow).prev('tr').eq(0);
+              previousRow = $previousRow.get(0);
+              groupHeight += $previousRow.is(':hidden') ? 0 : previousRow.offsetHeight;

Couldn't $previousRow = $(previousRow).prev('tr').eq(0); be re-written as $previousRow = $previousRow.prev('tr').eq(0);?

@@ -389,9 +399,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
         break;
       case 40: // Down arrow.
       case 63233: // Safari down arrow.
-        var nextRow = $(self.rowObject.group).filter(':last').next('tr').get(0);
-        while (nextRow && $(nextRow).is(':hidden')) {
-          nextRow = $(nextRow).next('tr').get(0);
+        var $nextRow = $(self.rowObject.group).filter(':last').next('tr').eq(0);
+        var nextRow = $nextRow.get(0);
+        while (nextRow && $nextRow.is(':hidden')) {
+          $nextRow = $(nextRow).next('tr').eq(0);
+          nextRow = $nextRow.get(0);

Same issue with $nextRow = $(nextRow).next('tr').eq(0);.

@@ -704,13 +722,15 @@ Drupal.tableDrag.prototype.updateField = function (changedRow, group) {
   // Parents, look up the tree until we find a field not in this group.
   // Go up as many parents as indentations in the changed row.
   else if (rowSettings.relationship == 'parent') {
-    var previousRow = $(changedRow).prev('tr');
-    while (previousRow.length && $('.indentation', previousRow).length >= this.rowObject.indents) {
-      previousRow = previousRow.prev('tr');
+    var $previousRow = $changedRow.prev('tr');
+    var previousRow = $previousRow;
+    while ($previousRow.length && $previousRow.find('.indentation').length >= this.rowObject.indents) {
+      $previousRow = $previousRow.prev('tr');
+      previousRow = $previousRow;

We don't need to set previousRow = $previousRow; in this bit of code because previousRow isn't used anywhere in or after this bit of code in the Drupal.tableDrag.prototype.updateField function.

+++ b/core/modules/color/preview.js
@@ -7,17 +7,17 @@
   Drupal.color = {
     callback: function(context, settings, form, farb, height, width) {
       // Solid background.
-      $('#preview', form).css('backgroundColor', $('#palette input[name="palette[base]"]', form).val());
+      form.find('#preview').css('backgroundColor', form.find('#palette input[name="palette[base]"]').val());

Is form passed as a jQuery object? I can't tell from the code in preview.js. Ditto with form in bartik/color/preview.js.

@@ -140,8 +144,8 @@ Drupal.fieldUIOverview = {
    */
   onChange: function () {
     var $trigger = $(this);
-    var row = $trigger.closest('tr').get(0);
-    var rowHandler = $(row).data('fieldUIRowHandler');
+    var $row = $trigger.closest('tr');
+    var rowHandler = $row.data('fieldUIRowHandler');

Shouldn't $row be defined like this? var $row = $trigger.closest('tr').eq(0); That would match the pattern you set earlier.

If you address my comments in #42 and #44 and roll a new patch (if needed), I can do an interdiff and mark this RTBC. This is a BIG patch and I went through it with a fine grained comb.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new93.84 KB
PASSED: [[SimpleTest]]: [MySQL] 35,907 pass(es).
[ View ]

Fixed the $(context) in machine_name.js
Fixed the issues of tabledrag.js
For form in preview, it's a jQuery object, have a look at modules/color/color.js:68 and form is defined at line 12 of the same file.

and for $row, .closest will always return one result, before it was using parents(), it was a mess and it needed the .get(0) to work properly.

Thanks so much for the review. I'll be sure to let you know how the changes affected perfs :D

(edit) ok did some checks on the performance side, some pages takes a bit more time to init on FF, everything's faster in chrome, don't know about IE yet. overall that's positive a couple of page got the time to go wayyyy down. I'll go back working on my reporting tools to have something more meaningful :p

droplet’s picture

Status:Needs review» Needs work

If i'm not wrong...

Couldn't $previousRow = $(previousRow).prev('tr').eq(0); be re-written as $previousRow = $previousRow.prev('tr').eq(0);?

@John suggest =

$(self.rowObject.element).prev('tr').eq(0)  // filter out other element, return single element
.prev('tr').eq(0);  // catch nothing.

Patch #41 is right. That restart to get the object again.

      case 63232: // Safari up arrow.
        var $previousRow = $(self.rowObject.element).prev('tr').eq(0);
        var previousRow = $previousRow.get(0);
        while (previousRow && $previousRow.is(':hidden')) {
          $previousRow = $(previousRow).prev('tr').eq(0);
          previousRow = $previousRow.get(0);
        }

also affect this condition, seems not correct??

       
  else if (self.table.tBodies[0].rows[0] != previousRow || $previousRow.is('.draggable')) {
nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new93.91 KB
PASSED: [[SimpleTest]]: [MySQL] 35,913 pass(es).
[ View ]

droplet is right, I forgot why it was this way but that's indeed the reason why I didn't change it in my previous patches.

reroll of #41 the only change is $context = $(context) is out of the loop for machine_name.

I'm glad it's explained somewhere at least. I want to refactor tabledrag for D8 anyway so we can reduce the huge code debt of this script.

JohnAlbin’s picture

Status:Needs review» Reviewed & tested by the community

ok. I reviewed the changes in #47 vs. my previous line-by-line review of #41, and all my comments in #42 and #44 have been addressed.

Nice work!

droplet’s picture

StatusFileSize
new25.59 KB

Anyway, why it needs eq(0) ??

My suggestion to tableDrag part.

nod_’s picture

Yeah we should be able to get rid of them. This is not a $(selector, context) into $(context).find('selector') so I'd rather make this change in an other issue, this patch is finally RTBC thanks to JohnAlbin!

We're going to need an "Optimize selectors" issue at some point as well.

JohnAlbin’s picture

Nod_ and droplet, yep, can one of you create a follow-up issue to work on the tabledrag inefficiencies? We can move the patch from #49 to that issue.

Unless others see problems in patch #47 that could break things, it remains RTBC.

Note to committers: Ignore the patch in #49 (it will be a follow-up). The patch in #47 is RTBC. I reviewed it in comments #42, #44 and #48.

catch’s picture

Status:Reviewed & tested by the community» Needs work

OK I went to commit this but it no longer applies:

patching file core/modules/user/user.js
Hunk #3 FAILED at 47.
Hunk #4 succeeded at 186 (offset 5 lines).

Not sure which patch broke it, but could we get a quick re-roll?

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new94.07 KB
PASSED: [[SimpleTest]]: [MySQL] 36,162 pass(es).
[ View ]
droplet’s picture

Status:Needs review» Reviewed & tested by the community

Here is the new changes:

@@ -47,11 +47,12 @@ Drupal.behaviors.password = {
         }

         // Adjust the length of the strength indicator.
-        $(innerWrapper).find('.indicator').css('width', result.strength + '%');
-        $(innerWrapper).find('.indicator').css('background-color', result.indicatorColor);
+        innerWrapper.find('.indicator')
+          .css('width', result.strength + '%')
+          .css('background-color', result.indicatorColor);

Remark for follow up issue:
user.js

     var innerWrapper = $(this).parent();
      var outerWrapper = $(this).parent().parent();

Needs a clean-up

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks folks. Committed and pushed to 8.x, moving back to 7.x for backport. We should also add a note in coding standards if there's not one already.

cosmicdreams’s picture

nod_’s picture

I'd like to hold off on the D7 backport. IE has a weird behavior performance-wise, it's slower overall.

I have a few things planned for D8 that will address that but it'll be too much of a change to be able to backport to D7.

I haven't had time to look exactly why perf are hit like that, tabledrag is definitely one of the reason but that can't be all there is to it.

droplet’s picture

Do you mean IE is slower with this patch ?

It's a big patch. Now is the good time to backport it.

nod_’s picture

yes, that's what I mean.

If we don't have time to address the perf issue this should not go in D7 and right now I have no time to dig into it. The good patch that needs to get in before and that has a lot of impact (in a good way) is #1428524: Replace all $.each() with filtered for loop.

droplet’s picture

tim.plunkett’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new660 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1419968-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
+++ b/core/modules/contextual/contextual.jsundefined
@@ -12,7 +12,7 @@ Drupal.contextualLinks = Drupal.contextualLinks || {};
-    $('div.contextual', context).once('contextual-links', function () {
+    $(context).find('div.contextual-links-wrapper').once('contextual-links', function () {

This was totally busted. Not sure how we/I didn't catch this.

s/contextual-links-wrapper/contextual

Status:Needs review» Needs work

The last submitted patch, drupal-1419968-61.patch, failed testing.

tim.plunkett’s picture

Version:7.x-dev» 8.x-dev
Status:Needs work» Needs review
StatusFileSize
new660 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,362 pass(es).
[ View ]

.contextual-links-wrapper used to be the class name, until #1216978: Clean up the CSS for Contextual module. And it IS still the class name in D7.
So that might explain why the wires got crossed here.

Reupping for the bot.

aschiwi’s picture

Version:8.x-dev» 7.x-dev

Patch in #61 applies cleanly and works for me.

aschiwi’s picture

Version:7.x-dev» 8.x-dev
nod_’s picture

Version:8.x-dev» 7.x-dev

It's already in 8, this is the backport.

tim.plunkett’s picture

Version:7.x-dev» 8.x-dev

No, the patch committed to D8 was wrong, and #61/63 (same patch just for the testbot) fix it.

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

There is no div.contextual-links-wrapper in D8.

attiks’s picture

Status:Needs review» Needs work

TestSwarm just complained, reason is "find$" instead of "find" in core/misc/states.js 530

$(document).bind('state:collapsed', function(e) {
  if (e.trigger) {
    if ($(e.target).is('.collapsed') !== e.value) {
      $(e.target).find$('> legend a').click();
    }
  }
});
cosmicdreams’s picture

so.....
Which patch needs to be fixed (please reference comment number)

attiks’s picture

#53 contains the error

droplet’s picture

Status:Needs work» Needs review
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 35,409 pass(es).
[ View ]
JamesK’s picture

Status:Needs review» Reviewed & tested by the community

Tested #71. Looks good.

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs work

Thanks for the follow-up. Committed/push to 8.x, moving to 7.x again.

nod_’s picture

Status:Needs work» Postponed

I'll leave this issue postponed for a while. There is a performance regression in older IEs with this new patch. To compensate for it the code needs to be cleaned up and that's probably way more changes than what is backportable to D7.

I don't want to mess around too much with D7 JS. If nobody is up for better perf testing and fixing it i'll just close this in a while.

sun’s picture

Version:7.x-dev» 8.x-dev
Status:Postponed» Fixed
Issue tags:-needs backport to D7

I actually think that this change, while being a minor performance improvement, is not really worth to backport.

There's simply too much functionality that would have to be tested in-depth, and we'd have to be 100% sure that every single detail works like before, in order to not break hundreds of thousands of Drupal 7 production sites.

Consequently, I think we can spend our time much better with cleaning up JS in D8 and making that some true awesomesauce. Hence, moving back to D8 and fixed.

nod_’s picture

agreed, thanks.

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

Anonymous’s picture

Issue summary:View changes

typo