TableDrag JS :first-of-type issues

This issue may be related.

Came here from #3085858: Drag and drop acts weird, sometimes not resetting the parent, or even clearing the region value and the patch solved this issue for us. Thanks!

If this issue: #3083051: Refactor tabledrag when core issues are resolved lands, all the Claro changes here can probably be removed. This would make the patch for this issue smaller, and thus easier to review/commit.

Here's a try to reroll #17 against 9.2.x (claro tests removed, as advised in #24.

Whoops

Thanks for the great work! The patch has tests and fixes the related problems with the Field Group module #3085858: Drag and drop acts weird, sometimes not resetting the parent, or even clearing the region value

This can't be set to RTBC because the most recent patch (#28) is not passing code standard/formatting tests (hence the "custom commands failed" status of the patch). Looks like it is just this

Checking core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragSubgroupTest.php


FILE: ...al/FunctionalJavascriptTests/TableDrag/TableDragSubgroupTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 228 | ERROR | Parameter $group is not described in comment
----------------------------------------------------------------------

Time: 155ms; Memory: 6MB

This isn't a deep review, but in a quick readthrough I spotted a few things that need attention before this is RTBC worthy, but I think this should probably get a deeper review overall:

1. +++ b/core/modules/system/tests/modules/tabledrag_test/src/Form/TableDragSubgroupTestForm.php
   @@ -0,0 +1,228 @@
   + * If using this as a model for development, note that if you completely empty a
   + * group, it will no longer set the group value correctly if you then try to add
   + * items again (it sets the value based on a sibling). The core block list
   + * builder uses a custom tabledrag.onDrop to set the correct region.
   

   This seems like quite a bit of documentation for an unlikely + unadvisiable scenario. I think this probably adds more noise than it helps. (though I typically prefer erring in the direction of more docs)

2. +++ b/core/modules/system/tests/modules/tabledrag_test/src/Form/TableDragSubgroupTestForm.php
   @@ -0,0 +1,228 @@
   +      // Set defaults
   

   Needs a period at the end.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragSubgroupTest.php
   @@ -0,0 +1,262 @@
   +   * @param int|null $weight
   +   *   (optional) The expected weight of the row or NULL to not test the weight;
   +   *   defaults to NULL.
   

   Nit: explicitly specifying (optional) here while next to several other variables that are also optional but not labeled the same way. The (optional) shouldn't be needed since there's a default value in the function signature.

I like that there is added test coverage for the edge cases reported in this issue. It would be good if the review confirmed the tests are fully covering the scenarios impacted by these scenarios, and are doing so in the most efficient possible manner.

Reroll of #32 against 9.4.x-dev. No other changes.

Oops. Forgot to build JS in #36... updated patch attached.

Still just a reroll of #32 with no other changes.

I was using the patch from #37 to seemingly fix my issue with not being able to place field groups on entity manage display form. This was with core 9.2.16.

I just upgraded core to 9.3.16 and this is now broken again.

and the patch from #31/32 that is listed for 9.3.x does not apply to 9.3.16.. :(

going back to latest patch and i have been able to get my new field group placed correctly by moving it in small steps at a time between saves. certainly still issues with this.

re-rolling the #32 patch for 9.3.x,9.4.x & 9.5.x

The patches from #37 and #40 are the same.

Here is another re-roll of #32 for 9.5.x.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Reroll of #43 for 10.1.x

This seems like a difficult one to test but from what I can tell it has something to do with parent value.

Tried using the test module provided by the patch. Can it be updated to include parent value as well.

The patch does not seem to apply anymore against core 10.2.

Re-roll of #45 attached for Drupal 10.2.x.

Recommend turning to an MR for quicker reviews.

#51 seems to have failures.

New functions should use type hints and returns.

Also learned we can use constructor promotion.

I can also confirm that the latest merge request applies against 10.2.

Are there any clear instructions how to repeat the issue without the patch? Would make it easier to test that it works.

I originally applied this patch in our project because we had trouble reordering the fields in the node edit form layout but I also had other related patches applied so I would like to confirm I am testing the right thing.

heikkiy:

For me, it happens when I am trying to create a set of vertical tabs. So, I create the "tabs" and then the "tab". I drag the tab up under the tabs, then add the content to the tab. Save. The content stays under the tabs item, but the tab has moved down to disabled.

I've tried using the latest patch, but I get:

a "b" folder created in my /web folder
failures in the patch

See #3333953: Nested field groups improperly display as "Disable" in the admin UI for details of an issue affecting the field_group module - the core patch seems to fix that for me.

@jsimonis - how are you applying the patch, and what are the failures that you see when trying to apply the patch?

https://vazcell.com/blog/how-apply-patch-drupal-9-and-drupal-10-composer is a good explanation of how to do this.

I am seeing the same as @malcomio this patch applies but unfortunately does not fix the issue for me.
I am unable to disable fields using the drag and drop. I need to turn that feature off and select disabled in the dropdown
Drupal 10.4.2
Gin 4.0.2
field_group 3.x-dev

Setting the priority to major, as this seems to cause a lot of trouble downstream for people using Field Groups heavily: #3085858: Drag and drop acts weird, sometimes not resetting the parent, or even clearing the region value

Rerolled and addressed these:

New functions should use type hints and returns.
Also learned we can use constructor promotion.

Something noticed is this could use a summary update. With a clear proposed solution. Changing these selectors think will need manual testing as well.

2x broken functional tests in pipeline.
Test-only test appears good:
https://git.drupalcode.org/issue/drupal-3089151/-/jobs/7086430

Re: #64, applied IS template and re-structured IS, some re-writing for clarity. I updated the 'Remaining tasks' according to #64.

Rerolled. The broken funtional tests were random temporary problems.

Rerolled

Sorry tried using the summary to figure out the problem but not super clear and want to make sure I'm testing the right scenario. Can the steps to reproduce section be added back to the summary please.