8000 Panning in mobile devices by dy · Pull Request #2296 · plotly/plotly.js · GitHub
[go: up one dir, main page]
Skip to content

Panning in mobile devices #2296

New issue

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

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Feb 6, 2018
Merged

Panning in mobile devices #2296

merged 13 commits into from
Feb 6, 2018

Conversation

dy
Copy link
Contributor
@dy dy commented Jan 25, 2018 •
edited
Loading

This prevents page scroll while panning 3d plots in mobile devices, related to #1411 (comment).

Also this disables event logging for 3d plots, akin to #2251.

@alexcjohnson please review.

@alexcjohnson
Copy link
Collaborator
alexcjohnson commented Jan 25, 2018 •
edited by dy
Loading
  • tests
  • verify manually on an actual iPad

Also two questions:

  • we have event.preventDefault() in a few places, but then we also have Lib.pauseEvent(event) which does other things as well... are there browsers that still need this (in which case we should use it everywhere) or is preventDefault now sufficient on every platform we support?
    • Corollary to that, and nothing to do with this PR but I'm curious - why do I see a couple of d3.event.preventDefault() but other places d3.event.sourceEvent.preventDefault() - is one pattern broken, are these different in some way that we need the different syntax, or can we simplify the latter?
  • Is gl2d/camera still in use anywhere, and if so, does it need this treatment as well?

@dy
Copy link
Contributor Author
dy commented Jan 26, 2018 •
edited
Loading

@alexcjohnson that might be difficult to test if CI window gets scroll changed while panning 3d/2d plots. Is that sort of test we ideally want?

@dy dy mentioned this pull request Jan 26, 2018
Open
@dy
Copy link
Contributor Author
dy commented Jan 26, 2018

@alexcjohnson I don't see any difficulty in e.preventDefault() direct call − since that is supported in IE ≥9, and that is precise instruction to avoid page scroll. Not sure if stopping propagation of the touchstart event and keeping the rest of the handlers (mousedown etc) is correct pattern. So I would not force Lib.pauseEvent everywhere (I would not consider Lib.pauseEvent a necessary wrapper, working with DOM directly is safe here).
As for d3.event.preventDefault − considering the source, that seems to be always DOM event, and seems to have no difference with d3.event.sourceEvent (some delayed events or alike).

@alexcjohnson
Copy link
Collaborator

Right, it's a little difficult, but if we're fixing a bug we really should try to find a way to test it. Two ideas come to mind:

  • attach an event handler to some parent element for the event (touchmove?) that would have led to scrolling, and verify that this handler is not called (but before the fix it is called)
  • hold on to the touchmove event we generate, inspect it afterward and make sure it had preventdefault called on it.

Thanks for looking into pauseEvent - indeed it looks safe to 🔪 , wouldn't need to happen in this PR but you're welcome to if you feel like it.

@felipe257
Copy link

I've noticed on python module that when fixedrange is true for x and y it doesn't reset the behavior of touchmove (or touchstart?), i.e., the page won't scroll when touchmove on the graph. Is it on purpose?