Table drag row region doesn't update when Choices.js is applied to select inputs

I need a bit of help with the issue fork and creating the merge request. I'm following the documentation on the Drupal site but I ended up pushing the changes to the 2.x branch and not the 3356663-table-drag-row branch in the issue fork. How should I proceed with the merge request? Thank you for your help as I learn and work through the process for Drupal projects.

@JamesPosko that's no problem, we can also create a MR from 2.x.

For the future, simply click "Show commands" here above, there's a guide with the steps required. I think you missed one, for example the last. Nothing untypical ;)

@Anybody I'm glad to hear this can still work as is with the 2.x branch. Thank you for making the MR from the 2.x branch in this issue fork and for your support.

The MR contains the proposed change to target the parent node with the class .draggable and not apply choices.js to the child select elements. The code comment has been updated as well.

@JamesPosko instead of replacing existing selectors, shouldn't we better simply add .draggable as exclude or can we be 100% sure that the previously existing selectors are no more needed?

I think the exclusion of .draggable should replace the need to search for the other two selectors that are currently being excluded. None of the select elements in the table drag row have the Choices.js applied. The single exclusion of .draggable seems to work when rearranging table drag elements on paragraphs, menus, blocks, roles, and views. It is a good question if another core JS feature might use weight or parent?

I came across another instance where the current ends with selector isn't matching a select. A new attribute selector that contains "weights" would need to be excluded. In the Search API manage processors admin config for processor order uses table drag with weight selects but the attribute uses "weights" instead of "weight" (data-drupal-selector="edit-processors-number-field-boost-weights-preprocess-index").

Thanks @JamesPosko - we won't (be able) to solve this for contrib cases. For that a custom exclusion selector is needed. From my perspective it's a bug / non-Drupal-best-practice value used in Search API. You may open an issue there.

Agreed. There doesn't seem to be a standard way the attribute is being implemented. Though, all these selects are part of the table drag implementation and the new exclude of .draggable should exclude all these selectors.

Weight, parent and region selects are mostly used with the table drag interface. I think the previously existing selectors can be replaced by .draggable.

Okay so I guess there are two tasks left as result:
1. Ensure the MR works for all core draggable cases (best would be to have automated tests, but that might also be disproportionate)
2. Create and implement a follow-up issues to add a custom exclude setting, where selectors like in #8 can be added

@JamesPosko would you do this?

Since changing to the draggable exclusion, table drag is working on core menus, manage form display, manage display, views, roles, and blocks. The update is working on contrib paragraphs and the Search API as well.

I haven't created an automated test but will learn and try.

I can create the follow-up issue feature request to add a custom exclude setting.

Thank you very much @JamesPosko! That's great and highly appreciated :)

I added a test to the global functional tests to check that choices isn't applied to the manage form display table drag selects. I'm not sure why the test for the choices widget failed, all tests are passing on my local development environment with the same versions.

Thanks @JamesPosko, so this needs work to fix the tests. Best would be to have tests for all the expected Drupal Core exclusions.

Ran the tests again and they all passed this time. I'm not sure what happened on the first run but after going through some of the documentation I thought the first step would be to run the test again.

Does the new test added meet the expectations on the core table drag functionality?

What are the other Drupal core exclusions needed?

Anybody → closed merge request !24

Thanks @JamesPosko great!

I think we should add tests for all core pages (matching the different scenarios), where dragging is used.
Some use nested/tree dragging, some not.

I remember the following ones for now:

• Node Fields (solved)
• Menu Items
• Blocks
• Taxonomy Terms
• Roles
• Maybe some core settings pages

I don't thing we have to test all, but we should test the different variants (tree / flat) and the most important, less ege-cases, do you agree?

Once done. could we have a test-only patch here to ensure the test works and broke before? So we can be 100% sure!

I'll take a look at a few of the different core admin pages and add test for the different variants (tree / flat).

Would a test-only patch mean to create a patch that contained the tests that can be applied to the feature branch / merge request?

Thanks @JamesPosko. Exactly, a test-only patch means, only the patch, not the fixes. So it's expected to fail when tested here.

Typically just uploaded as patch file, not as MR. But a separate MR should also work, alternatively.

So remove the tests from MR23 but keep the change to use .draggable, correct?

Create a new branch in this issue fork to apply the tests in a different MR?

No. Keep the current MR as-is (plus additonal tests).

Additionally upload a patch file (or create a MR) that ONLY contains the tests. So they are expected to fail under old conditions, without the changes.

So it's kind of a vice-versa test "would this have failed, before we fixed it, as we would expect". :)

Sounds good! Thank you!

Added a test to run on menu Administration edit page and one to run on views Content filter rearrange UI dialog along with the first test on the manage form display.

There is a new branch that has the new test but not the update to the global JavaScript and the tests fail.

Thanks! We'll take a look! Super awesome :)

@Grevil: Perhaps create a test-only patch file or MR that should fail? If it doesn't take too much time...

I think the MR 25 based on branch 3356663-tests-no-table-drag-row-change in this same issue fork is showing the 3 new test failing. The branch only contains the tests and not the global JavaScript update. Is this how a test-only set should be created?

There were 3 errors: [...]

Nice! Tests LGTM! RTBC +1

Created a new release!

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