Use of .parents() is really .closest()

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

Update status.

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.

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

This version works in the ui.

Let's get first the bugfix into d8.

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

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

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?

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

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.

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.

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

Yep. Looks like everything is still working.

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

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

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.

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.

I agreed with @nod_

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.

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

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!!