While drag and drop support was added, RTL concerns were completely put aside, as Steven Wittens noted. The last few days, I went in and gradully fixed the two CSS bugs I noted:

- http://drupal.org/cvs?commit=90204 - the indentation divs were left floated
- http://drupal.org/cvs?commit=90089 - table drag handlers were left positioned

But there are JavaScript bugs still. If you set your Drupal language to Arabic for example, all is nicely positioned, the drag and drop handles and indents are positioned nicely on the right, as intended. But, as soon as you have a hierarchical list (ie. try with menus, taxonomy or forums), there are two UI problems:

- when dragging an item with children, the lines connecting the items are not properly facing towards the items
- you need to move the item left(!) to make it one level higher, and right to make it one level deeper (while the visuals suggest otherwise)

Note, that you can detect an RTL page by looking for body's direction property to be rtl.

CommentFileSizeAuthor
#37 fix-tabledrag-d7-197641-37.patch733 bytesherom
PASSED: [[SimpleTest]]: [MySQL] 41,384 pass(es). View
#33 197641-33.patch713 bytesherom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,655 pass(es). View
#29 197641-29.patch1.25 KBgood_man
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es). View
#25 yh.patch898 bytesyhager
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch yh_38.patch. View
#16 td.diff2.58 KBmooffie
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch td.diff. View
#12 drupal_rtl_dnd.patch3.63 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_rtl_dnd_1.patch. View
#10 drupal_rtl_dnd.patch4.57 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_rtl_dnd_0.patch. View
#9 drupal_rtl_dnd.patch4.57 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_rtl_dnd.patch. View
#3 dnd-connectors-rtl.patch1.31 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch dnd-connectors-rtl_0.patch. View
#2 dnd-connectors-rtl.patch1.35 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch dnd-connectors-rtl.patch. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Gábor Hojtsy’s picture

Keyboard event handlers are also need their effects swapped.

Gábor Hojtsy’s picture

Status: Active » Needs work
FileSize
1.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch dnd-connectors-rtl.patch. View

Looked into fixing the connector lines, and it seemed from the tree images, that it is indeed built into them, so with a different background position, RTL themes are served.

This is still short of fixing the JS mouse and keyboard events.

Gábor Hojtsy’s picture

Priority: Normal » Critical
FileSize
1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch dnd-connectors-rtl_0.patch. View

Doh, this could be made simpler on the RTL css side.

quicksketch’s picture

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?

Gábor Hojtsy’s picture

quicksketch: Try it! Enable locale module, add Arabic, and switch your site to Arabic. Browsers flip the order of table columns automatically. So you get the enabled checkboxes for modules on the right, now with my fixes, you get the dnd handle on the right, you get the indentation on the right, and with the applied patch above, even the connectors face to left properly. BUT the JavaScript would need to indent items when you move to the left and outdent, when you move to the right. Currently it does this in the other direction. The same goes for keyboard events.

Browsers are intelligent enough to reverse stuff like character order or table column order. They are not intelligent enough to reverse our indenting code in JavaScript ;)

So basically we need the left/right indent/outdent actions swapped when body has a direction=rtl CSS rule. Also for keyboard shortcuts. Otherwise, with the patch above, all seems to work for me right in RTL.

quicksketch’s picture

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

Gábor Hojtsy’s picture

Great. Awaiting your patch soon.

quicksketch’s picture

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?

quicksketch’s picture

Title: Drag and drop is (still) not RTL aware » Drag and drop is not RTL aware
Status: Needs work » Needs review
FileSize
4.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_rtl_dnd.patch. View

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.

quicksketch’s picture

FileSize
4.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_rtl_dnd_0.patch. View

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

This one works both ways.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Hm, as I have indicated two times above, and as visible in the rtl themes, body gets a direction CSS property set to RTL in RTL themes. While it is theoretically possible that LTR tables might end up on the same page, they might receive a direction property or LTR too. I don' think that looking at the language object is the right way to go here. The information about the page being RTL is already in the styles.

People suggested earlier that we should actually add dir="rtl" as a HTML attribute to the body tag, so pages displaying unstyled will still be acceptable with their semantics. If this HTML attribute would make your life easier, feel free to add this to core themes. This can be done with:

- adding a $variables['direction'] = ' dir="'. ($language->direction ? 'rtl' : 'ltr') .'"'; to theme.inc's variable init
- then adding print $direction; to the body tag on core themes.

I think this should give the JS enough info in the HTML code to hook on.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
3.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_rtl_dnd_1.patch. View

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.

Gábor Hojtsy’s picture

Status: Needs review » Fixed

Thanks. I tested this, found it nicely working, so removed the console.log call and committed.

mooffie’s picture

Status: Fixed » Needs review

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!)

Gábor Hojtsy’s picture

Priority: Critical » Normal
Status: Needs review » Active

No patch attached. Not critical.

mooffie’s picture

Status: Active » Needs review
FileSize
2.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch td.diff. View

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

Gábor Hojtsy’s picture

Drupal.t() and the Id changes look good. The other two I am asking quicksketch to comment.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

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!

mooffie’s picture

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...

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks, committed.

quicksketch’s picture

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

Gábor, prompt as always. Thanks!

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

elcuco’s picture

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.

elcuco’s picture

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.

yhager’s picture

Status: Closed (fixed) » Needs review
FileSize
898 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch yh_38.patch. View

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.

NaX’s picture

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.

Status: Needs review » Needs work

The last submitted patch, yh.patch, failed testing.

TR’s picture

Issue tags: +RTL

Tagging

good_man’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es). View

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Can someone else help review this?

sinasalek’s picture

Issue summary: View changes

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

Gábor Hojtsy’s picture

Version: 6.x-dev » 8.0.x-dev
Issue tags: +D8MI, +language-base

Based on our backport process this would need to be fixed in 8.x now and then backported. We want to avoid introducing regressions.

herom’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
713 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,655 pass(es). View

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.

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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

  • alexpott committed 862d51f on 8.0.x
    Issue #197641 followup by herom, good_man, yhager: Fixed Drag and drop...
Gábor Hojtsy’s picture

Yay :) Thanks @herom!

herom’s picture

Status: Patch (to be ported) » Needs review
FileSize
733 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,384 pass(es). View

Rerolled for D7.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Same as in D8.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: fix-tabledrag-d7-197641-37.patch, failed testing.

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
sinasalek’s picture

Does this work for tree tables as well? like terms

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: fix-tabledrag-d7-197641-37.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Testbot fluke.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: fix-tabledrag-d7-197641-37.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • David_Rothstein committed 32efb5c on 7.x
    Issue #197641 by herom, good_man, elcuco: Drag and drop does not work...

Status: Fixed » Closed (fixed)

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

mohsen81666’s picture

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.

herom’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Closed (fixed) » Active

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

  • alexpott committed 862d51f on 8.1.x
    Issue #197641 followup by herom, good_man, yhager: Fixed Drag and drop...

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Gábor Hojtsy committed a859f4a on 8.3.x
    #197641 follow up by moofie: make drag and drop handle text translatable...
  • Gábor Hojtsy committed b0007cd on 8.3.x
    #197641 by quicksketch and myself: fix drag and drop RTL issues (...
  • alexpott committed 862d51f on 8.3.x
    Issue #197641 followup by herom, good_man, yhager: Fixed Drag and drop...

  • Gábor Hojtsy committed a859f4a on 8.3.x
    #197641 follow up by moofie: make drag and drop handle text translatable...
  • Gábor Hojtsy committed b0007cd on 8.3.x
    #197641 by quicksketch and myself: fix drag and drop RTL issues (...
  • alexpott committed 862d51f on 8.3.x
    Issue #197641 followup by herom, good_man, yhager: Fixed Drag and drop...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Gábor Hojtsy committed a859f4a on 8.4.x
    #197641 follow up by moofie: make drag and drop handle text translatable...
  • Gábor Hojtsy committed b0007cd on 8.4.x
    #197641 by quicksketch and myself: fix drag and drop RTL issues (...
  • alexpott committed 862d51f on 8.4.x
    Issue #197641 followup by herom, good_man, yhager: Fixed Drag and drop...

  • Gábor Hojtsy committed a859f4a on 8.4.x
    #197641 follow up by moofie: make drag and drop handle text translatable...
  • Gábor Hojtsy committed b0007cd on 8.4.x
    #197641 by quicksketch and myself: fix drag and drop RTL issues (...
  • alexpott committed 862d51f on 8.4.x
    Issue #197641 followup by herom, good_man, yhager: Fixed Drag and drop...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.