Incorrect targetElement.className value when dragging table rows between regions

I confirmed that .js-indentations is not in use anymore. There is code that is no longer necessary as a result, and this patch correctly removes it.

This seems only to be patching Claro. What about core/misc/tabledrag.es6.js? Is that deliberately excluded from the scope?

Was a bit of a rabbit hole, long story short: this is not dead code, it's code broken in several places. Attached patch seem to work as this whole thing was intended.

Turns out this code has never worked as intended because since the first commit #181066: Drag and drop of table rows (e.g. block admin) of the tabledrag there is an extra s after indentation in the class name and I could not find any reference to it anywhere. This class was never generated and never used. On top of that the refacto and subsequent patch broke things again subtly #2489826: tabledrag is broken.

It doesn't seem to affect anything, and is only triggered when using the mouse. since we save the page quickly after moving things I guess it was never an issue. Also the the combinaison of rowSettings.relationship === "siblings" and rowSettings.action !== "order" which could be problematic doesn't seem to happen anywhere I tested, so the problematic ifs and classnames have no consequences on important things.

You can test on the manage block or manage display screen and dragging a row in between the different regions, using the debugger look into the Drupal.tableDrag.prototype.copyDragClasses function and check the classes being changed, when changing region, the wrong class is added to the dragged row.

With the attached patch things go as expected.

I'm trying to test this based on #7:

You can test on the manage block or manage display screen and dragging a row in between the different regions, using the debugger look into the Drupal.tableDrag.prototype.copyDragClasses function and check the classes being changed, when changing region, the wrong class is added to the dragged row.

Both with and without the patch, I checked:

targetElement[0].className = sourceElement[0].className;

when dragging a row on the Article Manage Display page page between regions.

I see the classes are set to:

field-weight form-text

and then:

js-field-parent field-parent form-select

But, the fix in the patch is related to changing js-indentations to js-indentation.

Any idea what I'm missing?

Screenshots:

Sorry Kristen Pol, a bit late but here it is:

On the block admin page (i'm using umami), when moving a row from one region (header) to another (pre-header) and we look at the source and target classes we can see:

sourceElement[0].className: "block-region-select block-region-header form-select required"
targetElement[0].className: "block-region-select block-region-header form-select required"

The source element (source == what we copy FROM) has a class of block-region-header which is not correct, we just moved something to the pre-header region.

With the patch the result is:

sourceElement[0].className: "block-region-select block-region-pre_header form-select required"
targetElement[0].className: "block-region-select block-region-header form-select required"

we selected the proper row to copy classes from.

The code tries to figure out the closest sibling and copy the classes. Siblings can be because of a "group" (like the theme regions) but it can also mean the same depth level, which is where we need the indentation thing to be working as expected.

@nod_ Thanks for the details. I used the block layout page and dragged a block from Primary menu to Header.

After this code ran in Drupal.tableDrag.prototype.copyDragClasses:

    var sourceElement = $(sourceRow).find(".".concat(group));
    var targetElement = $(targetRow).find(".".concat(group));

The classes were:

• sourceElement.className is block-region-select block-region-header form-select required"
• targetElement.className is block-region-select block-region-primary_menu form-select required"

and after this code ran:

    if (sourceElement.length && targetElement.length) {
      targetElement[0].className = sourceElement[0].className;
    }

The classes were:

• sourceElement.className is block-region-select block-region-header form-select required"
• targetElement.className is block-region-select block-region-header form-select required"

This is inline with your example above.

I made an attempt at updating the issue summary though I'm not sure it's clear.

Assuming the issue summary is ok (or someone can fix it better than me), this seems RTBC to me. The changes are straightforward and it passes manual testing and automated testing.

Adding tag as I worked on this yesterday for the Bug Smash Initiative.

Given the subtle-ness of this bug this looks worth adding an automated test for.