Nested Paragraphs create multiple drag handles

The bug exists in 8.9x as well.

From @lauriii:

we should extend Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest and change the theme to Claro and add additional test case for the nested table drags.

This specific issue is the bug of Claro's tabledrag.js rewrite, and happens in every case when draggable tables are nested into each other.

Tabledrag_test module

I improved the tabledrag_test module (core/modules/system/tests/modules/tabledrag_test) by adding a nested tabledrag test page. I refactored the original Drupal\tabledrag_test\Form\TableDragTestForm to be able to re-using the code in the new form's controller.
Basically, the existing test table is reused as the content of the parent table's first row's first cell.

TableDragTest

While I added the new test method, I has to refactor some minor things in this test class.

• Added a new test method to Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest: testNestedDraggableTables. This is testing the nested draggable tables provided by the new test page added to tabledrag_test.
• In order to reuse the methods, I moved the content of the original ::testKeyboardAccessibility into a new, protected method ::assertKeyboardAccessibility that accepts (optional) arguments ($drupal_path, $structure).
• I also had to add (optional) arguments to ::assertDraggableTable: These are $table_id and $skip_missing. They're used for making assertions on the parent table's structure.
• The same happened to the assertTableRow method: it got a new optional argument $skip_missing that is used for optionally skipping those value assertions that's target field's aren't present in the parent table.
• findRowById also had to got an additional argument because of the reasons above.

Additional bugs I've found:

1. Stark places the nested parent table's drag handle to a completely wrong location. It adds the handle in the child table's (last?) rows.
2. I tried to repeat the same assertions of testRootLeafDraggableRowsWithKeyboard rendered in an another draggable table's row, but it's completely broken in core.
3. When I tried to use a draggable parent table with indentation enabled, I wasn't able to get the same results for the nested table as we get from the non-nested tests, and this happened even by repeating testKeyboardAccessibility and testRootLeafDraggableRowsWithKeyboard.
4. RTL drag and drop is broken in core (#197641: Drag and drop is not RTL aware), and even with Claro (but only with nested draggable tables).

Todo: report issues.

This is a reroll that was necessary after #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests

Unfortunately, this reroll results in ClaroTableDragTest not working. I believe it's due to how far within a target row a dragged row needs to be before it can be placed there.

It can be made to work by changing the creation of currentRowCenterOffset to this:
const currentRowCenterOffset = direction === 'down' ? currentRowOffestY + (currentRowHeight / 4) : currentRowOffestY + (currentRowHeight / .75);, so the dragged row doesn't need to travel as far into the row it's trying to bump out of place. There may be a better way, but this is what I discovered when confirming the test failures were not due to problems with the reroll.

Keeping at "Needs review" to trigger tests, but this should switch to "Needs work" automatically unless there's something up my my local test env.

Re @bnjmnm,

Actually, you are wrong. The test added in #13 uses the keyboard for testing the draggable tables.

Could you please provide an interdiff? It would help a lot.

(I do have a case that highlights why we need the calculation fixes, but the needed case is managed by the test module AND the test I changed in $13.)

The test added in #13 uses the keyboard for testing the draggable tables.

-- right, I think this has more to do with the changes to TableDragTest that happened in #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests - the issue that also made the reroll necessary. The attached currentRowCenterOffset-attempt.patch adds that change to the offset and allows ClaroTableDragTest to pass. The interdiff of that attempt-patch and the current patch is interdiff_18--changing-currentRowCenterOffset.txt

There was also a code standards mistake in the reroll from #15 so a fix for that is attached too.

I'll create a demo module and a video for highlighting why the improved claro drag'n'drop is untestable with dragTo (and why is it still much more better than the default).

Ah, I understand now. The tests are failing on testDragAndDrop(), a method recently added to TableDragTest in #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests. That method uses dragTo(), which sounds like it's not appropriate for testing Claro's tabledrag. Overriding that method in ClaroTableDragTest, is a more appropriate solution than changing offsets (which felt out of scope even when I mentioned it).

This seems to be an issue in the Claro tabledrag, which has overriden the core tabledrag

Does anybody know why Claro overrode core's tabledrag.js? The git logs from the old contrib repo only have one commit message for that file:

commit 5625a33c33ffa17ed43c672ec10dea7ea0f127ba
Author: Lauri Eskola <lauri.eskola@acquia.com>
Date:   Thu Oct 10 22:50:00 2019 +0300

    Issue #3087187 by ckrina, justafish: Remove claro. prefix from JavaScript files

so it's unclear as to why the override is necessary...

Ah, nevermind, I found the other log messages using --follow since the file was renamed, so there is a bit more history:

* 5625a33 Issue #3087187 by ckrina, justafish: Remove claro. prefix from JavaScript files - Lauri Eskola, 9 weeks ago
* fa01680 Issue #3086995 by lauriii, Wim Leers: Update eslint rules to match with core - Lauri Eskola, 9 weeks ago
...
* 78c908d Issue #3021094 by huzooka, lauriii, ckrina, fhaeberle, antonellasevero, jasonbarrie: File field style update - Lauri Eskola, 9 weeks ago
...
* 7cd522a Issue #3023326 by huzooka, lauriii, finnsky, lot007, ckrina, evankay, fhaeberle: Field Cardinality Style Update - Lauri Eskola, 3 months ago
...
* d3cb661 Issue #3032365 by huzooka, bnjmnm, lot007, lauriii, ckrina: Table drag style update - Lauri Eskola, 3 months ago

There's also documentation in the tabledrag.es6.js about what we changed which might help understanding why it was done ✌️

#13 applies cleanly over 8.8.0 (the next patches do not) and it fixes the issue for me.

Re #25, #26:

The patch from #15 (and #18) are for core 8.9.x.

Yesterday I released CD tools 2.4.

If you enable cd_tools:tabledrag, you'll have even more complex draggable table examples on the /tabledrag path.

Based on the nested draggable tables, Claro needs more work to be faultless.

Added a minor enhancement, and I also attached two videos that compare Seven (core-default tabledrag.js) and Claro (replaced tabledrag.js).

Claro seems to be RTL-friendly with the most recent patches.

Core tabledrag does not add the expected tabledrag-changed mark if the moved row contains it somewhere (e.g. in a nested draggable table).

So I decided to skip that assertion (see the interdiff).

I also attached a screen capture that shows why the drag test fails with Claro.

Long story in short, Claro switches the rows only if the pointer device passes the 'hovered' row's middle line. This does not happens if we're using dragTo() for these kind of tests. But besides this, I still think that Claro's current approach is the best (current means the patch I attach now). The visual UX-explanation can be checked by watching the Seven-vs-Claro--draggable-table-with-mixed-row-height.mp4 video from #30.

In order to make Claro's tabledrag.js testable with dragTo(), I had to override TableDragTest::testDragAndDrop() in

\Drupal\FunctionalJavascriptTests\Theme\ClaroTableDragTest

, and besides this, make Claro's tabledrag.js dragTo-friendly (it seems to be a rare condition if the pointer moved 100 pixels in one step, but who knows...).

And here is the interdiff for #37 (I forgot adding it previously).

I tried patch #37 on drupal 8.8.1, but it failed to apply the patch for me. (with claro enabled as backend theme).

I tried this https://www.drupal.org/files/issues/2019-12-20/claro-nested_paragraphs_m... patch.

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2019-12-20/claro-nested_paragraphs_multiple_drag_handles-3092181-37--complete.patch

[Exception]
Cannot apply patch Nested Paragraphs create multiple drag handles (https://www.drupal.org/files/issues/2019-12-20/claro-nested_paragraphs_multiple_drag_handles-3092181-37--complete.patch)!

Or does it only work on the dev branch of drupal?

@bskibinski It seems like the patch only applies on the dev branch (8.9.x).

@bskibinski & @lauriii : I adapted the patch #37 to work on 8.8.1.
I'm not sure if it's ok to post it here, but it could be handy for all the early adopters.

@elgandoz: Thanks, this works well on 8.8.1!

Awesomesauce! @elgandoz thank you so much! helps out a lot over here!
patch works great.

There seem to be a fairly large number of unrelated coding standards cleanup changes in the tabledrag file, which makes reviewing this very hard. Any way that we can trim this down to the absolute minimal changes that are necessary to make this work?

For me the patch doesn't apply with 8.8.2 and Paragraphs 1.10.0

RE @saschaeggi, @ swatichouhan: this is still fine.

The target version is 8.9.x, not 8.8.whatever.
I re-scheduled a test run for #37, and it seems that it still applies cleanly and passes the tests.

Back to NR.

@saschaeggi the patch from #43, which is not meant for dev purposes, applies fine on 8.8.2.

Applied patch.
before:

after:

@slasher13 I'm switching this back to "Needs review" as a core patch (particularly one of this size) requires a more thorough review than before/after screenshots. This is understandable, however, as I've seen plenty of documentation that suggests this is all that is required in the review process This guide does a good job of succinctly describing what is needed from a review.

I did a quick pass and spotted a few typos. For a patch this large further review is still needed, however.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -130,16 +144,30 @@ public function testDragAndDrop() {
   +   *   The expected table structure. If this isn't specified or equals to NULL,
   +   *   than the expected structure will be set by this method. Defaults to NULL.
   

   s/equals to/equals
   s/than/then

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -307,6 +335,55 @@ protected function assertOrder(array $items) {
   +    // Re-test the nested draggalbe table.
   

   s/draggalbe/draggable

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroTableDragTest.php
   @@ -27,4 +37,65 @@ protected function findWeightsToggle($expected_text) {
   +   * Tests draggable table drag'n'drop.
   

   "drag'n'drop" should be changed to drag and drop to match usage in the rest of core.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroTableDragTest.php
   @@ -27,4 +37,65 @@ protected function findWeightsToggle($expected_text) {
   +   * In Claro, the pointer should pass the horizontal center of the currenty
   

   s/currenty/currently

Tested on 8.8.2 version of Drupal core and result is OK

Navigating the issue queue is very confusing so it's understandable, but setting this to reviewed & tested by the community in #56 is not appropriate for a few reasons.

1. This patch is for 8.9.x, (check "Version" in the issue metadata) so testing on 8.8.2 is not pertinent to the requirements of the issue.
2. Comment #55 points out four things that require additional work. Those need to be fixed before there's any possibility of setting this to RTBC. (Anyone can upload a patch with these changes!)
3. #55 also points out that patching and screenshots is only a small part of reviewing a core patch. Particularly for a 66k patch, a more thorough review of the code itself is required. The patch review documentation on Drupal.org is a good place to get familiar with what is required of a review, and that documentation links to several other good resources that may help clarify things.

I rerolled the patch in #37 to fix spellings/grammar mistakes in #55
More review of source code would be required

Retested on 8.9.0 version of Drupal core and result is OK with patch from #59

43 coding standards messages

Coding standards check must pass, see https://www.drupal.org/pift-ci-job/1578231

Maybe #63 out of scope here

Here is a not-thoroughly-tested-and-quick backport for Drupal 8.8.3 based on #59.

For 8.9.x, please check #59.

Sorry, I messed up with the name. Here is the same file with a better name.

There are some in-scope coding standard violations.

• Running yarn lint:css reveals some violations added by this patch:
  

  themes/claro/css/components/tabledrag.pcss.css
   252:3   ✖  Expected "overflow" to come before "line-height"          order/properties-order    
   253:27  ✖  Expected single space after "," in a single-line          function-comma-space-after
              function                                                                            
   253:29  ✖  Expected single space after "," in a single-line          function-comma-space-after
              function

• Acheck of Claro's JS with node ./node_modules/eslint/bin/eslint.js ./themes/claro/
  

  core/themes/claro/js/tabledrag.es6.js
    1071:28  error  Replace `.toArray()` with `⏎············.toArray()⏎············`  prettier/prettier
    1074:28  error  Replace `.toArray()` with `⏎············.toArray()⏎············`  prettier/prettier
  

• This rule needs to be enforced for any arrays added in this patch (this happened on some test files)

  If the line declaring an array spans longer than 80 characters, each element
       |       |     should be broken into its own line

  . There may be existing arrays in these files that don't meet this standard, and those can remain as is as they were created before standards were updated. New code should adhere to this rule, however.

In addition to the CS items above, this still needs a full code review and evaluation of test coverage before it can be RTBC again.

The JS file still needs to be reviewed. Posting what I have so far as it may be a moment before I can get to that.

1. +++ b/core/modules/system/tests/modules/tabledrag_test/src/Form/NestedTableDragTestForm.php
   @@ -0,0 +1,62 @@
   +    $parent_row_ids = ['parent_1', 'parent_2', 'parent_3'];
   +    $parent_rows = array_combine($parent_row_ids, $parent_row_ids);
   
   +++ b/core/modules/system/tests/modules/tabledrag_test/src/Form/TableDragTestForm.php
   @@ -44,46 +44,56 @@ public function getFormId() {
      /**
   -   * {@inheritdoc}
   +   * Builds the draggable test table.
   +   *
   +   * @return array
   +   *   The renderable array of the draggable table used for testing.
       */
   -  public function buildForm(array $form, FormStateInterface $form_state) {
   -    $form['table'] = [
   +  protected function buildTestTable(array $rows = [], $table_id = 'tabledrag-test-table', $group_prefix = 'tabledrag-test', $indentation = TRUE) {
   

   Missing @param docblock info

2. The test-only patch clearly fails, but I wasn't sure if there was anything explicitly confirming the expected number of drag handles. In some cases, it's possible to drag/drop when the multiple handles are present and could result in a passing test that probably shouldn't be. I possibly missed an assertion that covers this.
   

3. +++ b/core/themes/claro/css/components/tabledrag.pcss.css
   @@ -283,11 +266,16 @@ body.drag {
     * These rules are styleing the inline SVG that is placed inside the .indetation
     * element.
     */
   +
   

   The additional space here isn't consistent with other comments. I'd be fine with sneaking in the typo correction of s/styleing/styling and s/indetation/indentation too :).

4. +++ b/core/themes/claro/css/components/tabledrag.pcss.css
   @@ -235,25 +235,6 @@ body.drag {
   -.tabledrag-cell-content .tree {
   -  min-height: 100%; /* Using simply 'height: 100%' would make IE11 rendering ugly. */
   -}
   -
   -/**
   - * Safari (at least version 13.0) thinks that if we define a width or height for
   - * and SVG, then we refer to the elements total size inside the SVG.
   - * We only want to inherit the height of the parent element.
   - */
   -/* stylelint-disable-next-line unit-whitelist */
   -@media not all and (min-resolution: 0.001dpcm) {
   -  @media {
   -    .tabledrag-cell-content .tree {
   -      overflow: visible;
   -      min-height: 0;
   -    }
   -  }
   -}
   

   Are these no longer needed due to changes in JS? Everything works well in Safari/IE, so it doesn't seem like they're needed, but I'd like to confirm

Even though there are many changes to the JS file, most of them fit into one of two categories: 1. managing scope by using self in a nested function 2. Changing several instances where ALL descendants are collected and changing to functions or selectors that get only immediate children or direct descendants, which I assume was the changes specifically targeted at this bug. It's probably worth one final review from me or someone else to triple-check uses of find() and selectors that might target all descendants when they shouldn't

1. +++ b/core/modules/system/tests/modules/tabledrag_test/src/Form/NestedTableDragTestForm.php
   @@ -0,0 +1,62 @@
   +    $form['actions'] = $this->buildFormActions();
   ...
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   +    $operation = isset($form_state->getTriggeringElement()['#op']) ?
   +      $form_state->getTriggeringElement()['#op'] :
   +      'save';
   

   The tests that access the controller don't actually submit the form so there's no need to include $form['actions'] and submitForm() can be removed since it'll meet the interface requirements via inheritance and it's not actually used.

2. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -1153,6 +1168,7 @@
   +            // We only have '.js-indentation', this will be always false.
   
   @@ -1167,6 +1183,7 @@
   +            // We only have '.js-indentation'.
   

   I'm confused by these comments. It looks like the logic around it is copied from misc/tabledrag.es6.js - perhaps these comments were added so we know to ignore this chunk of code when troubleshooting?

#68.4: I'm not sure I could find reason for changing from using max-height: 100% to using overflow: visible other than that the current solution is simpler. It seems like this was introduced by @huzooka on #30 so I tried to reach out to him. He wasn't able to recall what was the specific reason he did that change but he thought it might have been some kind of browser compatibility issue. I tried testing most of our supported browsers and I couldn't see any difference between the two approaches.

#69.4 I removed the comment and opened follow-up for addressing it properly.

This should address feedback from #67-#69.

Here's the follow-up: #3129871: Incorrect targetElement.className value when dragging table rows between regions.

This looks good! I just spotted one thing:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
@@ -317,13 +478,18 @@ protected function assertOrder(array $items) {
-  protected function assertDraggableTable(array $structure) {
-    $rows = $this->getSession()->getPage()->findAll('xpath', '//table[@id="tabledrag-test-table"]/tbody/tr');
-    $this->assertSession()->elementsCount('xpath', '//table[@id="tabledrag-test-table"]/tbody/tr', count($structure));
+    protected function assertDraggableTable(array $structure, $table_id = 'tabledrag-test-table', $skip_missing = FALSE) {
+      $rows = $this->getSession()->getPage()->findAll('xpath', "//table[@id='$table_id']/tbody/tr");
+      $this->assertSession()->elementsCount('xpath', "//table[@id='$table_id']/tbody/tr", count($structure));
 
     foreach ($structure as $delta => $expected) {
-      $this->assertTableRow($rows[$delta], $expected['id'], $expected['weight'], $expected['parent'], $expected['indentation'], $expected['changed']);
+      $this->assertTableRow($rows[$delta], $expected['id'], $expected['weight'], $expected['parent'], $expected['indentation'], $expected['changed'], $skip_missing);

The indentation here got bumped out of place

This should also get a 9.1.x-specific patch, it won't apply 100% cleanly to that branch.

Patch for 8.9.x

8.8.x backport. Ignored the following test files in the original patch:

• core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
• core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroTableDragTest.php

Tested #75 and it works like a charm! Great work!

I don't think this is RTBC, because there is some feedback from #73 that mentions a 9.x patch which is not done yet. In addition to that, you only tested the patch from #75, but not #74.

Also changing the priority back to normal, because this bug should be taken care of really soon as Claro is used more widely in combination with paragraphs now.

@hctom indentation mentioned in #73 has been fixed.
I think we need different issue for 9.0 and 9.1.
Can you list the issues you want me to fix?

@VladimirAus: I only pointed out the 9.x patch but no more issues (if this has to be done in a different branch, perhaps there should be a follow up, even though I think the problem should be addressed for 9.x here as well). I reverted the status to "Needs review", because only the 8.8.x patch has been tested, but not the 8.9.x patch. So there is still some review need before being able to set it to RTBC.

The 9.1 patch is in the scope of this issue and it does not require any extensive changes. It looks like the only necessary change is JavaScript formatting that is done with automatic build tools.

To make the 9.1 patch: Apply the 8.9 patch to the 9.1 branch then in /core run yarn prettier and yarn build:js to rebuild the JS files using the 9.1 standards. That should be all you need. Just one thing to watch out for: if any JS files change as a result of prettier/build:js that are not related to this patch, those should be reverted and not included.

Here's a reroll for 8.9.x and 9.0.x.

Rebuilt the .js files on the 8.9.x patch which seems to fix the test failure.

The 8 and 9 patches both look good aside from an extremely minor nit in both:

+++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
@@ -130,16 +144,30 @@ public function testDragAndDrop() {
+  /**
+   * Asserts accessibility through keyboard of a test draggable table.
+   * @param string $drupal_path
+   *   The drupal path where the '#tabledrag-test-table' test table is present.
+   *   Defaults to 'tabledrag_test'.
+   * @param array|null $structure
+   *   The expected table structure. If this isn't specified or equals NULL,
+   *   then the expected structure will be set by this method. Defaults to NULL.
+   */

CS nit. There needs to be a space before the first @param.

@bnjmnm #85 has been resolved.

It seems parts of this patch should go to the main tabledrag.js file, It will not be pretty if we have to support 2 different versions of the tabledrag script which have different functionality. Could you separate which fixes should go in core and which ones should go in claro?

Seems seven is also broken in the nested tabledrag situation. Doesn't seem to be a claro-only issue.

are all the self = this lines necessary to fix the bug? seems like scope creep.

The 8.8.x backport in #75 works like a charm for me.

For me, the path #75 (8.8.x) works well. Tested on core 8.8.6.

Re #88

It seems parts of this patch should go to the main tabledrag.js file, It will not be pretty if we have to support 2 different versions of the tabledrag script which have different functionality. Could you separate which fixes should go in core and which ones should go in claro?

Seems seven is also broken in the nested tabledrag situation. Doesn't seem to be a claro-only issue.

This problem doesn't occur in Seven. Screenshots attached for reference.

are all the self = this lines necessary to fix the bug? seems like scope creep.

Good point, in all but one case there's no change in scope to necessitate the use of self

The instances of self = this; added in this patch can all be removed with one exception (that could be addressed pretty easily);

The self = this; added in initColumns() is only needed for this bit that appears near the end because the scope changes due to it being in a jQuery callback.

$table
            .find('> thead > tr, > tbody > tr, > tr')
            .each(self.addColspanClass(columnIndex));

This can be avoiding by iterating through this.querySelectorAll('> thead > tr, > tbody > tr, > tr') instead. (done in the IE11 compatible way of iterating through a NodeList)

Similarly, most added self.(anything) can be changed to this.(anything), although if they're next to pre-existing calls to the self variable, those can remain as self for consistency.

To get this out of "needs work":

1. All instances of self = this; added in this patch should be removed,.
2. Added calls to self.(whatever) should be changed to use this.(whatever) unless they are next to pre-existing uses of self.(whatever)
3. In initColumns, this needs to be refactored to iterate without jQuery
   

   $table
               .find('> thead > tr, > tbody > tr, > tr')
               .each(self.addColspanClass(columnIndex));

8.9.x patch from #87 works well for me with Core 8.9.0.

Patch from #97 works for me with Drupal 8.9 and Drupal 9.0

... picking this one up for the D4D sprint to work on fixes for #92 ...

I've addressed issues 1 and 2 from #92.

Regarding item 3, I tried using this.querySelectorAll('> thead > tr, > tbody > tr, > tr') but when babel converts this down to es5, I get the following error in the js console:

Uncaught TypeError: _this2.querySelectorAll is not a function 

After inspecting this in the current context, I realized that the table could be target with this.table but when I tried this.table.querySelectorAll('> thead > tr, > tbody > tr, > tr'); I get the following error in the js console:

Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Element': '> thead > tr, > tbody > tr, > tr' is not a valid selector.

So I'm not sure what I'm doing wrong.

Futhermore, I don't get why we're going through all the trouble to back out jQuery, when the standard way to solve the parent scope problem in a javascript foreach is well known to just use self = this. I've tested with leaving this single instance of self = this in place in IE11 and modern Chrome all seems to work as expected in both places (tested using https://github.com/zolhorvath/cd_tools at the /tabledrag/nested route).

Anyway, here is a working patch against 9.1.x with interdiff agaist #87.

whoops, ignore 3092181-87-90x.patch attached to the previous comment. I meant to upload the interdiff, but grabbed the wrong file🤦‍♂️.

Thanks for the patches!

Regarding item 3, I tried using this.querySelectorAll('> thead > tr, > tbody > tr, > tr') but when babel converts this down to es5, I get the following error in the js console:
This should work:
$table[0].querySelectorAll('> thead > tr, > tbody > tr, > tr').etc.....

Futhermore, I don't get why we're going through all the trouble to back out jQuery, when the standard way to solve the parent scope problem in a javascript foreach is well known to just use self = this

self = this was a necessary convention pre-es6. JavaScript now provides this scoping natively and native support should be used over a library whenever possible, particularly in the case of jQuery, which will eventually be phased out of Drupal. Whenever possible, new JS in core should avoid using jQuery.

Thanks for the explanation. I'll take another crack at it.

1. This should work:
   $table[0].querySelectorAll('> thead > tr, > tbody > tr, > tr').etc.....

   This produces:

   Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Element': '> thead > tr, > tbody > tr, > tr' is not a valid selector.
   

   querySelectorAll requires that a selector with child combinators include the parent element, i.e parent > child.

   So, my solution was to grab the parent element outside the selector then include "table" as the parent.

   This now works:

   $table[0].parentElement.querySelectorAll('table > thead > tr, table > tbody > tr, table > tr');
   

   However, I don't know how safe this is — there could be an unrelated sibling table that is not the table drag table. Is this something to worry about? Or is there a better way to code this?

2. The next step would be to replace the jQuery each iterator, with an ES6 iterator that supports IE11.
   I tried restoring "this" but now I'm getting a different error

   const rows = $table[0]
     .parentElement
     .querySelectorAll('table > thead > tr, table > tbody > tr, table > tr');
   Array.prototype.forEach.call(rows, function(row, i) {
     this.addColspanClass(columnIndex);
   });
   

   TypeError: this.addColspanClass is not a function
   

   So there is still a scope issue with using this inside the forEach.call() callback. The compile down to ES5 is not switching "this" to "_this2" (which would be the appropriate one for the parent context), so the only way I can see to do that is to use self = this here. However, when I do that, the table row weights are not properly hidden.

   I've tried all of the following, and none of these work properly; the first, third and fourth producing Uncaught TypeError: [element].addColspanClass is not a function.

   Array.prototype.forEach.call(rows, function(row, i) {
     this.addColspanClass(columnIndex);
   });
   
   const self = this;
   Array.prototype.forEach.call(rows, function(row, i) {
     self.addColspanClass(columnIndex);
   });
   
   Array.prototype.forEach.call(rows, function(row, i) {
     row.addColspanClass(columnIndex);
   });
   
   Array.prototype.forEach.call(rows, function(row, i) {
     $table[0].addColspanClass(columnIndex);
   });
   
   

   There is something else going on here with scoped variables that I don't understand, and have not been able to properly troubleshoot.

To clarify point 2 above with an image, when I use:

initColumns() {
  const self = this;
  // ....
  const rows = $table[0]
    .parentElement
    .querySelectorAll('table > thead > tr, table > tbody > tr, table > tr');
  Array.prototype.forEach.call(rows, function(row, i) {
    self.addColspanClass(columnIndex);
  });

I see the following:

Added issue template and provided "expected" vs "actual" screenshots from CD_Tools.

I'm not sure we should convert the find to not use jQuery. It seems like we're using similar selectors in the same file so it would probably make sense to change them all at once. This way we don't have to worry about side effects of that change in this issue.

This removes the self, while keeping the selector consistent by continuing to use jQuery.

Not sure what went wrong with generating patch in #103 🤷‍♂️

The unnecessary self additions spotted in #88 have been addressed. I've reviewed this several times and that was the last item I'm aware of that needed to be addressed. I applied the patch for one final manual review and everything is working as expected.

+1

Works for me.

It work for me

RTBC + 1

Rerolled #105 😁

The test failure was an unrelated QuickEdit test known to intermittently fail. Switching back to RTBC.

Thanks everyone +1 for RTBC.
Those who looking for 8.9.2 https://www.drupal.org/files/issues/2020-05-28/3092181-87-89x.patch from Comment#87 works fine.

Hi - https://www.drupal.org/files/issues/2020-05-28/3092181-87-89x.patch was working very well for me, but unfortunately isn't effecting the change anymore.

The patch wasn't being applied correctly - all good now.

One thing I just came across due to #3164499: [Paragraphs] Missing "collapse all" / "edit all" button is that the "Collapse all / Edit all" Button is somehow still missing in Claro. Switching back to Seven the button is present, but not in Claro.

Edit: Also the more menu (three dots) is missing as well.

Seven:

Claro:

Researched #116 and it seems unrelated to the issue that is being fixed here. #116 happens even before applying the patch here and seems to be caused by claro_preprocess_field_multiple_value_form overriding some render arrays that Paragraph is setting. Could you by any chance open a new issue for solving that?

@lauriii done #3164979: Paragraphs: Missing "collapse all" / "edit all" buttons 🙂

The latest patch, #111, has coding standard errors, https://www.drupal.org/pift-ci-job/1793545. I think those need to be fixed.

And let's update the issue summary to state what the proposed resolution is and update the remaining tasks as well.

Most of the coding standard issues seem to be related to CSSLint. However, Drupal core is not using csslint anymore: https://www.drupal.org/node/2868114. We should probably prioritize #2876298: [12.x] Remove scaffolded csslint and eslint config given that it causes some confusion. Or maybe there's a way to disable CSSLint from Drupal core?

Rerolled and updated issue summary.

This was previously RTBC and only needed a reroll so I'm switching back to that.

@lauriii, thanks for the comments in #123. I am now less confused :-)

So.... at the end of all this we seem to have a community-tested fix, which is awesome.

What's the process for advancing this further?
Personally I feel this is an important fix for a big issue.

Can anyone enlighten us on whether this fix is slanted for inclusion in core 8.9.x/9.x and by when?

Just to confirm that today (12 Sep 2020) the patch attached to comment #87 still works for Drupal 8.9.x.

If you have installed drupal from git then you can apply the patch like so:

  cd [your Drupal root, where /core and index.php live]
  curl https://www.drupal.org/files/issues/2020-05-28/3092181-87-89x.patch | git apply -v

Alternatively (and perhaps better), use composer and adding the patch to your composer.json file:

{
    "name": "drupal/drupal",
    "description": "Drupal is an open source content management platform powering millions of websites and applications.",
    .....
    ....
    "extra": {
        "patches": {
            "drupal/core": {
                "Nested Paragraphs create multiple drag handles": "https://www.drupal.org/files/issues/2020-05-28/3092181-87-89x.patch"
            }
        }
    }
}

Then:

composer update --lock

+++ b/core/modules/views_ui/js/views-admin.es6.js
@@ -1341,7 +1341,7 @@
+        .on(' click', function(event) {

+++ b/core/modules/views_ui/js/views-admin.js
@@ -594,7 +594,7 @@
+      $context.find('a.display-remove-link').once('display').on(' click', function (event) {

why is this changed from `click` to ` click` because I can't see any reference to that in the discussion above - and this is in the views module, not claro

Addresses #129

Also want to mention that this issue would also be solved with #3083051: Refactor tabledrag when core issues are resolved the bug reported here is happening with code that was planned for removal once a few prerequisite issues were fixed.

Needs reroll. Patch does not apply any more.

@bnjmnm Thanks for your work. Re-roll the patch.

#87 still applies cleanly to 8.9.7, but no longer fixes the problem

#132 does not apply anymore on the 9.1.x or 9.2.x branches.
Reroll is not straightforward (there are conflicts on tabledrag.pcss.css and tabledrag.es6.js)

Working on reroll for D9.2.x. will update soon.

Reroll for 9.2.x

Is this issue still relevant since this bug is also fixed once #3083051: Refactor tabledrag when core issues are resolved is committed?

I would assume this issues is still relevant since that bug is only relevant to D9. This multi-handle problem also impacts Claro in D8.

If the other ticket only solves the issue for D9 then we either need to backport the solution of the other ticket for D8. Otherwise this issue is still very relevant, as the problem described here makes Paragraphs almost unusable with Claro in D8.

Rerolls for 8 and 9

Patches don't work for me in D9.0.10, can patch will be apply because it doesn't install ?

@Nicolas S. The patch applies to Drupal 9.1, so you could upgrade to that version (released last week) or you can roll a 9.0 version of the patch for your own use.

I made a reroll for 9.0 that some might find useful.

Patch #145 works with drupal 9.0.10

let's stay on track. The issue is about 9.2.x

#142 Works well for me on 8.9.11

Without patch

With patch

Confirmed #142 works on Drupal 9.1.0

It's very close, just a few details. And also a good example of why I want to do #3083051: Refactor tabledrag when core issues are resolved sooner rather than later because this patch is also fixing some obscure but bad problems with the 'basic' core tabledrag:)

1. +++ b/core/modules/system/tests/modules/tabledrag_test/src/Form/NestedTableDragTestForm.php
   @@ -0,0 +1,36 @@
   +    $form['table'] = $this->buildTestTable($parent_rows, 'tabledrag-test-parent-table', 'tabledrag-test-nested-parent', FALSE);
   

   here the 'parent' table is not 'indentable' we can just order the elements, if it were 'indentable' it would show a bug in the core tabledrag (not the claro one). Where when you change the parent element indentation, it changes the children' and messes with the indentation of the 'inner' tabledrag. Need a follow-up so we can the rest of core.

2. +++ b/core/modules/system/tests/modules/tabledrag_test/src/Form/NestedTableDragTestForm.php
   @@ -0,0 +1,36 @@
   +    $form['table'][reset($parent_row_ids)]['title'] = $this->buildTestTable() + ['#caption' => $this->t('Nested table')];
   

   Here the 'inner' table has no initial 'indentation', if there was an initial indentation, it would show another core bug where the first 'deepest' item of the 'inner' table get 2 drag handles, and the parent row seems like it doesn't have a drag handle (because the parent drag handle is added to the first 'deepest' item of the inner table). Need a follow up to fix this in core.

   The 2 points are solved by the current patch so claro won't show those problems after this patch is in.

3. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -232,19 +232,26 @@
   +        .appendTo(self.table);
   

   this.table or just table, let's not add more use of 'self' in the code.

4. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -337,9 +344,11 @@
   +          const rows = $table.find('> thead > tr, > tbody > tr, > tr');
   +          Array.prototype.forEach.call(rows, (row) => {
   +            this.addColspanClass(row, columnIndex);
   +          });
   

   This is for init, no fancy selector, I would try querySelectorAll here, it's just for init.

5. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -350,35 +359,34 @@
   +     * @param {HTMLElement} row
   +     *   The row HTML element which columns to add colspan class.
   ...
         * @return {function}
         *   Function to add colspan class.
   ...
   +    addColspanClass(row, columnIndex) {
   

   This is changing the method signature of function, and it's changing the first parameter that might already be used. It would be best to not change the signature, or at least add the parameter after the existing one, otherwise there is a 100% change of breaking code using this somehow.

   Also the return type is different, might want to keep that in line with the 'basic' tabledrag script. I know returning a function is not useful, but we got to keep method signatures stable.

6. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -350,35 +359,34 @@
   +      const cells = $row.children();
   +      cells.each((n, cell) => {
   

   should be $cells instead of cells

7. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -541,33 +549,43 @@
   +        .children('td:first-of-type')
   ...
   +      const $targetElem = $firstCell.children('.js-tabledrag-cell-content')
   ...
   +        ? $firstCell.children('.js-tabledrag-cell-content')
   ...
   +        .children('.js-indentation')
   ...
   +      const $indentationLast = $targetElem.children('.js-indentation').eq(-1);
   ...
   +          $targetElem.children('.js-indentation').length,
   
   @@ -653,7 +671,9 @@
   +                    '> td > .js-tabledrag-cell-content > .js-indentation',
   
   @@ -1158,7 +1184,9 @@
   +            '> td > .js-tabledrag-cell-content > .js-indentation',
   
   @@ -1207,7 +1235,9 @@
   +                '> td > .js-tabledrag-cell-content > .js-indentation',
   
   @@ -1400,24 +1430,34 @@
   +        '> td > .js-tabledrag-cell-content > .js-tabledrag-handle',
   ...
   +        '> td > .js-tabledrag-cell-content > .js-indentation',
   ...
   +          '> td > .js-tabledrag-cell-content > .js-indentation',
   ...
   +              '> td > .js-tabledrag-cell-content > .js-indentation',
   
   @@ -1454,18 +1494,23 @@
   +          nextRow.find('> td > .js-tabledrag-cell-content > .js-indentation')
   ...
   +              .find('> td > .js-tabledrag-cell-content > .js-indentation')
   
   @@ -1557,7 +1602,10 @@
   +        ? $(nextRow).find('> td > .js-tabledrag-cell-content > .js-indentation')
   
   @@ -1573,8 +1621,8 @@
   +          $prevRow.find('> td > .js-tabledrag-cell-content > .js-indentation')
   
   @@ -1616,11 +1664,15 @@
   +              '> td > .js-tabledrag-cell-content > .js-indentation:first-of-type',
   ...
   +            .find('> td > .js-tabledrag-cell-content')
   
   @@ -1660,7 +1712,9 @@
   +                '> td > .js-tabledrag-cell-content > .js-indentation',
   

   This is what fixes issues 1. and 2., nice one :)

8. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -870,7 +891,7 @@
   +          const currentRow = self.findDropTargetRow();
   
   @@ -1017,62 +1040,65 @@
   -     * @param {number} x
   -     *   The x coordinate of the mouse on the page (not the screen).
   -     * @param {number} y
   -     *   The y coordinate of the mouse on the page (not the screen).
   ...
   +    findDropTargetRow() {
   

   same here, method signature change to reverse (even if you don't use the parameters, need to update the doc saying the parameters are useless in case someone tries to change them).

   Also need some more documentation on what changes, and should core update it's code to follow this? It's not obvious what is the difference.

9. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -1017,62 +1040,65 @@
   +      const directionUp = this.lastDragSwitchPointerY >= pointerY;
   ...
   +            this.lastDragSwitchPointerY = pointerY;
   

   this is a new property of the object, need to be declared/documented in the constructor.

I agree with @nod_ that #3083051: Refactor tabledrag when core issues are resolved should be prioritized over this as it would fix the issue by removing technical debt instead of just remixing it. If it weren't for the fact that this fix should get into 8.9, I'd close this issue as a duplicate.

While I'm not a fan of how this issue is getting more attention than my preferred #3083051: Refactor tabledrag when core issues are resolved I took care of #142 as that was a very complex reroll that was a little easier for me due to my familiarity with the changes that necessitated the reroll. Perhaps someone else can take care of the feedback in #151. It shouldn't be that difficult to address, so perhaps one of the many folks monitoring this issue to have a crack at it? Anyone motivated but stuck on (the objectively confusing) Drupal process stuff is welcome to ping me on Slack 🙂

Are there changes that need to land as part of this change now that #3083051: Refactor tabledrag when core issues are resolved has been committed?

Seems like based on #152 this could be closed. As a normal bug, this is not eligible for backport to 8.9.x anymore.

I gave credit for contributors that should have received credit on this issue here: #3083051: Refactor tabledrag when core issues are resolved.

Oh! Thanks, that's why I didn't understand why I have been credited on an issue I've never been onto. ;) Well, this is too late, but I think me getting credit is a little too much in regards of what I have done... ^^' Good job anyone, though!

Confirmed #142 works on Drupal 8.9.13. Thanks!

- Applying patches for drupal/paragraphs
    https://www.drupal.org/files/issues/2020-12-08/3092181-142-8X_0.patch (Nested Paragraphs create multiple drag handles)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-12-08/3092181-142-8X_0.patch

Any suggestions?

drupal 8.9.13
paragraphs 1.12.0

Thank you

@bond708 you must apply the patch on drupal/core, not drupal/paragraphs.

@Dakwamine true, but the patch doesn't apply for me against 8.2.x either.

@Lukas von Blarer the latest 8.x patch will apply to 8.9.x, you need to update.

For anyone using this patch in Drupal 8, please be aware that this patch will never make it to core. This issue was fixed in Drupal 9 and will not be backported to Drupal 8. What I would recommend is to use Gin in Drupal 8 where they have fixed the issue properly.

Since it sounds like many people facing this issue are still on Drupal 8, I want to mention that upgrading from 8 to 9 is much easier than any prior upgrades. No data migration is required, only code changes that can be made incrementally.

You'll first need to be on Drupal 8.8+. Use the upgrade status module to identify what needs to be changed in order for a site to be ready for D9 https://www.drupal.org/project/upgrade_status/. It will tell you what changes need to be made, and none of the changes will break a D8 site, it will just become D9 ready.
Once those changes are made, update core https://www.drupal.org/docs/upgrading-drupal/upgrading-from-drupal-8-to-...

Since this issue is Claro-specific, I should also mention that Claro has many improvements only available in 9. I encourage everyone facing this issue to give the 8 -> 9 upgrade a shot.

Thanks!
#142 worked
Drupal 9.1.5

#142 patch is not applying for
Drupal 9.2.1 . It worked till Drupal 9.1.4. Any help

Thanks in advance !

I believe you don't need this patch in 9.2.x because the issue is resolved for 9.2.x in #3083051: Refactor tabledrag when core issues are resolved

*comment removed*

the issue still exists, so follow-up is #3112245: Cross for multiple entity reference field in paragraph

maybe it needs another one for paragraphs