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

Enable touch interactions for select/lasso/others #1804

Merged
merged 17 commits into from
Jul 10, 2017
Merged

Enable touch interactions for select/lasso/others #1804

merged 17 commits into from
Jul 10, 2017

Conversation

dy
Copy link
Contributor

@dy dy commented Jun 20, 2017

Resolves #1790

We have to avoid creating dragCover for touch-only devices, because touchmove, unlike mousemove, requires the same element where touchstart happened.

Is there any particular reason we don't use document for catching mouse events? What makes dragCover different from document?

@etpinard @alexcjohnson

TODO:

  • dragCover does not hide cursor for panning and makes it a text cursor, we have to disable
    selection in a way similar to this
  • panning/zooming do not work for scatter2d, that is related to gl2d/camera
  • no mode cursor for scatter2d gl2d cursor doesn't correspond with chosen modebar tool #1489
  • panning is mixed up with page scrolling if body content is long
  • double click should reset the selection

@alexcjohnson
Copy link
Collaborator

Interesting... I'll have to take a look at this and play with it, but to answer your question:

Is there any particular reason we don't use document for catching mouse events? What makes dragCover different from document?

We wanted users to be able to drag off the edge of the plot, but without interacting with other things they might be dragging over until they mouse up. So for example put two plotly plots next to each other, and pan one of them (or zoom by dragging one axis end) until the cursor is over the other - there should be no hover effects in the second one.

@dy
Copy link
Contributor Author

dy commented Jun 20, 2017

@alexcjohnson I don't see any side-effects of panning/dragging over the second plot, although I caught one more bug.

@etpinard
Copy link
Contributor

etpinard commented Jul 4, 2017

@dfcreative would you mind making a standalone page with these patches to help us test them?

@dy
Copy link
Contributor Author

dy commented Jul 4, 2017

@etpinard sure https://dfcreative.github.io/plotly-contrib/select-touch.html

@etpinard
Copy link
Contributor

etpinard commented Jul 4, 2017

https://dfcreative.github.io/plotly-contrib/select-touch.html

Having a bunch of fun playing with ⤴️ on my phone 🎉

@jackparmer wanna give it a shot too?

@etpinard
Copy link
Contributor

etpinard commented Jul 4, 2017

@dfcreative Non-blocking for this PR of course, but I'm curious: how hard would it be to mock double-clicks on mobile?

@jackparmer
Copy link
Contributor

img_4494

very nice! 📱

@etpinard
Copy link
Contributor

etpinard commented Jul 6, 2017

Well, this thing look great to me. 🎉

@alexcjohnson any objections to those patches in dragelement.js?


function pointerOffset(e) {
return mouseOffset(
e.changedTouches && e.changedTouches[0] || e,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not blocking, but I'd prefer more parentheses in cases like this - it works fine, but takes me too much mental effort to work out the operator precedence while reading it.

@alexcjohnson
Copy link
Collaborator

Yeah, looks great! What would it take to test this?

@dy
Copy link
Contributor Author

dy commented Jul 6, 2017

@alexcjohnson emulating touch interactions. Although we had some issues reproducing selection test-cases in CI, so now they are disabled.

@etpinard
Copy link
Contributor

etpinard commented Jul 6, 2017

emulating touch interactions.

👍

Although we had some issues reproducing selection test-cases in CI, so now they are disabled.

Right, we have issue for the select tests for scattergl on CI. But, we should be able to test on SVG scatter here: https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/select_test.js

@dy
Copy link
Contributor Author

dy commented Jul 6, 2017

@etpinard @alexcjohnson done.

Probably we may want to enable touch interactions for rangeslider?

@etpinard
Copy link
Contributor

etpinard commented Jul 6, 2017

Probably we may want to enable touch interactions for rangeslider?
View changes

That's in -> #1098 would be nice to do at some point.

@dy
Copy link
Contributor Author

dy commented Jul 6, 2017

@etpinard may I ask you to run npm run test-jasmine -- select --verbose --nowatch - seems that touches fail in linux

@alexcjohnson
Copy link
Collaborator

Looks great to me! FWIW I played with it at 473f72a on desktop, and I saw the issues that led to the creation of the coverSlip in the first place - like if you start a pan and drag off the edge of the plot, it stops dragging, cursor changes, etc. But at e215ffc, dragging off the edge on both mobile and desktop works as desired.

- so that window.TouchEvent API works in Ubuntu
@etpinard
Copy link
Contributor

etpinard commented Jul 10, 2017

@etpinard may I ask you to run npm run test-jasmine -- select --verbose --nowatch - seems that touches fail in linux

Yeah, it did fail, but

80db1af made it work on my laptop and on CI.

@etpinard
Copy link
Contributor

etpinard commented Jul 10, 2017

✅ on CI, @dfcreative merge away 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants