Prevent line breaks in draggable (first) table cells

Here's a first patch. This addresses the issue and indirectly addresses #197641: Drag and drop is not RTL aware as well, due to some RTL logic changes needed in this scope.

Note that Claro will be addressed in #3083051: Refactor tabledrag when core issues are resolved

Needed a reroll

I'm wondering how should we communicate the hierarchy after the table runs out of space. The behavior of the current patch seems confusing since it's pushing the items to left which makes those items look like they are not a child of their parent. I attached a gif to demonstrate this.

It seems like in Claro #6 is solved so that the table doesn't shrink. This could work in combination with #3068696: Tables overflow on mobile but I'm not sure how to solve this for Seven. Maybe we could implement this based on how Claro has implemented this, and open a follow-up to improve this in Seven.

Good catch in #7. The patch was based on Claro's approach, but it looks like the indent elements in Seven aren't consistently adhering to the width: 20px; style. Setting min-width: 20px; solves this.

1. +++ b/core/misc/tabledrag.es6.js
   @@ -207,14 +207,15 @@
   +        '<tr><td><div class="tabledrag-cell-content"></div></td></tr>',
   

   Shouldn't we generate the test row using the theme function?

2. +++ b/core/misc/tabledrag.es6.js
   @@ -207,14 +207,15 @@
   +        .find('.tabledrag-cell-content')
   
   @@ -1655,7 +1663,9 @@
        const marker = Drupal.theme('tableDragChangedMarker');
   

   Wouldn't it be more consistent to use data-drupal-selector here too?

3. +++ b/core/misc/tabledrag.es6.js
   @@ -502,6 +504,10 @@
   +    $item
   +      .find('td:first-of-type')
   +      .wrapInner($(Drupal.theme('tableDragCellItemsWrapper')));
   

   I think this change (including the CSS rules attached to the element) is a bit risky because this could break custom styles or JavaScript. I'm not sure how to solve this without risk of having regressions on some sites.

I swear I'd already tried this approach before, but it seems to be working with a patch half the size of the previous one.... None of the changes reviewed in #10 are there as they're not needed with this approach. Tested in IE, RTL, nested tables and it all seems fine?
The BC concerns mentioned in #10.3 is how Claro currently addresses the line break issue. If the solution here works, we'll be able to eliminate the BC risks in #3083051: Refactor tabledrag when core issues are resolved. 🤞

The test failure in #12 was because the location of the "changed" marker is changed in this patch. Having it appear between the tabledrag handle and label makes it possible to consistently control the width between all the elements. In its prior position, its distance from the label varies depending on the amount of text wrapping within the label.

While addressing this, it occurred to me that the approach as-is could cause problems for themes extending Stable/Stable9, as their CSS would not necessarily be equipped for the JavaScript related DOM changes. To get around this, the JS adds new (BEM friendly) selectors that receive the styling related to this fix. Because these selectors are new, they can be added to the Stable themes since they aren't altering any existing styles.

The classes added in #13 broke some xpath selectors in some tests. This should take care of that.

Tested this on Seven and seems like this doesn't work as well as #8 did. The last item is not displayed at current indentation and when dragging rows the handle disappears.

Re #16

The last item is not displayed at current indentation

. Looks like this is due to the test indents being added to separate lines. Once I updated the <td> holding the indents to display:flex, this fixed it.

when dragging rows the handle disappears

Looks like this is a pre-existing problem when .touchevents is present. Since the .drag state changes the background position of the handle, I suspect this image used to be a sprite, and when it was changed to svg, the .touchevents use case was not addressed. I chose to address here as it's needed to for manual testing, and I the benefits of a non-vanishing drag handle need to be delayed. Specific concerns about the implementation of the fix can be addressed in a followup if need be.

Test fail was unrelated and in a test known to intermittently fail. Back to NR

The last row is still rendered so that its depth could be confusing 😓

@huzooka problem with the Claro approach is that it requires a BC breaking markup change. We've tried to find approach that would be less disruptive.

Fixes #20 by bringing back a min-width that was unintentionally removed during the trial and error to find a BC friendly solution.

Tested this manually and the deeply nested tabledrags were working as expected at least on Chrome 👌Would be good idea to confirm with other browsers too before committing this.

1. +++ b/core/misc/tabledrag.es6.js
   @@ -477,11 +478,12 @@
        // Add a class to the title link.
        $item
          .find('td:first-of-type')
   +      .addClass('draggable__first-cell')
          .find('a')
          .addClass('menu-item__link');
   

   I think we should try to find a way to get rid of this in a follow-up.

2. +++ b/core/misc/tabledrag.es6.js
   @@ -1655,7 +1657,7 @@
   -      cell.append(marker);
   +      cell.find('[data-drupal-draggable-handle-wrapper]').after(marker);
   
   @@ -1690,7 +1692,7 @@
   -        return `<abbr class="warning tabledrag-changed" title="${Drupal.t(
   +        return `<abbr class="warning tabledrag-changed tabledrag-changed--before-label" title="${Drupal.t(
   

   Aren't these changes out of scope?

3. +++ b/core/misc/tabledrag.es6.js
   @@ -1700,7 +1702,7 @@
   -        return '<div class="js-indentation indentation">&nbsp;</div>';
   +        return '<div class="js-indentation indentation draggable__indentation">&nbsp;</div>';
   

   I'm +1 for using more specific class names in general. Wondering why do we need draggable__indentation in the scope of this issue?

4. +++ b/core/modules/system/css/components/tabledrag.module.css
   @@ -47,6 +50,9 @@ a.tabledrag-handle:hover .handle,
   +  min-width: calc(2em + 14px);
   

   Where does 2em + 14px come from?

5. +++ b/core/themes/stable9/css/system/components/tabledrag.module.css
   @@ -20,6 +20,9 @@ tr.region-populated {
   +.draggable__first-cell {
   +  display: flex;
   +}
   

   This seems potentially something that could have an impact on some themes. Let's at least write a release note to let themes know they should specifically test table drags when they update.

#24.1 & 3: .draggable__first-cell and .draggable__indentation were added so Stable/Stable9 could get these new styles without breaking the Stable promise, especially since there was an opportunity to use more specific classnames.

#24.2 I felt these changes were needed as the changed marker can't be consistently positioned when it's on the wrapping-side of the drag label. This approach was implemented in Claro to address the same symptom.

#24.4 Added comment to explain the 2em + 14px

#24.5 Updated the existing CR since its contents are no longer applicable since we shifted to the BC friendly approach.

Uploading screenshots here as D.O. got sick of me uploading in #25

As the screenshot may indicate, I opted for a graceful degradation with IE11. If the table cell is set to display: flex; the table becomes less accommodating with narrow viewports. As it turns out, the bug this issue addresses does not occur in HEAD with IE11 anyway, so there's no regression.

Re #24.1 & 3 The approach you proposed is nice because it provides a safer way to include these changes in Stable. However, I think it's fine to fix this only in system module so we don't risk having regressions on sites extending Stable. This could be something like .draggable td:first-of-type { display: flex; } in system module.

Re #24.2 Let's open a follow-up for figuring out what to do with the changed marker. I think for now we shouldn't change it because a) I think it could use bigger design change than just the position of the asterisk b) it seems like this would only happen in the same edge cases where tabledrags would have rendered awkwardly before this change meaning that it's not a regression.

I'm fully convinced by the rationale of both points in #27.

The approach I previously had for adding the changes was only needed when the JavaScript altered the DOM structure. Now that I've found an approach that doesn't require DOM changes, the fix can be in the system module CSS, which is already fenced away by Stable. This eliminates the need for almost all of the JS changes, too.

With the changed marker, I also suspected this was out of scope but wasn't sure if it would be perceived as a regression. It was pointed out that the only behavior that would cause the variable change marker distance is :
1) Unlikely to happen in themes other than Claro (which has its own fix for this anyway)
2) Actually existing behavior, it's just masked by the much worse symptoms addressed by this patch.
I've created a followup for that #3166623: [PP-1] Make tabledrag changed marker consistiently postioned from it's label

Seems like there are some unrelated changes in the patch 🤔

Wrong patch in #28

This is looking great in Firefox & Safari as well:

Will test on Edge/IE tomorrow.

A final test in Edge & IE shows no issues/bugs:

Yay! Thank you for fixing this 🎉

Oops, Claro already has a fix for this 🙈

Tested with Seven on all browsers, only IE seems to be missing the display: flex; rule which makes the term hierarchy practically invisible 😕 Adding the rule to the <td> tags aligns the indentation divs nicely so I guess that's the only thing left to fix?

Tested RTL as well (thanks for the reminder @laurii), didn't notice any issues (apart from the one shown above).

Good catch in #33 @Antoniya!. It is intentional to not provide the improvements in this patch due to IE11, but the IE11 experience should not be worsened as a result. This patch should address that.

IE11 has a hard time dealing with Flex items inside a table. If the table row has display: flex the label won't wrap at all, which is still beneficial as it helps with the more common occurrence of labels wrapping to a second line, eliminating the need to horizontally scroll to access all parts of the table. A third line is less likely

There is a workaround to make these IE flex items in a table wrappable by explicity setting a child's flex-basis, but this prevents the table from properly calculating column widths

Because of this, my approach is to not provide this fix in IE11, but to also be sure the experience isn't made worse. Between the usage statistics for IE11 and this being a somewhat uncommon scenario, I think this is a good candidate for graceful degradation.

@bnjmnm I understand, flexbox in IE is a nightmare lol

I tested the latest patch in #35 and I can confirm that it's a nice improvement to all major browsers and it also introduces no regressions in IE AFAICS (hierarchy is clearly visible):

Thank you! 👏

Marking as reviewed even though it would be nice if someone else also tested manually.

Tested manually with Chrome, Firefox, Safari, Edge, IE 11, Chrome for Android and Safari for iOS and the behaviour was as expected 👌I don't think we can add test coverage for this since this is only a visual change.

Committed c2bcd73 and pushed to 9.1.x. Thanks!

Opened #3169872: Table cells not aligned properly vertically which seems to be caused by this.

Reverted this because of the regressions documented in #3169872: Table cells not aligned properly vertically.

What needs to be done here? Can we get an IS update to identify the next steps? Adding tag.

I have created a patch for same, also handled table cells alignment. please review

Fixed the failures of patch #50