Refactor tabledrag when core issues are resolved

I think we can de-scope #3083039: Allow tabledrag to be disabled from the Claro roadmap since it's a new core feature which is not specific to Claro.

In the scope of this issue, we could probably override Drupal.tableDrag.prototype.row.prototype.markChanged instead of solving #3084910: Insert tabledrag changed mark after the drag handle, and not after the last item in the cell. Removing it from the dependencies and unpostponing this.

It seems like Claro also adds some additional wrappers inside tabledrag cells. We might be able to adjust some styles to not depend on those. However, if that doesn't seem feasible, we should open additional issues for adding APIs that would allow Claro to add those wrappers.

Work in progress. This strips out evertying other than the needed overrides.
Still need to address positioning issues in IE11 and confirm there aren't any styles referencing selectors no longer in use.

Another work in progress. This patch includes #3083256: Create smaller variations for form elements, which seems (?) pretty close to completion.

Currently working on getting it to consistently look good in Paragraphs and nested tables with/without table weights being visible. This works fine with Claro's current implementation, but it needs to work without the wrapper div being added, otherwise there's a risk of breaking BC.

Discussed with @ckrina, @saschaeggi and @Gábor Hojtsy and we agreed that we can move forward on this even before #3083044: Prevent line breaks in draggable (first) table cells has been resolved.

Interdiff wouldn't be particularly helpful as the scope and code it's riffing on has changed since the prior patches were posted.

This removes most of Claro's tabledrag code. The theme functions are still there, and a behavior was added to maintain the tabledrag wrapping fix that was previously in an override of the very large makeDraggable function. I did some tests to see if this approach was susceptible to a race condition an it doesn't appear to be, but feedback on that would be great.

\Drupal\FunctionalJavascriptTests\Theme\ClaroEntityDisplayTest::testEntityForm needed to be updated since toggle weights was changed from a link to button (which happens to match core and is the correct element for the functionality).

There was another "show weights" toggle that needed to be changed to a button.

Did a bit of manual testing with #17:

1. 
   For some reason, the tree is not rendered correctly after the third item.
2. 
   Focus ring is rendered behind the table header.

Addresses #18

Discussed with @bnjmnm and we thought it would be good idea to bring test coverage from #3092181: Nested Paragraphs create multiple drag handles here so that they are not lost.

This adds the test coverage from #3092181: Nested Paragraphs create multiple drag handles , which confirms that the errors reported in that issue happen to be fixed here as well.

I haven't reviewed the code in depth, but I'm using patch #22 on Drupal 9.0.7 with nested paragraphs and it's working fine for me!

Not sure if this is related to this patch, but I noticed a weird dash on every record of e.g. Form display:

needs reroll

Rerolled patch #21 for 9.2.x

Rerolled and fixed a few small things.

I'm not seeing the dashes reported in #24, but if there are specific steps to reproduce I'll check those against what I did to get this screenshot.

Test failure was in an unrelated test known to intermittently fail.

The patch in #21 introduces the issue in #24. You can see it by navigating to a menu page like /admin/structure/menu/manage/main when using Claro as admin theme.

With stock core 9.0.9, the markup is:
<a href="#" class="tabledrag-handle js-tabledrag-handle" title="Drag to re-order"></a>

After the patch it is:
<a href="#" class="tabledrag-handle" title="Drag to re-order"><div class="handle">&nbsp;</div></a>

It is the extra <div class="handle">&nbsp;</div> that causes the issue seen in #24

#32 identified where the point of confusion is! This was incredibly helpful. It mentions "With stock core 9.0.9, the markup is:"

This patch builds on changes added in 9.1. It makes sense it wouldn't work right on 9.0. Patches should be tested against the version specified in the "Version" field of the issue metadata. Much of the time it's possible to get away with reviewing patches on earlier versions. However, it won't work with issues such as these that build on changes that won't be added until the next release. Since Drupal 9.1 is now in prerelease, the current dev branch is 9.2. Test against that and let me know if it's working!

This is looking very good :) just need some clarifications, might be a good idea to put a sample html structure of what's the end result of the different functions additionnaly to the comment explaining it in english, might be faster to understand for some (like me :p) like the fact that we want the drag handle inside .tabledrag-cell-content all the time.

1. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -1,1704 +1,72 @@
   +          .first()
   ...
   +          .first()
   ...
   +          .first()
   

   I'd prefer that we replace first() with eq(0), we should limit the use of jquery "helpers".

2. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -1,1704 +1,72 @@
   +        const $targetElem = $firstCell.find('.js-tabledrag-cell-content').length
   +          ? $firstCell.find('.js-tabledrag-cell-content')
   +          : $firstCell.addClass('js-tabledrag-cell-content');
   

   not sure why we need the ternary. wrapInner $targetElem is going to be the td in all case and we just added an element with this class in there. for non-draggable rows this function will not be called so I don't see how we'll be in a situation where we do the addClass.

3. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -1,1704 +1,72 @@
   +          .detach()
   +          .prependTo($targetElem);
   ...
   +          .detach()
   +          .prependTo($targetElem);
   

   Not sure we need the detach, we're not holding on to the element waiting for something it's added back to the DOM straight away, is it for safety?

4. +++ b/core/themes/claro/js/tabledrag.es6.js
   @@ -1713,27 +81,26 @@
   +          $indentToMove.detach().prependTo($cellContent);
   

   Same thing for the detach, does it messes thing up if it's not detached first?

It works and I find the same bugs we already have with the non-claro version, so that's good :)

This hopefully takes care of #34

The detach() calls weren't necessary, that was completely due to mixing this up with another issue worked on the same day 🙂

We have tests, things work as well as before and there is doc explaining why we need the js for claro.

+++ b/core/themes/claro/js/tabledrag.es6.js
@@ -1,1704 +1,115 @@
+        // Move handle into the '.js-tabledrag-cell-content' target.
+        $targetElem
+          .eq(0)
+          .find('> .tabledrag-cell-content__item > .js-tabledrag-handle')
+          .prependTo($targetElem);
...
+        // Move indentations into the '.js-tabledrag-cell-content' target.
+        $targetElem
+          .eq(0)
+          .find('> .tabledrag-cell-content__item >.js-indentation')
+          .prependTo($targetElem);

It's a detail, we can do a single call to find with the two selectors, that would do the same and not duplicate the code. But it works like this, no problem.

Issue could use a title change and issue summary update.

So assuming #3084910: Insert tabledrag changed mark after the drag handle, and not after the last item in the cell and #3083044: Prevent line breaks in draggable (first) table cells eventually land in core, would we remove the remaining overrides then?

@catch if both of those issues landed, we could remove all of the remaining overrides except the theme function overrides.

RE #39

So assuming #3084910: Insert tabledrag changed mark after the drag handle, and not after the last item in the cell and #3083044: Prevent line breaks in draggable (first) table cells eventually land in core, would we remove the remaining overrides then?

Good call, I added @todo items for those. I'd overlooked doing that as BC concerns make those issues very unlikely to happen, but it could happen in 10 or beyond.

I also consolidated the find() call based on #36

Switching back to NR for these changes, so hopefully it's quick for someone to jump in and re-review so this can be available for Claro+Paragraphs users dealing with multiple drag handles.

The interdiff looks good to me.

I credited @alexpott for peer-reviewing this on Slack.

Committed a373aa3 and pushed to 9.2.x. Thanks!

Not backporting to 9.1.x because the patch requires a cache clear.

Crediting folks from #3092181: Nested Paragraphs create multiple drag handles .

Sorry for commenting on a Fixed issue, but I wanted to know if this was going to get backported to Drupal 8? At the moment Paragraphs is pretty much unusable with Claro in D8 unless you use a patch from #3092181: Nested Paragraphs create multiple drag handles .

Does anyone have a patch that applies against latest 9.1.x?

#62

this was going to get backported to Drupal 8

No
#64

Does anyone have a patch that applies against latest 9.1.x?

Adding this to versions earlier than 9.2 isn't feasible as it depends on several other changes only present in 9.2. There is a different issue that has patches that have worked for earlier versions: #3092181: Nested Paragraphs create multiple drag handles . I'm unsure if these patches still work, but they don't depend on the changes specific to 9.2 as they make changes to the buggy code instead of this issues approach: removing the buggy code altogether and letting core take care of it.

Sorry, I didn't test the latest patch in #3092181: Nested Paragraphs create multiple drag handles . This works for me:

https://www.drupal.org/project/drupal/issues/3092181#comment-13931509