Drag and drop is not RTL aware

I did think about RTL support when building drag and drop, but I found that floating items left or right will only give us a small amount of benifit. For full RTL support, wouldn't we need to write all our tables so the columns could be in reverse order? Floating the handles right instead of left while still remaining in the left-most column seems like it would do more harm than good, as the handles would be located in the middle of the table. Is there a solution for tables in the rest of Drupal core that I could base a fix on?

Oh fancy! That makes everything much more feasible. I'll whip up something to swap the keys and mouse directions.

I have javascript that will successfully switch the direction (just multiple by -1), but I can't find any property in the page source that would indicate RTL languages, other than the inclusion of -rtl css files. Is there an existing method for this or should we add one to the body tag?

Here's a patch that adds the RTL direction as part of drupal_add_tabledrag(). It's not incredibly ideal, but it would allow you to have a right-aligned table and a left-aligned table on the same page (however unlikely). If there's a more central place to get RTL I'd prefer just making it one or the other per-page, but this method certainly will work and changes none of the parameters of our current functions.

Oops. That last patch reversed LTR languages drag and drop :)

This one works both ways.

Sorry Gábor I didn't read the "CSS" mention and was looking for something in the HTML source. The direction = 'rtl' in the CSS code is plenty enough. This patch checks for the direction == 'rtl' and if so flips the logic. Otherwise it assumes LTR.

Superb! Thanks!

Would you mind if we checked the directionality of the table instead of the body's? I've attached this 'fix' as a little patch. I know that this doesn't make a _lot_ of sense right now to have LTR tables on RTL pages --because we don't support both rtl and ltr CSS on the same page-- but it's nice nevertheless.

Note that the css() method gives us _computed_ styles, so we don't have to have a 'direction: rtl/ltr' directive on the tables themselves: it's inherited from an ancestor container (such as BODY).

The patch includes three other tiny fixes:

1. Made the title of the handle translatable.
2. Fixed something that looked like a bug ("if ($('...', context)").
3. Changed "$Id $" to "$Id$".

(And sorry for piggybacking on this issue!)

Here's the patch again. (I know I attached it. It's probably a bug here.)

Using .attr('title', Drupal.t('Drag to re-order')) to to set the title isn't really necessary, I probably would've just included the translated text inline as part of the <a> tag, but that's just a matter of style. This approach is more readable, which is probably the reason for doing it this way.

This looks great, the $Id$ tags I'd been meaning to fix with every patch but it kept slipping away. I tested it in all browsers (IE6/7, Opera, Safari, Firefox) just to ensure that the computed css was being retrieved properly. Works like a charm!

Thanks for giving the computed css an extra check.

Using .attr('title', ...) to to set the title isn't really necessary, I probably would've just included the translated text inline as part of the <a> tag

Although uncommon, translated strings may contain quotes and apostrophes, so it's a Good Thing to ensure they are escaped. attr() relieves us from this task. Unlike in English, these two characters aren't used just for quoting. Here's what happened for somebody who didn't escape his JS strings...

Thanks for that tip mooffie! I'll keep that in mind for future work. :D

Gábor, prompt as always. Thanks!

I opened this issue #374652: Nesting of sublists in RTL is broken ... it seems this is not fixed in Drupal 6.

What's wrong? It seems the code does support this.

Ok. Looked at it again (around line 412):

-      var indentDiff = Math.round(xDiff / self.indentAmount * self.rtl);
+      var indentDiff = Math.round(xDiff / self.indentAmount);

This works on my setup (FF 3, Opera 9.63 on Linux/64). However I got another report on IRC that this does not work.

As mentioned above - it would be smarter to check the direction of the element instead of the body: what happens when I force an LTR table on an RTL window? Trust me, this will happen! ;-)

Credits to this "patch" go to yhager who looked at the code and found a possible bug.

I have to reopen this, as this does not work for me.

The code in tabledrag.js computes the indentation amount in:

    this.indentAmount = $('.indentation', testCell).get(1).offsetLeft - $('.indentation', testCell).get(0).offsetLeft;

Then, it measures the amount of mouse movement, and divides by indentAmount, but then multiplied by self.rtl:

      var indentDiff = Math.round(xDiff / self.indentAmount * self.rtl);

The multiplication is wrong, since indentAmount calculation already takes RTL in consideration, so the patch attached simply removed this multiplication.

The only problem I was having was that I had to drag in the same direction as when in a ltr language, so in a rtl language it would be the opposite to the direction I would want to move it on screen.
I don't know if this would be correct or not, but the only thing that worked for me was to change this.rtl to 1, regardless of direction.
When I first tried it, I thought I had to be wrong, but it worked.
I tested this on the Primary links admin page in English and Arabic using D 6.17 in Opera and Firefox.

From:

  this.rtl = $(this.table).css('direction') == 'rtl' ? -1 : 1; // Direction of the table.

To:

  this.rtl = 1;

All the other problems described in the original post seem to be resolved.

Tagging

Converted #25 to git patch, I think it's simple & RTBC.

Wondering why no one responded to this!
I applied patch #30 manually on Drupal 7.32 and it fixed the problem.
Haven't tried it on Drupal 6 since i'm not using it for several years now
Check on both rtl and ltr and works well
Tx Gábor

Rerolled the patch.
Tested, the issue still exists in D8 and the patch fixes it. This has also been reported as working in #24, #25, and #31, so marking as RTBC.

Committed 862d51f and pushed to 8.0.x. Thanks!

Rerolled for D7.

Does this work for tree tables as well? like terms

Testbot fluke.

I tried this out on the Menu administration pages to see how it worked, and yeah, it makes things way way better :)

Committed to 7.x - thanks!

I'm using D7 v7.32 and I had the same problem in RTL mode. I used patch #37 and it worked, but only when we have 1 indentation. If We have more level of indentation than one, it bounces middle levels.

Patch #37 changes one line(500):

      -        var indentDiff = Math.round(xDiff / self.indentAmount * self.rtl);
      +        var indentDiff = Math.round(xDiff / self.indentAmount);

I changed another line(505) too:

      -        self.dragObject.indentMousePos.x += self.indentAmount * indentChange * self.rtl;
      +       self.dragObject.indentMousePos.x += self.indentAmount * indentChange;

It solved the problem. I checked it in ltr mode and there is no problem as far as I know.

Marking as active so I don't miss this, but anyone feel free to post a new patch.
Also, the mentioned line exists in Drupal 8 too, so going back to 8.x

Re @mohsen81666's comment #52:

I'm really happy that I've found and seen your remark!

The change you're suggesting (still) fixes the issue of the hierarchy rearrangement for RTL languages.

Exactly what is still needed here? Perhaps someone can update the issue summary.

Added reroll of patch #67 for Drupal 9.1.x.

This was fixed by #3083044: Prevent line breaks in draggable (first) table cells. Since this issue has a patch committed earlier, marking as fixed.