Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dd-gestures inclusion makes DD.Drag select page text during the drag action #693

Closed
opichals opened this issue May 6, 2013 · 9 comments
Closed
Labels

Comments

@opichals
Copy link
Contributor

opichals commented May 6, 2013

I believe the issue came in http://yuilibrary.com/projects/yui3/ticket/2532718 bugfix. The fix moves the preventDefault handling from 'mousedown' to 'mousemove' to enable click handling on DD.Drag instances. However the mousemove handler calls DDM._move via Y.throttle and therefore selection and other DOM default actions are handled in the throttled-out occurences.

I have worked around that using the following patch, but I am not sure that would not cause other side-effects in other D&D use-cases. Please review.

diff --git a/html/lib/yui/3/build/dd-gestures/dd-gestures.js b/html/lib/yui/3/build/dd-gestures/dd-gestures.js
index dfe2762..2e617f2 100644
--- a/build/dd-gestures/dd-gestures.js
+++ b/build/dd-gestures/dd-gestures.js
@@ -51,7 +51,10 @@ YUI.add('dd-gestures', function (Y, NAME) {

         this._createPG();
         this._active = true;
-        Y.one(Y.config.doc).on('gesturemove', Y.throttle(Y.bind(DDM._move, DDM), DDM.get('throttleTime')), { standAlone: true });
+        Y.one(Y.config.doc).on('gesturemove', Y.throttle(Y.bind(DDM._move, DDM), DDM.get('throttleTime')), {
+            standAlone: true,
+            preventDefault: true
+        });
     };
@rgrove
Copy link
Contributor

rgrove commented May 6, 2013

@opichals This is the fix I would recommend. I recently had the same issue with a separate drag and drop library I'm working on, and this was the solution.

@opichals
Copy link
Contributor Author

It's just that the dd-ddm::_move method does not always call preventDefault() while this patch would prevent it for every occurrence.

@opichals
Copy link
Contributor Author

Also our QA found the suggested solution doesn't work in FF 20. So I opted to to roll my dd-* modules back to version 3.7.1 which we upgraded from. This seems to be working as expected so far.

So in my eyes the http://yuilibrary.com/projects/yui3/ticket/2532718 bugfix introduced quite a nontrivial regression.

@andrewnicols
Copy link
Contributor

Hi all,

We're seeing this issue. What's the chance of it getting some kind of fix integrated for this?

@opichals, what issues were you seeing on FF20?

@andrewnicols
Copy link
Contributor

Pull request submitted in #1557

@tilomitra
Copy link
Contributor

@andrewnicols @rgrove @opichals The proposed fix that we merged in breaks page scrolling due to the document listening to gesturemove and preventing default browser behavior.

You can see this by going to http://yuilibrary.com/yui/docs/dd/groups-drag.html on a touch-enabled device and trying to scroll the page.

I agree that we need the preventDefault() in there to prevent text selection, but doing it on the entire document is too heavy-handed.

@tilomitra
Copy link
Contributor

@clarle and I were thinking that instead of using e.preventDefault(), we can use CSS3 user-select to prevent text selection (that's the end goal, right?)

Here's how it can work:

  • On gesturemovestart, add user-select styles to the boundingBox of the draggable area. This will ensure that none of the children have a visible text selection.
  • Remove the styles on gesturemoveend. (or not. we may want these styles to be permanently set on the boundingBox. What do you guys think?)

This way, you don't need preventDefault and you don't get text selection either. The one caveat when it comes to user-select is that IE is not supported, but since this is in dd-gestures and the problem is specifically with touch input, this isn't a big deal.

I came up with a very rough proof of concept to test if this would work. I basically changed the gesturemove event handler to this:

        Y.one(Y.config.doc).on("gesturemove", function (e) {
            DDM._move(e);
            e.currentTarget.setStyles({
                "-khtml-user-select": "none",
                "-moz-user-select": "none",
                "-ms-user-select": "none",
                 "user-select": "none",
                "-webkit-touch-callout": "none",
                "-webkit-user-select": "none"
            });
        }, {standAlone: true});

Again, we wouldn't want to set styles on gesturemove and we wouldn't want to have this CSS written in JS, but this worked so I think this solution may have legs. Let me know what you guys think.

@andrewnicols
Copy link
Contributor

For reference, this is the original issue we had reported: https://tracker.moodle.org/browse/MDL-42409

I've just installed the android sdk and used avd to replicate on a generic tablet.

Before the first patch, drag/drop is completely unreliable; afterwards it seems to work well. However, the stock Android browser is now blocked from other touch events - e.g. using gestures to move the page.

I've put a video up at https://www.youtube.com/watch?v=yc-MhA4g9v0 showing:

Suffie to say, none of the above performs well at all (not even jQuery).

@tilomitra
Copy link
Contributor

Thanks @andrewnicols the video helps. So looks like we have two problems:

  1. If we use {preventDefault: true} then the page can't scroll which is a bit of a deal-breaker.
  2. If we don't have {preventDefault: true}, then the page isn't usable in Android, which is also a deal-breaker.

I'll look into it to see what else we can do. I have an Android tablet that I can test on, so maybe my results will vary.

andrewnicols added a commit to andrewnicols/yui3 that referenced this issue Mar 21, 2014
Android browsers require that we prevent the default browser action on the
start of drag. We tried this in yui#693/yui#1557, but this broke in other ways
for other browsers.

The issue stems from preventDefault being called on the document instead of
the Node.

This commit moves the preventDefault to the gesturemovestart in the
prototype for DD.Drag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants