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 project? Sign up 8000 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?

@etpinard
Copy link
Contributor
etpinard commented Jan 29, 2018 •
edited
Loading

Is gl2d/camera still in use anywhere, and if so, does it need this treatment as well?

Unfortunately yes. All non-scattergl gl2d trace types use it.

@etpinard etpinard added bug something broken status: in progress labels Jan 29, 2018
@dy
Copy link
Contributor Author
dy commented Jan 29, 2018 •
edited
Loading

@etpinard gl2d events fixed with fa1db3b. Waiting for @mikolalysenko to approve PRs.

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

@alexcjohnson seems that browsers do not allow to trigger default action by emulated events, like scrolling container/document by emulating wheel event. By that, trying to emulate touch drag or scroll do not cause container.scrollTop change. Emulated events can only trigger javascript handlers.

Not sure if we should stop propagation, but testing prevented default is a good thought.

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

@alexcjohnson should be fixed, it remains merging mouse-wheel PR, although it should not affect this PR.

@etpinard
Copy link
Contributor

it remains merging mouse-wheel PR,

We should wait for new mouse-wheel and 3d-view-control versions to be published so that we can bump our pacakge.json and get a proper run on CI.

@dy
Copy link
Contributor Author
dy commented Jan 30, 2018

@etpinard I merged and published 3d-view-controls already, it is up to mouse-wheel, but that only affects single console warning, a patch version.

@etpinard
Copy link
Contributor

but that only affects single console warning, a patch version.

Cool. Let's wait a few days. If we don't get any news from Mikola, we'll merge this thing before the next minor v1.34.0 early next week.

@etpinard etpinard added this to the v1.34.0 milestone Jan 30, 2018
@etpinard
Copy link
Contributor
etpinard commented Feb 5, 2018

@dfcreative Any news from Mikola? If not, would you mind fixing the merge conflicts and merge this in to include this in 1.34.0?

@etpinard
Copy link
Contributor
etpinard commented Feb 5, 2018

Oh wait. This PR was opened before the new npm5 package-lock file was added. Can you regenerate the package-lock with the new deps?

etpinard
etpinard reviewed Feb 6, 2018
View reviewed changes
package.json Outdated
@@ -55,7 +55,6 @@
},
"dependencies": {
"3d-view": "^2.0.0",
"3d-view-controls": "^2.2.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch here. Thanks!

@etpinard
Copy link
Contributor
etpinard commented Feb 6, 2018

Well done @dfcreative 💃

@dy dy merged commit d3a7eab into master Feb 6, 2018
@etpinard etpinard deleted the prevent-mobile-panning branch February 6, 2018 14:19
6D40
@etpinard etpinard mentioned this pull request Feb 15, 2018
Closed
@etpinard etpinard mentioned this pull request Nov 19, 2018
Closed
@archmoj archmoj mentioned this pull request Feb 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etpinard etpinard etpinard left review comments

Assignees
No one assigned
Labels
bug something broken
Projects
None yet
Milestone
v1.34.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dy @alexcjohnson @felipe257 @etpinard
0