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
#104 drupal7-core-js-tabledrag-mobile-1261002-104.patch10.93 KBPere Orga
PASSED: [[SimpleTest]]: [MySQL] 40,936 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

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.

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

Issue tags:+mobile

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

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

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.

Assigned:Unassigned» Brolag

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

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.

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

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.

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.

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.

Status:Needs work» Needs review

engaging bot.

Status:Needs review» Needs work

Coding standards, no tabs.

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?

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

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.

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

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.

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.

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

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/

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.

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.

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.

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

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.

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

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.

Issue tags:-#d8mux+d8mux

Issue tags aren't Twitter hashtags. :-)

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.

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.

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

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.

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.

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.

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

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

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.

Status:Needs review» Reviewed & tested by the community

Works like a charm

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?

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

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

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

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.

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.

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.

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?

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.

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.

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.

Status:Needs work» Needs review

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.

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

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.

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?

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.

Ok roger that, just checking. :)

Status:Needs review» Reviewed & tested by the community

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

it doesn't work for me :(

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?

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

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

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.

Status:Needs review» Reviewed & tested by the community

We just need the bot to go green.

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?

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;
}

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.

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.

Status:Needs review» Reviewed & tested by the community

Let's finish this.

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.

alignment problem in iPad after this patch.

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

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.

Status:Needs work» Needs review

StatusFileSize
new3.18 KB

I learnt how to do interdiffs!

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.

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

Status:Needs review» Reviewed & tested by the community

great :) all good.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

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.

Status:Needs review» Reviewed & tested by the community

The explanation sounds good to me.

RTBC +1

Status:Reviewed & tested by the community» Fixed

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

Priority:Critical» Normal

Priority:Normal» Major

it was a major to begin with :)

Right, sorry :/

Status:Fixed» Closed (fixed)

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

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

Issue summary:View changes

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

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?

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

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 :(

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.

Just checked what modernizr uses:

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

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

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

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

Assigned:Unassigned» Pere Orga

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

Assigned:Pere Orga» Unassigned
Status:Patch (to be ported)» Needs review
StatusFileSize
new10.93 KB
PASSED: [[SimpleTest]]: [MySQL] 40,936 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.

Now that was really fast!!!

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

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 )

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

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.

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

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

Status:Needs work» Needs review

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