In core JS files, every time .parents() is used .closest() is what was meant.

.closest()
Begins with the current element
Travels up the DOM tree until it finds a match for the supplied selector
The returned jQuery object contains zero or one element

.parents()
Begins with the parent element
Travels up the DOM tree to the document's root element, adding each ancestor element to a temporary collection; it then filters that collection based on a selector if one is supplied
The returned jQuery object contains zero, one, or multiple elements

The js present in core never use multiple elements and always add a selector to limit results to 1 element. It's used with either :first, .get(0), .eq(0) or parents('tr')[0] and sometimes it just hopes there is no nested structures (tableselect/tabledrag) #1319420: multiple execution of tableselect.js with nested tables.

The following patch replace all occurrences of .parents() to .closest(), in the very deep HTML tree Drupal usually outputs that could mean some pretty serious performance improvement. I checked every replacement by hand, it's all still working :)

Along the way I fixed a bug in shortcut.admin.js where "New set" wouldn't be checked when focusing the text input.
filter.js could be less complicated as the change event bubbles up, it can be catched by .filter-wrapper directly.

Files: 
CommentFileSizeAuthor
#28 core-js-parents-to-closest-backport-1400310-28.patch9.85 KBm.stenta
PASSED: [[SimpleTest]]: [MySQL] 38,432 pass(es). View
#24 core-js-parents-to-closest-backport-1400310-24.patch9.89 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 37,892 pass(es). View
#13 drupal8.closest-revert.13.patch9.93 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,341 pass(es), 1 fail(s), and 0 exception(es). View
#8 core-js-parents-to-closest-backport-1400310-8.patch9.89 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 37,023 pass(es). View
#8 core-js-parents-to-closest-1400310-8-D8.patch659 bytesnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-parents-to-closest-1400310-8-D8.patch. Unable to apply patch. See the log in the details link for more information. View
#6 core-js-parents-to-closest-backport.patch10.51 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 37,008 pass(es). View
core-js-parents-to-closest.patch10.57 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 34,609 pass(es). View

Comments

nod_’s picture

Status: Active » Needs work

woops, forgot this.

nod_’s picture

Status: Needs work » Needs review

It's too late, sorry about this.

dawehner’s picture

+++ b/core/misc/tableselect.jsundefined
@@ -29,7 +29,7 @@ Drupal.tableSelect = function () {
+        $(this).closest('tr').toggleClass('selected', this.checked);

Oh that's a nice feature os jquery.

+++ b/core/modules/shortcut/shortcut.admin.jsundefined
@@ -103,7 +103,7 @@ Drupal.behaviors.shortcutDrag = {
+      $(this).closest('form').find('.form-item-set .form-type-radio:last input').attr('checked', 'checked');
     };

Trying out d8 had the behavior that the ui didn't automatically switched to the new radio button.
The new patch fixed this
This code looks quite unspecific, as there could be other forms which has new somewhere in the block but that's outside of scope of this issue.

-22 days to next Drupal core point release.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Update status.

Dries’s picture

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

This looks like a nice clean-up that can improve performance and fix bugs. Committed to 8.x.

We should backport this to Drupal 7.

nod_’s picture

Status: Patch (to be ported) » Needs review
FileSize
10.51 KB
PASSED: [[SimpleTest]]: [MySQL] 37,008 pass(es). View

Was not tested as well as D8 patch but everything looks in order.

dawehner’s picture

Status: Needs review » Needs work

The patch is the same as for 8.x so i just test the functionality.

As written in irc this patch breaks the views ui. You have to click twice on every link to get it working, I'm sorry for that :(

nod_’s picture

Status: Needs work » Needs review
FileSize
659 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-parents-to-closest-1400310-8-D8.patch. Unable to apply patch. See the log in the details link for more information. View
9.89 KB
PASSED: [[SimpleTest]]: [MySQL] 37,023 pass(es). View

My apologies, I messed up the only relevant use of .parents().

This applies to D8 too, the following patches fixes the issue for D8 and the patch is rerolled for D7 including the fix (more like not including my "fix" :p).

Sorry about that.

dawehner’s picture

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

This version works in the ui.

Let's get first the bugfix into d8.

Dries’s picture

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

Committed the bugfix core-js-parents-to-closest-1400310-8-D8.patch (659 bytes) to 8.x. Back to Drupal 7.

sun’s picture

Version: 7.x-dev » 8.x-dev
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

WTF? Seriously, what's going on here?

Change the JS of plenty modules without a single confirmation that the expected functionality still works in all browsers...?

sun’s picture

Status: Needs review » Needs work

That said, the changes converting .parents('*:first') into .closest('*') seem to be correct.

I did not, however, test them manually.

+++ b/misc/tableselect.js
@@ -29,7 +29,7 @@ Drupal.tableSelect = function () {
         // Either add or remove the selected class based on the state of the check all checkbox.
-        $(this).parents('tr:first')[ this.checked ? 'addClass' : 'removeClass' ]('selected');
+        $(this).closest('tr').toggleClass('selected', this.checked);

@@ -39,14 +39,14 @@ Drupal.tableSelect = function () {
     // Either add or remove the selected class based on the state of the check all checkbox.
-    $(this).parents('tr:first')[ this.checked ? 'addClass' : 'removeClass' ]('selected');
+    $(this).closest('tr').toggleClass('selected', this.checked);

toggleClass() was intentionally not used here, because it is unreliable in bulk operations.

When clicking the check-all checkbox, a checkbox (or checkbox row) is factually unchecked if it is already checked.

+++ b/modules/shortcut/shortcut.admin.js
@@ -106,7 +106,7 @@ Drupal.behaviors.shortcutDrag = {
     var selectDefault = function() {
-      $($(this).parents('div.form-item').get(1)).find('> label > input').attr('checked', 'checked');
+      $(this).closest('form').find('.form-item-set .form-type-radio:last input').attr('checked', 'checked');
     };
     $('div.form-item-new input').focus(selectDefault).keyup(selectDefault);

This selects something completely different.

As noted in #3 it seems to fix an actual, separate bug. So it's quite unfortunate to have that fix squeezed in here. Did you search for existing issues?

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.93 KB
FAILED: [[SimpleTest]]: [MySQL] 33,341 pass(es), 1 fail(s), and 0 exception(es). View

Lacking proper reviews and any proper testing, here's a total revert of the cumulative patches.

EDIT: That said, using a native git revert would be better, as it provides more context and magic:

git reset --hard
git revert 47ca221a13ffe253c1a67d64457f2be93bb684a8
git revert f5563bf8778f9e371296c767972fc2b8f9b6bf6f

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.closest-revert.13.patch, failed testing.

nod_’s picture

Wow I certainly wasn't trying to get anyone angry.

I did search for existing issues regarding #3, no luck. As it's not a change in js present in /misc and it's used only for shortcut admin I don't see the issue of selecting a different element, which is needed because it's not possible to fix this with what the former selector was giving. parents was used incorrectly here too.

I checked that elements selected by the new selectors were exactly the same as before for every replacement. If it's different from a browser to another it's a bug in jQuery.

Regarding the toggleClass I didn't know about the reliability issue (never had an issue with it), do you have more info? I'm interested to know about that and I couldn't find a ticket talking about this in jQuery's bug tracker. And I can't see the issue you're raising about checkbox rows getting unchecked (or rather unselected), works for me.

As for the mess in ajax.js that broke views UI, it was me trying to be too smart.

(edit) oh and what's happening with js testing? should I go with qunit module if I want to write tests for my patches? Because this is not going to end well if manual testing is needed for all js refactors.

sun’s picture

Status: Needs work » Fixed

I overlooked that the second parameter to .toggleClass() was used here, which is a Boolean to enforce either .addClass() or .removeClass(), instead of the regular .toggleClass() behavior.

manual testing is needed for all js refactors

Yes, manual testing is required for all JS patches. Additionally, some JS is written in a way to allow contrib modules to do advanced things, so some API changes may need to be confirmed by testing whether more complex contrib modules still work or can be adapted.

nod_’s picture

I figured testing was required, just wanted to know if writing core tests for the qunit module could be considered testing and make our life much easier. Workflow ending up like, here is my patch, you can find qunit tests there run that on your browser check it's enough testing and mark need work or rtbc. I wasn't hopping for test bot integration.

Dries tagged it as needing backport to 7.x, what's the final decision about that ?

sun’s picture

What essentially happened here is that the D8 patch is now tested by the crowd of core developers. Since D8 cannot be used for production sites, it's unlikely that anyone happens to "implicitly" test all of these changes soonish.

Someone needs to have a close look at the queue of new issues filed for D8 core to make sure that this change did not break something.

A backport to D7 requires full manual testing.

nod_’s picture

Version: 8.x-dev » 7.x-dev
Priority: Major » Normal
Status: Fixed » Needs review

It's all in the hands of a Good Samaritan then.

nod_’s picture

Issue tags: +JavaScript clean-up
nod_’s picture

If people want to help review, patch for D7 is at #8.

Niklas Fiekas’s picture

Status: Needs review » Needs work

Hit the wrong patch, or does it need a reroll?

niklas@niklas-pc:~/Projekte/drupal-7$ git apply core-js-parents-to-closest-backport-1400310-8.patch
error: patch failed: misc/tabledrag.js:125
error: misc/tabledrag.js: patch does not apply
error: patch failed: modules/file/file.js:96
error: modules/file/file.js: patch does not apply
error: patch failed: modules/simpletest/simpletest.js:14
error: modules/simpletest/simpletest.js: patch does not apply
nod_’s picture

Yeah probably, quite a few changes has been made on js files.

nod_’s picture

Status: Needs work » Needs review
FileSize
9.89 KB
PASSED: [[SimpleTest]]: [MySQL] 37,892 pass(es). View

Should work now :)

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community

Yep. Looks like everything is still working.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Can we be clear on what exactly was tested here, and in what browsers?

nod_’s picture

The change is browser agnostic, and I've using the patch on my dev D7 and didn't run into any issue with tabledrag, file upload and the rest of it.

Went to each page the files were used and it still works. But since I made the patch…

m.stenta’s picture

FileSize
9.85 KB
PASSED: [[SimpleTest]]: [MySQL] 38,432 pass(es). View

The patch in #24 didn't apply against the latest HEAD (due to commit 0025dcb), so I rerolled it.

m.stenta’s picture

Status: Needs review » Reviewed & tested by the community

Tested the following:

I went through each change and tested them all manually. These are the results. Note that there were a few specific pieces I did not test (see note at bottom of this comment).

Attaching machine-name form element JS bahavior

Pages tested:
/admin/structure/taxonomy/tags/edit (vocabulary machine name)
/admin/structure/types/manage/article (content type machine name)

Results:
-Successfully attaches the JS behavior to the machin-name form element, causing it to be collapsed into something like "Machine name: article [Edit]" next to the "Name" field, and clicking "[Edit]" expands the machine-name field as expected.

Hiding "weight" columns in draggable tables

Pages tested:
/admin/structure/taxonomy/tags (tags vocab with some test terms)

Results:
-Successfully hides the "weight" column.

"Select all" checkbox behvaior in tableselect.js

Pages tested:
/admin/content

Results:
-Successfully selects all checkboxes when the "Select All" checkbox is checked.
-Successfully checks the "Select All" checkbox when all checkboxes are manually checked.
-Successfully unchecks the "Select All" checkbox when all of the checkboxes are checked, and then one or more is manually unchecked.

Blocks region select list

Pages tested:
/admin/structure/block

Results:
-Successfully moves a block from one region to another when a different region is selected in the block's Region select list.

"Add new field" field-type select list

Pages tested:
/admin/structure/types/manage/article/fields

Results:
-Successfully updates the "Widget" select list options when the value of the "Field type" select list changes

"Add existing field" name select list

Pages tested:
/admin/structure/types/manage/page/fields

Results:
-Successfully updates the "Widget" select list options, and fills in the "Label" text field when the "Select an existing field" select list is changed.

"Format" select list in Fields "Manage Display"

Pages tested:
/admin/structure/types/manage/article/display

Results:
-Successfully changes the region that a field is in when the "Format" select list is set to "".

File field extension validation

Pages tested:
/node/1/edit

Results:
-Successfully displays validation error message above file field when a file with the wrong extension is selected.

Input filter guidelines

Pages tested:
/node/2/edit

Results:
-Successfully swaps out the input filter guidelines UL list when a different one is selected from the "Text format" select list.

"Cool table collapsing on the testing overview page"

Pages tested:
/admin/config/development/testing

Results:
-Successfully collapses and expands the test groups in the list of tests.

Specific parts I did NOT test:

I wasn't sure where to go to test the following pieces, or they involved more in-depth environment setup:

Line 757 of misc/tabledrag.js:

<?php
targetElement.value = $('.indentation', $(sourceElement).closest('tr')).length;
?>

Line 889 of misc/tabledrag.js:

<?php
this.table = $(tableRow).closest('table').get(0);
?>

Line 98 of modules/file/file.js:

<?php
// Check if we're working with an "Upload" button.
var $enabledFields = [];
if ($(this).closest('div.form-managed-file').length > 0) {
  $enabledFields = $(this).closest('div.form-managed-file').find('input.form-file');
}
?>

Line 121 of modules/file/file.js:

<?php
/**
 * Add progress bar support if possible.
 */
progressBar: function (event) {
  var clickedButton = this;
  var $progressId = $(clickedButton).closest('div.form-managed-file').find('input.file-progress');
?>

Line 135 of modules/file/file.js:

<?php
    // Show the progress bar if the upload takes longer than half a second.
    setTimeout(function () {
      $(clickedButton).closest('div.form-managed-file').find('div.ajax-progress-bar').slideDown();
    }, 500);
?>

Line 109 of modules/shortcut/shortcut.admin.js:

<?php
/**
 * Make it so when you enter text into the "New set" textfield, the
 * corresponding radio button gets selected.
 */
Drupal.behaviors.newSet = {
  attach: function (context, settings) {
    var selectDefault = function() {
      $(this).closest('form').find('.form-item-set .form-type-radio:last input').attr('checked', 'checked');
    };
    $('div.form-item-new input').focus(selectDefault).keyup(selectDefault);
  }
};
?>

However, I think it's safe to say that they will work, because the changes that have been made to them are syntactically the same as all the changes that I DID test above.

nod_’s picture

Oh my god, you're awesome. Thank you so much for testing all of this.

the bits and pieces you did not test in tabledrag were implicitly tested since it's used by other pieces of code. The file js was tested if you sucessfully uploaded a big file and the progressbar showed up. for the newSet, if on the shortcut module by clicking on "Other" the textfield got focus it would be a successful test.

If we ever run into each other, drinks are on me!

m.stenta’s picture

Status: Reviewed & tested by the community » Needs review

Just talked with xjm, and she said to mark this back to "Needs Review" until it can get tested in all supported browsers for 7.x (http://drupal.org/node/61509).

Since this is already committed to 8.x, that just means in needs to work in IE 6 and 7 (support for those two is dropped in 8.x).

Edit: FYI ... I tested it in Chrome originally.

nod_’s picture

the change is browser agnostic, if there is an issue it's a jQuery/Sizzle bug.

Thanks again for taking the time to do all this.

droplet’s picture

I agreed with @nod_

m.stenta’s picture

Status: Needs review » Reviewed & tested by the community

Changing this back to RTBC, after further discussion with xjm. Since the JS maintainer is cool with it, it should be good to go. I'll leave it up to the committer to decide whether it needs manual IE6 testing, or if we can assume it's good to go based on jQuery's browser agnosticism.

To-the-committer-whom-it-may-concern: see comment #29 for my full testing (done in Chrome) and #32 for nod_'s go-ahead. Also note this is already committed to 8.x, so it's just a matter of IE6 compatibility, which as nod_ said, is up to jQuery and should work.

tim.plunkett’s picture

Issue tags: -Needs manual testing

Confirming RTBC, in case there was any worry about the reroller RTBCing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs manual testing

Wow, that's some EPIC manual testing there, m.stenta! Thank you so much!

I agree with nod_ that since we're testing a jQuery function change, testing in one browser should be sufficient here.

Committed and pushed to 7.x. Thanks!!

nod_’s picture

Awesome! thanks everyone for reviewing the patch!

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