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.
Comment | File | Size | Author |
---|---|---|---|
#156 | draggable_tables_do_not-1261002-156.patch | 1.46 KB | Trey |
#154 | draggable_tables_do_not-1261002-154.patch | 1.44 KB | Trey |
#152 | draggable_tables_do_not-1261002-152.patch | 1.15 KB | Trey |
#134 | draggable_tables_do_not-1261002-134.patch | 8.98 KB | mglaman |
#130 | interdiff-1261002-129-130.txt | 765 bytes | Pere Orga |
Comments
Comment #1
Everett Zufelt CreditAttribution: Everett Zufelt commentedOn 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.
Comment #2
LewisNymanThere 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
Comment #3
LewisNymanInteresting information here. http://www.quirksmode.org/mobile/tableTouch.html
Comment #4
sun.core CreditAttribution: sun.core commentedComment #5
LewisNymanI've seen windows 8 do some amazing things with drag and drop and multi touch. It's a dream.
Comment #6
Bojhan CreditAttribution: Bojhan commentedNote 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
Comment #7
LewisNymanI'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.
Comment #8
Brolag CreditAttribution: Brolag commentedComment #9
Brolag CreditAttribution: Brolag commentedI 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
Comment #10
ZenDoodles CreditAttribution: ZenDoodles commentedThis 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().
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.
Comment #11
altrugon CreditAttribution: altrugon commentedHere 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 :)
Comment #12
nod_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.
Comment #13
blueshadow2911 CreditAttribution: blueshadow2911 commentedHi..
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..
Comment #15
LewisNymanI've managed to get this patch to apply cleanly to D8. No promised apart from that yet.
Comment #16
mgiffordengaging bot.
Comment #17
nod_Coding standards, no tabs.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedI 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?
Comment #19
nod_I'd rather bind the touch events to the tabledrag callbacks than try to simulate the events.
Comment #20
seutje CreditAttribution: seutje commentedI'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.
Comment #21
nod_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 :)
Comment #22
sabsbrain CreditAttribution: sabsbrain commentedI 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.
Comment #23
mgiffordchanging status for bot.
EDIT: Oops.. Just realized that was redundant.
Comment #25
mgifford#15: drupal-TouchTableDrag-1261002-15.patch queued for re-testing.
Comment #26
Bojhan CreditAttribution: Bojhan commentedGoing 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/
Comment #27
JohnAlbinDuring 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.
Comment #28
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.
Comment #29
nod_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.
Comment #31
seutje CreditAttribution: seutje commentedwhen 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...
Comment #32
nod_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:
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.
Comment #33
LewisNymanDo we need to get hammer.js in before feature freeze? It's definitely going to be useful beyond this issue alone.
Comment #34
nod_Not sure. It's a bug so…
If you have time to get this going feel free, I don't right now.
Comment #35
JohnAlbinIssue tags aren't Twitter hashtags. :-)
Comment #36
nod_Raising priority since tabledrag still isn't usable on mobile. Pasting a irc chat with Bojhan.
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.
Comment #37
quicksketchI'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.
Comment #38
quicksketchSpeaking 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).
Comment #39
jessebeach CreditAttribution: jessebeach commentedCKEditor also needs touch support for draggables: #1955926: CKEditor admin interface not touch-compatible (Add jQuery Touch Punch to core).
Comment #40
trawekp-1 CreditAttribution: trawekp-1 commentedI'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.
Comment #41
LewisNymanPatch 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.
Comment #42
nod_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.
Comment #43
attiks CreditAttribution: attiks commentedSo on touch enabled notebooks this isn't going to work? I'll try to check on mine later today.
space between if and (
also in other places
Comment #44
attiks CreditAttribution: attiks commentedI 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
Comment #45
LewisNymanAlright, 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.
Comment #46
attiks CreditAttribution: attiks commentedWorks like a charm
Comment #47
webchickAWESOME 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:
Need some docs on this function.
Not "indentEventPost.x" on the second one?
Comment #48
attiks CreditAttribution: attiks commented#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.
Comment #49
webchickHm. I was testing on an iPhone with Safari and iOS 5.something. Not sure what xjm was testing on.
Comment #50
LewisNymanThe 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’.
Comment #51
droplet CreditAttribution: droplet commentedIt 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.
Comment #52
droplet CreditAttribution: droplet commentedIt 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.
Comment #53
LewisNymanI 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.
Comment #54
attiks CreditAttribution: attiks commentedI 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?
Comment #55
quicksketchThis patch seems to contain an unrelated file:
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.Comment #56
quicksketchThis 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.
Comment #57
quicksketchFixing 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.
Comment #58
quicksketchComment #59
nod_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.
Comment #60
jessebeach CreditAttribution: jessebeach commentedI 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.
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
Comment #61
oresh CreditAttribution: oresh commentedpatch 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.
Comment #62
webchickIt 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?
Comment #63
quicksketchUnfortunately 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.
Comment #64
webchickOk roger that, just checking. :)
Comment #65
nod_Works for me too, let's get this one out of the way.
Comment #66
proxyninjas CreditAttribution: proxyninjas commentedit doesn't work for me :(
Comment #67
nod_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?
Comment #68
LewisNymanSorry 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
Comment #69
mgifford@LewisNyman is this what you added to make the touch area bigger:
background-position: 23px 8px
Just looking in the interdiff I made.
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....
Comment #70
jessebeach CreditAttribution: jessebeach commentedI just updated the comments with type hinting.
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.
Comment #71
jessebeach CreditAttribution: jessebeach commentedWe just need the bot to go green.
Comment #72
webchickAwesome! 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:
Can we just center it in both orientations so it stays put?
Comment #73
nod_Changed 2 lines of CSS, should make it work properly now.
Comment #74
droplet CreditAttribution: droplet commentedHit target is too small, when I point my finger to screen, it selected "first line text" rather than icon handler.
minimized the event target to table ? or more limited scope.
Comment #75
LewisNymanI'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
I'm not willing to optimise any of the Javascript in this file. Those event listeners are a exact copy of the mouse events.
Comment #76
nod_Let's finish this.
Comment #77
alexpottThis looks great just a small nit for consistency...
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.Comment #78
droplet CreditAttribution: droplet commentedalignment problem in iPad after this patch.
Comment #79
quicksketch@droplet: That's pretty vague. Could you describe the alignment problem or (better) provide a screenshot?
Comment #80
LewisNymanI'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.
Comment #81
LewisNymanComment #82
LewisNymanI learnt how to do interdiffs!
Comment #83
nod_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.
Comment #84
LewisNymanThanks 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
Comment #85
nod_great :) all good.
Comment #86
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #87
nod_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.
Comment #88
amateescu CreditAttribution: amateescu commentedThe explanation sounds good to me.
Comment #89
Wim LeersRTBC +1
Comment #90
alexpottCommitted 6c1c3b9 and pushed to 8.x. Thanks!
Comment #91
amateescu CreditAttribution: amateescu commentedComment #92
nod_it was a major to begin with :)
Comment #93
amateescu CreditAttribution: amateescu commentedRight, sorry :/
Comment #95
mat15 CreditAttribution: mat15 commentedIs there a patch, that's compatible with drupal 7?
Comment #95.0
mat15 CreditAttribution: mat15 commentedUpdating details based on Issue summary standard to bring clarity to the pending items.
Comment #96
Pere OrgaCan I suggest porting this to Drupal 7?
Comment #97
klonosFrom what I see in the issue summary there is no API change and this is a bug, so I guess it qualifies.
Comment #98
Wim LeersIt 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 :(
Comment #99
Pere Orgawe could replace:
with:
And when this is false, add a 'no-touch' class to 'a.tabledrag-handle' to apply the different style.
Comment #100
Pere OrgaJust checked what modernizr uses:
https://github.com/Modernizr/Modernizr/blob/master/feature-detects/touch...
Comment #101
klonos...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.
Comment #102
Wim LeersHah! I had no idea Modernizr only had to do something that simple! By all means, go ahead then :) Sorry to hold you up!
Comment #103
Pere OrgaNo worries, I wanted to make sure it makes sense before I try to submit a patch :)
Comment #104
Pere OrgaI 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.
Comment #105
klonosNow that was really fast!!!
Any specific places that need testing? I can test on iPhone.
Comment #106
Pere OrgaIf 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 )
Comment #107
acbramley CreditAttribution: acbramley commentedThanks @Pere Orga we just had a bug raised for iPad for the draggable tables and your patch fixed it
Comment #108
wesleydv CreditAttribution: wesleydv commentedTested 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.
Comment #109
acbramley CreditAttribution: acbramley commentedThis 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
Comment #110
Pere Orga@acbramley would you able to reproduce the same issue in Drupal 8?
Comment #111
Pere OrgaComment #112
acbramley CreditAttribution: acbramley commented@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 :)
Comment #114
Pere Orga104: drupal7-core-js-tabledrag-mobile-1261002-104.patch queued for re-testing.
Comment #115
Pere OrgaRestoring status back to 'Reviewed & tested by the community'.
Comment #116
David_Rothstein CreditAttribution: David_Rothstein commentedReally 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).
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.
Comment #117
Pere OrgaThanks 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.
Comment #118
Pere OrgaNew patch attached, removing CSS selector not used anymore ('.tabledrag-handle-hover').
Comment #119
Pere OrgaAnd 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)
Comment #120
quicksketchThanks 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:
Use:
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.
Comment #121
quicksketchOh... 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.
Comment #122
LewisNymanComment #123
mgiffordFun. 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.
Comment #124
mglamanTested and worked perfectly to resolve our issues.
Comment #125
sethmac CreditAttribution: sethmac commentedTested on iOS7 without success. When I try to drag the handler it drags the screen rather than the row.
Comment #126
LewisNymanJust tested on iOS8. Worked great for me. @sethmac maybe you just need to be a touch more accurate?
Comment #127
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for the continued work on this.
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.
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?
A couple typos here ("trigger" => "triggered", and double "that").
Comment #128
Pere OrgaThanks 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.
Comment #129
Pere OrgaAdding patch with the requested changes. Adding interdiff too.
Tested again with latests Chrome desktop and Chrome mobile.
Comment #130
Pere OrgaPatch attached with minor improvements to comments
Comment #131
mglamanWe've been using this for some time and has fixed our issues.
Comment #134
mglamanHere is a reroll of #130 against latest 7.x HEAD!
Comment #135
xjm(Making it clear that this patch is a backport.)
Comment #137
TVoc CreditAttribution: TVoc commentedTried it on Android 5.0.2, which did not work with Google Chrome. Any chance this could be updated?
Edit: sorry for raising a false alarm; it does work (version 7.39 at time of writing). It seems I wasn't accurate enough when selecting the item to drag. The fact that the theme I'm using (Software Responsive) uses a smaller icon for draggable elements might have contributed
Comment #138
paulwdru CreditAttribution: paulwdru commentedHi,
Could make it committed into core ?
Already dragged for 5 years. Thanks.
Comment #139
mgifford@paulwdru want to mark it RTBC?
Comment #140
paulwdru CreditAttribution: paulwdru commented@mgifford
Yes, I tried the patch #134 and it works on many mobile browsers as tested.
Thanks
Comment #141
Pere OrgaGreat, thanks.
Setting it to RTBC.
Comment #142
paulwdru CreditAttribution: paulwdru commentedHi,
Still NOT committed in Drupal 7.44.
Please COMMIT urgently. Thanks
Comment #143
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThis should be re-tested by whoever ends up committing this _prior_ to commit.
Marking for Drupal 7 commit as patch-wise this looks good.
Comment #144
droplet CreditAttribution: droplet commentedthis is backported of old code with buggy. We should do this way instead: #2177293: Drag and drop broken on touch enabled laptops
Comment #145
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThanks, droplet.
Comment #146
paulwdru CreditAttribution: paulwdru commented#134 work, why still NOT committed into Drupal 7.x-50 ?
Comment #147
OFF CreditAttribution: OFF commentedThank you!
Comment #148
OFF CreditAttribution: OFF commentedI am trying patch #129 and #134
Drag with mouse not working on windows 10 tablet..
Android is working fine!
Comment #149
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#146: Because Drupal 8 has fixed this in a different way and we need to backport the new fix, per droplet, one of our Javascript maintainers.
Comment #152
Trey CreditAttribution: Trey commentedThis may be over simplistic, but I tested it on OSX and Android, I'll check out iPad when I get home tonight...
Comment #153
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#152: Thank you very much. This looks pretty much already like the fix that ultimately did get into D8:
http://cgit.drupalcode.org/drupal/commit/?id=2867b25
Can we also support the pointerdown and pointermove please like in this issue? (we want to keep code parity for maintenance reasons)
Thank you!
Comment #154
Trey CreditAttribution: Trey commentedUpdated patch to handle pointerdown/move/up
Comment #155
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThanks for the continued work on this. Unfortunately I found two more things before this can be RTBC:
- Should use bind() instead of on() as on() is for later jQuery versions.
- Should have right code style, e.g.
Comment #156
Trey CreditAttribution: Trey commentedGlad to help, wish I had time to work on core stuff more often. I always use jQuery update so I haven't actually used bind in a very long time! But here is the "final" version
Comment #157
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThank you for doing the work on the patch! Every little contribution counts! :)
RTBC! - Some little nits that can be fixed on commit:
nit nit nit - Syntax is not according to our standards. (wrong spacing)
Can be fixed on commit.
Comment #158
stefan.r CreditAttribution: stefan.r commentedComment #160
stefan.r CreditAttribution: stefan.r commentedRe-reviewed with Fabianx, and committed and pushed to 7.x, thanks!
Comment #161
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #163
rudiedirkx CreditAttribution: rudiedirkx at ezCompany commentedThis commit breaks my desktop table drag, because my browser understands
PointerEvent
, but jQuery doesn't.Most browsers don't understand PointerEvent yet, so it's not broken yet, but in the next Chrome version (55), it will break everywhere.
The reason is
jQuery.event.fixHooks
, which has a list of event properties to copy per event type.pointermove
type doesn't exist, so they don't getpageX
,clientX
etc.Broken for every jQuery after 1.5, so it's not actually broken in Drupal Core.
2 solutions:
1. Make TableDrag dumber (don't pointer)
2. Make jQuery smarter (understand pointer)
Comment #164
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#163: This was fixed in #2821441: Newer Chrome versions and IE11/Edge cannot drag and drop anymore on desktop after 7.51 update when jQuery is updated to 1.7-1.11.0, which was already commited, but not yet released.
Please consider applying the patch from there.
Comment #165
rudiedirkx CreditAttribution: rudiedirkx at ezCompany commentedThat's the one. Thanks.
Comment #166
mistermastermatt CreditAttribution: mistermastermatt commentedI think it might also be worth binding the a.tabledrag-handle with preventDefault() so that a tap-and-hold doesn't bring up the extra options menu.
I'm jumping into the end of a very long thread here so sorry if this has already been discussed or is not applicable.
Comment #167
droplet CreditAttribution: droplet commented@mistermastermatt,
It does. Or something I missed? A new issue and patch is welcomed :)
Comment #168
LewisNyman