Problem/Motivation

The UI added by drupal_add_tabledrag() does not function at all on tested touch screen devices.

Proposed resolution

if you drag an element on top of another one it becomes a child and goes away from the current screen, very much like a folder behave in a file manager lambda.

As inferred from chat transcript at: #36

Remaining tasks

prototype & implementation.

User interface changes

needs prototyping & detailing.

API changes

not applicable

Original report by [LewisNyman ]

The UI added by drupal_add_tabledrag() does not function at all on tested touch screen devices.
One possible solution is to change the default functionality to the 'page weight' boxes when touch events are disabled.

Files: 
CommentFileSizeAuthor
#134 draggable_tables_do_not-1261002-134.patch8.98 KBmglaman
PASSED: [[SimpleTest]]: [MySQL] 41,502 pass(es).
[ View ]
#130 interdiff-1261002-129-130.txt765 bytesPere Orga
#130 drupal7-core-js-tabledrag-mobile-1261002-130.patch8.99 KBPere Orga
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal7-core-js-tabledrag-mobile-1261002-130.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#129 interdiff-1261002-118-129.txt3.66 KBPere Orga
#129 drupal7-core-js-tabledrag-mobile-1261002-129.patch8.36 KBPere Orga
PASSED: [[SimpleTest]]: [MySQL] 41,121 pass(es).
[ View ]
#118 drupal7-core-js-tabledrag-mobile-1261002-118.patch9.22 KBPere Orga
PASSED: [[SimpleTest]]: [MySQL] 41,098 pass(es).
[ View ]
#117 xp_ie7__Running_.png275.19 KBPere Orga
#117 xp_ie6__Running_.png226.93 KBPere Orga
#117 drupal7-core-js-tabledrag-mobile-1261002-117.patch8.81 KBPere Orga
PASSED: [[SimpleTest]]: [MySQL] 40,950 pass(es).
[ View ]
#104 drupal7-core-js-tabledrag-mobile-1261002-104.patch10.93 KBPere Orga
PASSED: [[SimpleTest]]: [MySQL] 40,875 pass(es).
[ View ]
#87 core-js-tabledrag-mobile.patch472 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 55,651 pass(es).
[ View ]
#84 core-js-tabledrag_touch-1261002-84.patch12.19 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 55,639 pass(es).
[ View ]
#84 interdiff.txt576 bytesLewisNyman
#82 interdiff.txt3.18 KBLewisNyman
#80 core-js-tabledrag_touch-1261002-80.patch14.1 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es).
[ View ]
#75 core-js-tabledrag_touch-1261002-75.patch12.19 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 55,523 pass(es).
[ View ]
#73 core-js-tabledrag_touch-1261002-73.patch11.52 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 54,714 pass(es).
[ View ]
#72 pic2.png25.86 KBwebchick
#72 pic3.png24.89 KBwebchick
#70 interdiff_68-to-70.txt627 bytesjessebeach
#70 1261002-touch-table-drag-70.patch12.12 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 54,443 pass(es).
[ View ]
#69 interdiff-57-66.txt894 bytesmgifford
#68 1261002-touch-table-drag-66.patch11.49 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es).
[ View ]
#57 interdiff.txt5.83 KBquicksketch
#57 1261002-touch-table-drag-57.patch11.36 KBquicksketch
PASSED: [[SimpleTest]]: [MySQL] 54,117 pass(es).
[ View ]
#53 1261002-touch-table-drag-53.patch13.46 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 54,004 pass(es).
[ View ]
#45 core-js-tabledrag_touch-1261002-45.patch9.86 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 53,850 pass(es).
[ View ]
#42 core-js-tabledrag_touch-1261002-42.patch9.44 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 53,847 pass(es).
[ View ]
#41 1261002-touch-table-drag-40.patch8.97 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 53,982 pass(es).
[ View ]
#29 core-js-tabledrag_touch-1261002-29.patch26.92 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 42,077 pass(es), 45 fail(s), and 19,182 exception(s).
[ View ]
#22 d7-tabledrag-touch-events.patch2.92 KBsabsbrain
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d7-tabledrag-touch-events.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 drupal-TouchTableDrag-1261002-15.patch2.63 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 41,988 pass(es).
[ View ]
#13 tabledrag.patch3.34 KBblueshadow2911
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tabledrag.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 drag-drop-touch-1261002-7.patch1003 bytesLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 35,656 pass(es).
[ View ]

Comments

Everett Zufelt’s picture

Status:Active» Closed (works as designed)

On touch devices users can touch the 'Show row weights' link above the table and can access the weight column.

Closing, but please re-open if you have a suggestion for a different UI pattern for touch devices.

LewisNyman’s picture

Status:Closed (works as designed)» Active

There is a difference between something being technically achievable and being a good interface. I think we need to take a bit more care with mobile users.

We shouldn't be implying an interface that does not work. I think the least we need to do is detect for touch capable devices and switch the default interface from draggable handles to weighting.

Modernizr has a full suite of comprehensive touch tests. It's pretty bullet proof now. ;-)
#1252178: Add Modernizr to core

LewisNyman’s picture

sun.core’s picture

LewisNyman’s picture

Issue tags:+mobile

I've seen windows 8 do some amazing things with drag and drop and multi touch. It's a dream.

Bojhan’s picture

Note this can only be read by acting out + a beer. We discussed this with swentel and yched at badcamp and we concluded:
1) You select a row and by shaking your phone it moves up or down.
2) If you select nothing and shake the phone all items get sorted ramdomly.
We thought especially the latter was brilliant :D

LewisNyman’s picture

Status:Active» Needs review
StatusFileSize
new1003 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,656 pass(es).
[ View ]

I've created a quick temporary fix. All it does is disable the script if it detects a touch enabled device. We can back port this in to Drupal 7.

I think a long term solution will be to recreate the functionality using touch events if support.

Brolag’s picture

Assigned:Unassigned» Brolag
Brolag’s picture

Assigned:Brolag» Unassigned

I tested drag-drop-touch-1261002-7.patch on Android 2.3 and its working, however it stills removing drag controls on Chrome.

http://modernizr.github.com/Modernizr/touch.html

ZenDoodles’s picture

Status:Needs review» Needs work
+++ b/core/misc/tabledrag.js
@@ -10,7 +10,12 @@
+ *
+ * This script requires a non-touch interface.
  */
+

This is specific to the conditional and probably does not belong in the docblock. I suggest moving the conditional and associated comment above so the docblock can stay with Drupal.behaviors.tableDrag().

+++ b/core/misc/tabledrag.js
@@ -1162,5 +1167,6 @@ Drupal.theme.prototype.tableDragIndentation = function () {
};
+}

})(jQuery);

Wrapping ALL of tabledrag.js in a conditional seems drastic. This patch will work, but it's a bandaid for #1524414: Rewrite tabledrag.js to use jQuery UI. I suggest we postpone this in the hope of moving that issue forward instead. Otherwise, we should probably re-roll it.

altrugon’s picture

Here is the bit that you are missing to detect Chrome:

http://davidwalsh.name/detecting-google-chrome-javascript

And this is how my code looks like :)

var mobileDevice = true;

if(typeof TouchEvent == "undefined") {
  mobileDevice = false;
}
else {
  var is_chrome = navigator.userAgent.toLowerCase().indexOf('chrome') > -1;
  if (is_chrome) {
    mobileDevice = false;
  }
}

if (!mobileDevice) {
Drupal.behaviors.tableDrag = {
...
...

nod_’s picture

Chrome exists on mobile. Also there is another patch out there with the same aim don't just wrap the script in a conditional, use a Drupal.isTouch function or something, we'll be needing this kind of things all over the place.

blueshadow2911’s picture

Status:Needs work» Needs review
StatusFileSize
new3.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tabledrag.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hi..

Here is my workaround for Drupal6, to enable drag-and-drop working on touch screen devices..
It's based on post from http://ross.posterous.com/2008/08/19/iphone-touch-events-in-javascript/

Thank you..

Status:Needs review» Needs work

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

LewisNyman’s picture

StatusFileSize
new2.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,988 pass(es).
[ View ]

I've managed to get this patch to apply cleanly to D8. No promised apart from that yet.

mgifford’s picture

Status:Needs work» Needs review

engaging bot.

nod_’s picture

Status:Needs review» Needs work

Coding standards, no tabs.

moshe weitzman’s picture

Issue tags:+JavaScript

I did a little research and indeed HTML5 drag and drop is not supported in iOS and Android browser. See http://caniuse.com/#feat=dragndrop. I doubt we will implement Touch Events in the next couple of months, so lets proceed as suggested and automatically show the weight dropdowns on touch enabled devices. Could we get a code review of #15?

nod_’s picture

I'd rather bind the touch events to the tabledrag callbacks than try to simulate the events.

seutje’s picture

I'm confused, are we trying to simulate touch events as mouse events, are we just binding to regular touch events or are we disabling tableDrag for touch-devices?
Because the patch in #15 seems to try to simulate touch events as mouse events.

Anything that isn't straight up UA-sniffing is fine by me, btw.

nod_’s picture

Ok touch is a mess :p

The best thing I found was https://github.com/EightMedia/hammer.js/ since it takes care of converting touch things into easily usable events. They don't emulate mouse events. There is no jQuery dependency. I like it very much and it's small enough I'd say.

I'll be working on a patch this week-end, running out of time at the moment. If someone wants to take care of it, feel free to assign yourself :)

sabsbrain’s picture

StatusFileSize
new2.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d7-tabledrag-touch-events.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I appreciate this is a D8 conversation, but as I couldn't get the patch in #15 to work on Drupal 7, I have created a patch for D7 based on the changes there.

I have verified it working on my Galaxy S.

Hope its of use to people.

mgifford’s picture

Status:Needs work» Needs review

changing status for bot.

EDIT: Oops.. Just realized that was redundant.

Status:Needs review» Needs work
Issue tags:-JavaScript, -mobile, -#d8mux

The last submitted patch, d7-tabledrag-touch-events.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
Issue tags:+JavaScript, +mobile, +#d8mux
Bojhan’s picture

Going from #1635416: Make drag & drop mobile friendly

1) moving by selecting and then tapping where it needs to be dropped
2) selecting with one finger, then scrolling with another
3) selecting with one finger, then it autoscrolls when you reach the end of the screen

Lewis mentioned "I think we should implement both 2 and 3" and nod_ agreed.

http://eightmedia.github.com/hammer.js/
http://touchpunch.furf.com/

JohnAlbin’s picture

During the mobile initiative meeting, Moshe brought up that if this issue needs a JS library, then it almost definitely needs to get done before feature freeze.

nod_’s picture

Assigned:Unassigned» nod_

Working on a patch during the next couple of days. Basing the work on HammerJS. Should be easy enough to swap if a better lib is found.

nod_’s picture

StatusFileSize
new26.92 KB
FAILED: [[SimpleTest]]: [MySQL] 42,077 pass(es), 45 fail(s), and 19,182 exception(s).
[ View ]

Ok so I can't test on mobile, but it should work. Turns out it was pretty anti-climatic :p

There are a few things to keep in mind but it's too late to write them down, just starting up the work still need polish.

Status:Needs review» Needs work

The last submitted patch, core-js-tabledrag_touch-1261002-29.patch, failed testing.

seutje’s picture

when I uncomment the version, I don't get any warnings anymore.

Played around a bit with it and I have to say, the implementation is pretty wonky. First I have to tap on an item (and directly on the little arrow cross, it seems), before being able to drag it around. I've also noticed that it helps to drag it slightly to the right before dragging it up/down.

#3 from #26 already sorta works, but the scrolling is ridiculously fast on iPhone and the whole thing feels a bit laggy on every device.
I've been looking into a way to implement #2, but I'm not entirely sure how to go about it, as we would need to first disable pinch zooming, maybe by using Hammer's transform event. Sounds very much like a hack, has to be a way to track 2 fingers separately...

nod_’s picture

I spent a whole day of badcamp working on that. Short story, it's messy and painful.

We got a possible UI that's scalable and usable on mobile, the prototype is almost working but there are some things to think about. First the all-table approach. On the taxonomy listing, tables are not the best way to markup a tree structure while on the field UI tables make a lot of sense. Same thing goes for some of the Views UI things that might be better out of a table (think reordering a list of fields or example).

Then there is the mobile problem of rendering a huge tree (taxonomies can get huge) the JS around the current tabledrag can get even desktop browsers to it's knee on deep tree structures.

The prototype was started with jQuery UI sortable and droppable widget but some custom things were required anyway. But we should clearly separate all the use cases:

  1. Reordering a table (field ui)
  2. Reordering a list (fields in views ui, deltas on multi-value fields?)
  3. Managing a tree structure (menu, taxonomy terms)

If you can think of more, or if you think that are currently using tabledrag should be changed to be closer to the proper markup, let us know.

LewisNyman’s picture

Do we need to get hammer.js in before feature freeze? It's definitely going to be useful beyond this issue alone.

nod_’s picture

Assigned:nod_» Unassigned

Not sure. It's a bug so…

If you have time to get this going feel free, I don't right now.

JohnAlbin’s picture

Issue tags:-#d8mux+d8mux

Issue tags aren't Twitter hashtags. :-)

nod_’s picture

Assigned:Unassigned» JohnAlbin
Priority:Normal» Major

Raising priority since tabledrag still isn't usable on mobile. Pasting a irc chat with Bojhan.

[17:59:07] <nod_> Bojhan: Thinking about trees. Would it be possible to have the tree structure as read-only and switch to a different interface for editing said tree (so that we can make it work on mobile too).
[17:58:06] <Bojhan> nod_: Maybe
[17:58:20] <Bojhan> nod_: Do you have a demo somewhere, how that might look?
[17:58:26] <nod_> the question is really if we can generalize that
[17:58:28] <Bojhan> nod_:  I have never seen that.
[17:59:07] <nod_> Bojhan: no demo yet, just thinking out loud
[17:59:20] <Bojhan> nod_: How is it easier on mobile?
[17:59:29] <WimLeers> nod_: tree handling → drag 'n drop necessary? I'm not sure if there's some kind of touch-enabled tree widget out there
[17:59:31] <nod_> demo might take some work, if I get a clear no from the begining that's that
[18:00:04] <nod_> Bojhan: the flat list with the "parent" item at the top, what we talked about at badcamp
[18:00:35] <Bojhan> nod_: ahh right
[18:00:37] <nod_> WimLeers: not that I've seen for mobile, also, tree on mobile sucks not enough withd
[18:00:55] <Bojhan> nod_: you like "drag" on top of containers" that collapse/open?
[18:00:58] <WimLeers> hm fiar fair enough
[18:01:00] <Bojhan> nod_: if I remember correctly
[18:01:43] <nod_> Bojhan: they don't collapse but if you drag an element on top of another one it becomes a child and goes away from the current screen
[18:02:04] <nod_> Bojhan: very much like a folder behave in a file manager lambda
[18:02:09] <Bojhan> nod_: yup
[18:02:33] <Bojhan> nod_: I think for mobile its a perfect, alternative interaction - for desktop its really not needed.
[18:03:18] <nod_> Bojhan: the think is that I'd want to avoid having two different scripts for mobile or not
[18:03:32] <Bojhan> nod_: Because, discoverability becomes a real problem with folder like interactions on your mobile phone - there simply isn't horizontal width to do deep hierachies because of identing.
[18:03:41] <nod_> Bojhan: since we can't really say for sure that's mobile, load that file. that's desktop, load this one instead
[18:03:49] <Bojhan> nod_: Yhea, then I'd say its a no go
[18:04:08] <Bojhan> nod_: Because it is a really troublesome interaction in terms of discoverability on the desktop, and on the desktop we don't really need it
[18:05:06] <nod_> Bojhan: so what about mobile then? I'm not very hopeful we'll get anything proper as far as mobile support goes for current tabledrag
[18:05:50] <Bojhan> nod_: Yhea, I dont quite know what the ideas are around things like this.
[18:06:08] <Bojhan> nod_: Besides you and me, no one ever asked about it :D
[18:06:45] <nod_> Bojhan: JohnAlbin :p
[18:07:03] <Bojhan> nod_: Perhaps we can split out the drag and drop, from the tree problem?
[18:07:12] <Bojhan> ahh maybe not
[18:07:14] <Bojhan> I dont know :D
[18:07:21] <Bojhan> JohnAlbin: what do we do!

Hence assigning to John for some feedback.

FYI: the "the flat list with the "parent" item at the top, what we talked about at badcamp" refers to #32 and the point is to only show one level at a time and dragging an element onto another would make it it's child and disapear from the current screen and adding a new "parent" element at the top of the list to be able to make an element go up the tree. Think folders and files in any file manager.

quicksketch’s picture

I'm totally behind getting hammer.js (or similar) into core until jQuery UI supports touch directly, but for tabledrag.js (which isn't jQuery UI based and essentially *can't* be as described in #1524414: Rewrite tabledrag.js to use jQuery UI), I think it would make the most sense if we just wrote the JS needed to support touch events directly. No emulation, no falling back on keyboard/mouse events. Just extend the current implementation to handle additional events.

EDIT: Sorry I was thinking that hammer.js offered similar functionality to Touch Punch, which basically hacks jQuery UI until it supports touch natively. I don't think hammer.js is a good fit for table drag. It can be a good library for detecting gestures, but we're mostly interested in touchstart/touchmove/touchend events, for which we don't need a library at all.

quicksketch’s picture

Speaking of Touch Punch, I made a separate issue for adding it over here: #1955926: CKEditor admin interface not touch-compatible (Add jQuery Touch Punch to core).

jessebeach’s picture

trawekp’s picture

I'm afraid that Touch Punch is not a solution. It seems that tabledrag.js is not using jQuery UI because of the lack of flexibility of the library. If you ask me it is better to focus on adding touch events support to tabledrag.js than rewriting it to use jQuery UI.

The main reason to not use jQuery UI is performance. I'm not sure if that would be possible with jQuery UI but it would be much easier to work on performance without it. As _nod said, tabledrag is able to kill desktop browser so imagine what might happen on mobile browser.

My solution

I guess that folderish way might be great. We could enable tabledrag only for active "folder" which is a current level in the tree. The idea behind it is that all levels are not-collapsed at the beggining. Tabledrag now works only for first level. Then we'd be able to collapse a "folder". After doing that only elements in collapsed "folder" are draggable. Moving an element onto another "folder" (element) would add that element into the "folder" and activate that "folder"

Enabling only elements in current "folder" would drastically reduce amount of draggable elements which is great for performance.

What is good it would be possible to use this also for desktop browsers. The only difference would be that all folders would be collapsed by default and dragging an element over the element would change the active "folder"

I hope it does not work like that already. I did not investigated the tabledrag.js line by line. Sorry if I'm messing up here.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new8.97 KB
PASSED: [[SimpleTest]]: [MySQL] 53,982 pass(es).
[ View ]

Patch time!

I got this to a place where it just about works on touch. You can drag and move stuff much like you would with a mouse. The window scroll doesn't work but I don't think window.scroll works great on low powered devices. I think this extra functionality would require a UI redesign that we don't have right now.

nod_’s picture

StatusFileSize
new9.44 KB
PASSED: [[SimpleTest]]: [MySQL] 53,847 pass(es).
[ View ]

That works!

Potential issue is that if someone has the bright idea (kidding) to use a computer with touch + a mouse he'll only be able to drag with touch. It's a problem for the whole web so I'm not sure how we can fix it. Worth keeping in mind though.
me can no read code properly.

For me I'd say we commit the patch and be done with this issue. Anything more involved will require way more time/work/testing.

New patch is only adding Modernizr as a dependency of the tabledrag script.

attiks’s picture

So on touch enabled notebooks this isn't going to work? I'll try to check on mine later today.

+++ b/core/misc/tabledrag.jsundefined
@@ -120,10 +120,16 @@ Drupal.tableDrag = function (table, tableSettings) {
+  if(!Modernizr.touch) {
...
+  if(Modernizr.touch) {

@@ -316,53 +322,23 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
+  if(Modernizr.touch) {
...
+  if(!Modernizr.touch) {

space between if and (
also in other places

attiks’s picture

Status:Needs review» Needs work

I tested #42 on an HP tm2 (Ubuntu 12.10, Chrome 26, touch screen + trackpad), both methods work, but after moving an item, I cannot move it a second time. On admin/structure/menu/manage/admin/edit, on selecting something it turns bright yellow and I can move it, after dropping it becomes pale yellow, but I can no longer move it.

If I only 'touch' an item so it turns bright yellow, but not moving it, after some items it stays light yellow and I noo longer can move any other item.

Related errors in console "Uncaught TypeError: Cannot call method 'dragStart' of undefined (tabledrag.js 338)"

FYI: Modernizr.touch == false, the touch is only emulating my mouse

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new9.86 KB
PASSED: [[SimpleTest]]: [MySQL] 53,850 pass(es).
[ View ]

Alright, I've hopefully fixed all the issues in #43 and #44

There is some “touch OR mouse” logic in this code. The reason is that it's pretty hard for the two to be consolidated together, because current touch only devices emulate mouse events so we would end up with both firing on one tap. None of the browsers have really confirmed around a spec yet. We might just have to work with what we know here. Although I highly expect that will change during the lifespan od D8.

ps. I'm also more than happy to open a follow up issue to redesign and reimplement drag and drop from the ground up.

attiks’s picture

Status:Needs review» Reviewed & tested by the community

Works like a charm

webchick’s picture

Status:Reviewed & tested by the community» Needs work

AWESOME to see this fixed! The code looks really nice, too.

(Sorry, haven't read the issue so apologies if this repeats what's above.)

Interaction-wise, though, both Jess and I had some problems with this. For one, the icon you have to click on in order to engage table drag is super tiny and hard to hit. Then once you've hit it, you have to stay on it and drag your finger carefully lest you slip off.

What might be better is:

1) Make the icon bigger (there's some website somewhere that lays out a minimum height/width for mobile icons). While I know we don't have much room here to play with (particularly in smartphone portrait mode), it feels too hard to tap right now.

2) Have some interaction like a "double-tap" that engages the "dark orange" mode, and lets you tap and drag *anywhere* on the table row to move it around, rather than having to keep your finger awkwardly positioned in the upper left corner of the table row the entire time. Then single tap once more to turn it off.

Then, a couple of things I noticed going through the code:

+++ b/core/misc/tabledrag.jsundefined
@@ -510,15 +488,45 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
+Drupal.tableDrag.prototype.dragStart = function (event, self, item) {

Need some docs on this function.

+++ b/core/misc/tabledrag.jsundefined
@@ -549,7 +557,7 @@ Drupal.tableDrag.prototype.dragRow = function (event, self) {
-      var xDiff = self.currentMouseCoords.x - self.dragObject.indentMousePos.x;
+      var xDiff = self.currentEventCoords.x - self.dragObject.indentMousePos.x;

Not "indentEventPost.x" on the second one?

attiks’s picture

#47 I tested this again to make sure, both on a touch enabled laptop and on a nexus 7 (chrome + ff + opera):

1/ on both devices the icon is big enough (at least for me)
2/ once started dragging my finger doesn't have to stay on the cross mark, as long as I don't move to much to the right it works fine, if I move to much to the right the selected item becomes a child.

I think we should go ahead with the patch as it is (assuming comment are added) and await broader testing by others.

webchick’s picture

Hm. I was testing on an iPhone with Safari and iOS 5.something. Not sure what xjm was testing on.

LewisNyman’s picture

The comments in #47 seem fair, I've been trying to avoid doing any design work and get it just about usable. I was too deep in Javascript I forgot about basic touch usability! So much for many hats...

If the touch targets are too small to be usable then that does fall under this issue, not a follow up. Let's get this ‘acceptable’.

droplet’s picture

It can be a function matching touch & mouse events, also remove modernizr dependency.

Some devices like windows tablets / Chromebook have a touch screen & mouse. On these devices, mouse won't work anymore.

droplet’s picture

It can be a function matching touch & mouse events, also remove modernizr dependency.

Some devices like windows tablets / Chromebook have a touch screen & mouse. On these devices, mouse won't work anymore.

LewisNyman’s picture

StatusFileSize
new13.46 KB
PASSED: [[SimpleTest]]: [MySQL] 54,004 pass(es).
[ View ]

I think droplet might be referring to a library like Pointer.js

Maybe that's a better option, it would be one we need to take across the whole UI though. It also would signify a huge design shift, we would have to assume that all devices are touch enabled, meaning that everything by default would need to be touch friendly, not just conditionally.

Either way I addressed some of the issues raised in #50.

I've increased the size of the handle for touch devices but I've steered clear of adding a higher res image for this patch. Chances are we would be better off adding the icon in a font pack or with SVG and I don't want to have that conversation now. Let's just see if it's big enough to be usable.

attiks’s picture

Status:Needs work» Needs review

I did a quick test of the patch in #53 and it's even easier to drag on tablets, so ++ from me, leaving at NR so @webchick (or somebody else) can test as well on iPhone.

#52 for devices supporting both touch and mouse, is one not emulating the other? Did you test the patch on such a device?

quicksketch’s picture

Status:Needs review» Needs work

This patch seems to contain an unrelated file:

diff --git a/core/modules/overlay/overlay-tabs.css b/core/modules/overlay/overlay-tabs.css
new file mode 100644
index 0000000..d76baa3
--- /dev/null
+++ b/core/modules/overlay/overlay-tabs.css
if(!Modernizr.touch) {

Space missing here after "if"

The current patch still seems to have the problem where after a row is dragged, it's no longer draggable a second time. This happens on desktop browsers (Chrome 26, Mac) with the error Uncaught TypeError: Cannot call method 'dragStart' of undefined in the console.

quicksketch’s picture

This patch also removes the use of "tabledrag-handle-hover" class, which is fine, since newer browsers support :hover on all elements (IE used to only support :hover on links), but there are still references to tabledrag-handle-hover in the remaining JS that needs to be removed.

quicksketch’s picture

StatusFileSize
new11.36 KB
PASSED: [[SimpleTest]]: [MySQL] 54,117 pass(es).
[ View ]
new5.83 KB

Fixing issues mentioned in #55/56.

I'm not sure about the new bigger handles for touch devices is very beneficial. Maybe we should make CSSing tabledrag a separate issue? There are a lot of things we could clean up about handles all at once: They shouldn't use "float: left" anymore for example, they can use inline blocks. The presentation of the connecting lines for child items is totally messed up when there is not enough horizontal space. And as mentioned earlier, if we're going to make the handle icon larger it needs a new image, probably at double-density for hires displays. Seems like all of that should be punted to a new issue.

quicksketch’s picture

Status:Needs work» Needs review
nod_’s picture

So this is a major bug. Taking into account the state of tabledrag pretty much everything that's being said lately here falls under what I said in #42 (yay!) "Anything more involved will require way more time/work/testing.".

I'm not against fixing all this and the feedback raise very good points that should be addressed. On the other hand If #57 fixes #55 then I don't see why we can't commit the patch which would solve "Draggable tables do not work on touch screen devices". Improvements are scope creep and time eating monsters. Sure it's not great but it's not totally broken anymore. We don't have time for a toolbar bis.

No reason why the follow-ups can't be normal tasks.

jessebeach’s picture

I like the change from 'mouse' to 'pointer' as a general term.

I tested this on a Nexus 4 and the drag-drop worked perfectly. The code looks good as well.

I have a few nitpicks regarding documentation. If the followups are logged and linked here and the doc tweaks made, we can RTBC this.

+++ b/core/misc/tabledrag.jsundefined
@@ -511,14 +483,47 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
};

/**
- * Mousemove event handler, bound to document.
+ * Pointer event initiator, creates drag object and information.
+ */
+Drupal.tableDrag.prototype.dragStart = function (event, self, item) {

The parameters in this function and others like it need @param notes e.g.

@param jQuery.Event event
@param Drupal.tableDrag self
@param Drupal.tableDrag.row item

oresh’s picture

patch works fine, no issues found while reviewing.

If what @jessebeach says is right, than the matter is just adding the @param.
I would also suggest for if (Modernizr.touch) { ... } cache the $(document) element, like var $document = $(document) for slightly better performance.

webchick’s picture

It looks like to solve this in CKEditor's admin page, because it uses jQuery UI, we're going to need to bring in the jQuery UI Touch Punch library: #1955926: CKEditor admin interface not touch-compatible (Add jQuery Touch Punch to core)

Is it easier to solve this by just re-using that here?

quicksketch’s picture

Unfortunately Touch Punch works by overriding the functionality of jQuery UI's .draggable() method. Because we're not using jQuery UI in tabledrag (nor can we, see #1524414: Rewrite tabledrag.js to use jQuery UI), Touch Punch's overriding will have no effect on tabledrag.js. This patch essentially does the same thing as Touch Punch, but does it for the bindings we use tabledrag.js.

webchick’s picture

Ok roger that, just checking. :)

nod_’s picture

Status:Needs review» Reviewed & tested by the community

Works for me too, let's get this one out of the way.

proxyninjas’s picture

it doesn't work for me :(

nod_’s picture

Can you give some details, we can't do anything with your feedback.

Which page are you on? which device? which browser on this device? Where did you click/touch to do the drag and drop?

LewisNyman’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new11.49 KB
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es).
[ View ]

Sorry nod! Just one more patch!

I've added the comments as per #60

I've reverted sizing of the handle image for touch devices BUT I've actually made the touch area bigger. So the target area is double the size but doesn't look it. Hopefully that helps enough without pushing into redesign territory

Anything even smelling like a redesign should be pushed to: #1972462: Redesign drag and drop interactions, and nested menu items, for all devices

mgifford’s picture

StatusFileSize
new894 bytes

@LewisNyman is this what you added to make the touch area bigger:
background-position: 23px 8px

Just looking in the interdiff I made.

-a.tabledrag-handle-hover .handle {
+.touch a.tabledrag-handle .handle {
+  background-position: 23px 8px;
+  height: 30px;
+  width: 30px;
+}

This looks like this is almost a re-roll of an RTBC patch. SimplyTest.me is failing to install it, but I think that's sadly an error with SimplyTest.me at the moment.

Is a Opera Mobile Emulator review good enough to mark RTBC? Probably not. Otherwise....

jessebeach’s picture

StatusFileSize
new12.12 KB
PASSED: [[SimpleTest]]: [MySQL] 54,443 pass(es).
[ View ]
new627 bytes

I just updated the comments with type hinting.

/**
* Pointer event initiator, creates drag object and information.
*
* @param jQuery.Event event
*   The event object that trigger the drag.
* @param Drupal.tableDrag self
*   The drag handle.
* @param DOM item
*   The item that that is being dragged.
*/

I retested this on a Nexus 4 and it works great. I think we're ready to RTBC. I'm comfortable with this patch as it is.

jessebeach’s picture

Status:Needs review» Reviewed & tested by the community

We just need the bot to go green.

webchick’s picture

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new24.89 KB
new25.86 KB

Awesome! the interaction here is much nicer than before, and the hit target is large enough I don't slip off accidentally when moving. (Even though it still looks the same size. Curious.)

The one last thing to clean up here (hopefully it's an easy fix) is that the icon, at least on iOS, moves fairly dramatically when I press down on it:

Yellow and up and to the rightOrange and down to the left

Can we just center it in both orientations so it stays put?

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new11.52 KB
PASSED: [[SimpleTest]]: [MySQL] 54,714 pass(es).
[ View ]

Changed 2 lines of CSS, should make it work properly now.

.touch a.tabledrag-handle .handle {
  background-position: 50% 8px;
  …
}
.touch .draggable.drag a.tabledrag-handle .handle {
  background-position: 50% -32px;
}
droplet’s picture

Hit target is too small, when I point my finger to screen, it selected "first line text" rather than icon handler.

+++ b/core/misc/tabledrag.jsundefined
@@ -120,10 +120,17 @@ Drupal.tableDrag = function (table, tableSettings) {
+  if (Modernizr.touch) {
+    $(document).bind('touchmove', function (event) { return self.dragRow(event.originalEvent.touches[0], self); });
+    $(document).bind('touchend', function (event) { return self.dropRow(event.originalEvent.touches[0], self); });
...
+  else {
+    $(document).bind('mousemove', function (event) { return self.dragRow(event, self); });
+    $(document).bind('mouseup', function (event) { return self.dropRow(event, self); });
+  }

minimized the event target to table ? or more limited scope.

LewisNyman’s picture

Assigned:JohnAlbin» Unassigned
StatusFileSize
new12.19 KB
PASSED: [[SimpleTest]]: [MySQL] 55,523 pass(es).
[ View ]

I've tried to maximise the most touch target space without breaking the look and feel. I don't think I can go any further with this patch.

#74

minimized the event target to table ? or more limited scope.

I'm not willing to optimise any of the Javascript in this file. Those event listeners are a exact copy of the mouse events.

nod_’s picture

Status:Needs review» Reviewed & tested by the community

Let's finish this.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

This looks great just a small nit for consistency...

+++ b/core/misc/tabledrag.jsundefined
@@ -511,14 +484,54 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
+  // Create a new rowObject for manipulation of this row.
+  self.rowObject = new self.row(item, 'mouse', self.indentEnabled, self.maxDepth, true);

I think we should either change 'mouse' to 'pointer' here... as this code is used in both the mousedown and touchstart event. We might be able to remove it because looking at the code I can't see how the method property that's assigned in Drupal.tableDrag.prototype.row is actually used.

droplet’s picture

alignment problem in iPad after this patch.

quicksketch’s picture

@droplet: That's pretty vague. Could you describe the alignment problem or (better) provide a screenshot?

LewisNyman’s picture

StatusFileSize
new14.1 KB
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es).
[ View ]

I've fixed the issue raised by Alex in #77 by simply removing the variable completely, it seems to be working fine without it.

I had a look over it on an iPad and it looks as expected. I should note that because the iPad is a touch screen device the alignment will be different but not wrong. The handle elements are much much larger on touch screen devices, which does shift things slightly.

LewisNyman’s picture

Status:Needs work» Needs review
LewisNyman’s picture

StatusFileSize
new3.18 KB

I learnt how to do interdiffs!

nod_’s picture

Status:Needs review» Needs work

The attribute is still needed. it's used in block.js, field_ui.js and views-admin.js and is used for this check: if ((rowObject.method !== 'keyboard' || rowObject.direction === 'down')) {

Changing to pointer or whatever would work.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new576 bytes
new12.19 KB
PASSED: [[SimpleTest]]: [MySQL] 55,639 pass(es).
[ View ]

Thanks Nod.

With that in mind, this patch simply changes 'mouse' to 'pointer'. Did a quick check and couldn't see a file that checks for the mouse method explicitly.

Interdiff is against #75

nod_’s picture

Status:Needs review» Reviewed & tested by the community

great :) all good.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

nod_’s picture

Priority:Major» Critical
Status:Fixed» Needs review
StatusFileSize
new472 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,651 pass(es).
[ View ]

The patch was created and tested before #1993350: Don't add modernizr everywhere went in (or not very far apart, didn't pull before either way). Tabledrag didn't declare it's dependency on Modernizr properly making the whole thing crash.

Patch add modernizr dependency to tabledrag and everything works properly afterwards.

amateescu’s picture

Status:Needs review» Reviewed & tested by the community

The explanation sounds good to me.

Wim Leers’s picture

RTBC +1

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 6c1c3b9 and pushed to 8.x. Thanks!

amateescu’s picture

Priority:Critical» Normal
nod_’s picture

Priority:Normal» Major

it was a major to begin with :)

amateescu’s picture

Right, sorry :/

Status:Fixed» Closed (fixed)

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

mat15’s picture

Is there a patch, that's compatible with drupal 7?

mat15’s picture

Issue summary:View changes

Updating details based on Issue summary standard to bring clarity to the pending items.

Pere Orga’s picture

Version:8.x-dev» 7.x-dev
Issue summary:View changes
Status:Closed (fixed)» Patch (to be ported)

Can I suggest porting this to Drupal 7?

klonos’s picture

From what I see in the issue summary there is no API change and this is a bug, so I guess it qualifies.

Wim Leers’s picture

It requires Modernizr… which is already part of Drupal 8 and therefor this didn't introduce API changes, but adding Modernizr to Drupal 7 very much is an API change. So I don't see how this can be backported, I'm afraid :(

Pere Orga’s picture

we could replace:

if (Modernizr.touch)

with:

if ('ontouchstart' in document.documentElement)

And when this is false, add a 'no-touch' class to 'a.tabledrag-handle' to apply the different style.

Pere Orga’s picture

Just checked what modernizr uses:

if(('ontouchstart' in window) || window.DocumentTouch && document instanceof DocumentTouch)

https://github.com/Modernizr/Modernizr/blob/master/feature-detects/touch...

klonos’s picture

...was about to suggest what @netol said. We don't need something as full-fledged as Modernizr in place - just the basics that'll make things work for us.

Wim Leers’s picture

Hah! I had no idea Modernizr only had to do something that simple! By all means, go ahead then :) Sorry to hold you up!

Pere Orga’s picture

Assigned:Unassigned» Pere Orga

No worries, I wanted to make sure it makes sense before I try to submit a patch :)

Pere Orga’s picture

Assigned:Pere Orga» Unassigned
Status:Patch (to be ported)» Needs review
StatusFileSize
new10.93 KB
PASSED: [[SimpleTest]]: [MySQL] 40,875 pass(es).
[ View ]

I didn't need to alter the CSS, or to add more classes.

Tested on Chrome for Android & Firefox Mobile for Android. Everything seems to work fine.
I verified that does not break current desktop functionality.

klonos’s picture

Now that was really fast!!!

Any specific places that need testing? I can test on iPhone.

Pere Orga’s picture

If you could test on iphone.
You can find a draggable table in the 'manage fields' section of a content type (i.e. /admin/structure/types/manage/article/fields )

acbramley’s picture

Thanks @Pere Orga we just had a bug raised for iPad for the draggable tables and your patch fixed it

wesleydv’s picture

Status:Needs review» Reviewed & tested by the community

Tested on
- Android 4.3 native en chrome
- Android 4.4 chrome
- iPad 1 iOS 5.1.1 safari
- Tested on latest (april 2014) iOS simulator safari

All seem to work.

acbramley’s picture

Status:Reviewed & tested by the community» Needs work

This may still need a bit of work, I've had my testers report it doesn't work on a samsung tablet they have been using. Will update soon with the Android version and browser.

Update: This was using Android 4.2.2 with the native "internet" browser so we're probably good to RTBC

Pere Orga’s picture

@acbramley would you able to reproduce the same issue in Drupal 8?

Pere Orga’s picture

Status:Needs work» Needs review
acbramley’s picture

Status:Needs review» Reviewed & tested by the community

@Pete Orga, works well on D8 on the same device and browser (there is however a small display issue with the handle icon being chopped off at the bottom). In fact, I just tested the drag-n-drop on D7 with the above patch on this android device and it does work, it's just the handle is very small so it's easy to miss. Looks like this is good to go :)

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 104: drupal7-core-js-tabledrag-mobile-1261002-104.patch, failed testing.

Pere Orga’s picture

Status:Needs work» Needs review
Pere Orga’s picture

Status:Needs review» Reviewed & tested by the community

Restoring status back to 'Reviewed & tested by the community'.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work

Really neat that there's a chance of getting this fixed in Drupal 7!

And thanks for all the testing on mobile devices, but has anyone tested the Drupal 7 patch on regular browsers? I tried Firefox and it seemed to be fine, but I'm particularly worried about versions of Internet Explorer (given the code that's being changed/removed).

-Drupal.tableDrag.prototype.mouseCoords = function (event) {
+Drupal.tableDrag.prototype.pointerCoords = function (event) {

This (and similar) are technically public API functions, so we shouldn't be removing them in a stable release. Basically, I'd encourage looking for the minimal patch possible here (without any unnecessary renames or code removal). If you feel strongly about renaming any functions/variables to reduce the usage of the word "mouse", then if those are public in any way the old one at least needs to be left around as a deprecated wrapper around the new one so as to not risk breaking anyone's code.

Pere Orga’s picture

Status:Needs work» Needs review
StatusFileSize
new8.81 KB
PASSED: [[SimpleTest]]: [MySQL] 40,950 pass(es).
[ View ]
new226.93 KB
new275.19 KB

Thanks for the review.

I have updated the code, touching now less code, keeping 'mouse' in the function / variable names. Comments are updated though (and I think it makes sense to do it).

I did test desktop browsers (Chrome and Firefox IIRC), and now I have tested IE 6, IE 7, IE 8 & IE9 and everything is working as expected (attached screenshots when testing IE6 and IE7). I could test newer versions of Internet Explorer (IE10+) and Konqueror but I see no reason why should not work in these browsers as well. Testing ancient versions of Konqueror would be quite pointless IMHO (I'm aware that a Konqueror-specific hack is being removed). I retested mobile versions too.

Pere Orga’s picture

StatusFileSize
new9.22 KB
PASSED: [[SimpleTest]]: [MySQL] 41,098 pass(es).
[ View ]

New patch attached, removing CSS selector not used anymore ('.tabledrag-handle-hover').

Pere Orga’s picture

And sorry the confusion, the Konqueror hack is kept, an IE6 hack is removed but I could not reproduce any IE6 issues, even when moving selects (and that's what the hack is targeting, just retested)

quicksketch’s picture

Thanks Pere Orga for your work on this! Undoubtedly this is a great improvement. Making a new method for start definitely makes sense, but why not use "this" instead of passing in the "self" parameter manually? You've already got a reference to the current object since this is a method on an existing instance.

Instead of:

+Drupal.tableDrag.prototype.dragStart = function (event, self, item) {
+  // Create a new dragObject recording the pointer information.
+  self.dragObject = {};

Use:

Drupal.tableDrag.prototype.dragStart = function (event, item) {
  var self = this;
  // Create a new dragObject recording the pointer information.

Or there's no harm in just using "this" instead of "self" throughout that function. The use of "self" is only needed if you're trying to use the variable inside of closures where the scope of "this" changes.

quicksketch’s picture

but why not use "this" instead of passing in the "self" parameter manually?

Oh... I see that other methods, like Drupal.tableDrag.prototype.dropRow do this as well, passing in "self" when they could have just used "this"... As the original author of such code I'll claim ignorance on behalf of my previous self. That was a long time ago.

Considering you're just maintaining consistency with the current code, it'd make sense to leave it as-is. This looks good to me.

LewisNyman’s picture

Issue tags:+frontend
mgifford’s picture

Fun. Just tested this patch in D7 with my Ubuntu touchscreen. Seems to work fine.

The Hide row weights/Show row weights is still in effect so this should be fine.

Did a quick test on ChromeVox and it seemed to work fine there too.

mglaman’s picture

Tested and worked perfectly to resolve our issues.

sethmac’s picture

Tested on iOS7 without success. When I try to drag the handler it drags the screen rather than the row.

LewisNyman’s picture

Status:Needs review» Reviewed & tested by the community

Just tested on iOS8. Worked great for me. @sethmac maybe you just need to be a touch more accurate?

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work

Thanks for the continued work on this.

-  // Add hover action for the handle.
-  handle.hover(function () {
-    self.dragObject == null ? $(this).addClass('tabledrag-handle-hover') : null;
-  }, function () {
-    self.dragObject == null ? $(this).removeClass('tabledrag-handle-hover') : null;
-  });

Why is all this being removed? When I tested the patch, this leads to a noticeable (although somewhat subtle) difference - you no longer see the drag handler change color when you hover over it.

-    // Hack for IE6 that flickers uncontrollably if select lists are moved.
-    if (navigator.userAgent.indexOf('MSIE 6.') != -1) {
-      $('select', this.table).css('display', 'none');
-    }
-
-    // Hack for Konqueror, prevent the blur handler from firing.
-    // Konqueror always gives links focus, even after returning false on mousedown.
-    self.safeBlur = false;
-
-    // Call optional placeholder function.
-    self.onDrag();

Similarly, why is this being removed? (Especially the last part - that exists to allow other code to hook into the event, right?) Also, for the IE6 and Konqueror stuff, I certainly wouldn't add new hacks for them at this point, but don't really see why we'd remove ones that are already there... unless the code is causing an actual problem for other browsers somehow?

+ * @param jQuery.Event event
+ *   The event object that trigger the drag.
+ * @param Drupal.tableDrag self
+ *   The drag handle.
+ * @param DOM item
+ *   The item that that is being dragged.
+ */
+Drupal.tableDrag.prototype.dragStart = function (event, self, item) {

A couple typos here ("trigger" => "triggered", and double "that").

Pere Orga’s picture

Thanks David, you are right. The lines above should be all kept unless there's a reason to remove them (I initially removed that because the patch was based in the D8 patch)

I will look into it this weekend, if no one does it before.

Pere Orga’s picture

Status:Needs work» Needs review
StatusFileSize
new8.36 KB
PASSED: [[SimpleTest]]: [MySQL] 41,121 pass(es).
[ View ]
new3.66 KB

Adding patch with the requested changes. Adding interdiff too.

Tested again with latests Chrome desktop and Chrome mobile.

Pere Orga’s picture

StatusFileSize
new8.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal7-core-js-tabledrag-mobile-1261002-130.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new765 bytes

Patch attached with minor improvements to comments

mglaman’s picture

Status:Needs review» Reviewed & tested by the community

We've been using this for some time and has fixed our issues.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 130: drupal7-core-js-tabledrag-mobile-1261002-130.patch, failed testing.

mglaman’s picture

Status:Needs work» Needs review
StatusFileSize
new8.98 KB
PASSED: [[SimpleTest]]: [MySQL] 41,502 pass(es).
[ View ]

Here is a reroll of #130 against latest 7.x HEAD!

xjm’s picture

Issue tags:+needs backport to D7

(Making it clear that this patch is a backport.)