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

Update OrbitControls.js to be in compliance with default browser behavior #18612

Closed
wants to merge 1 commit into from

Conversation

EAS-GUO
Copy link

@EAS-GUO EAS-GUO commented Feb 12, 2020

removed event.preventDefault() from onTouchStart(event) to be in compliance with the behavior of "https://developer.mozilla.org/en-US/docs/Web/API/Touch_events/Supporting_both_TouchEvent_and_MouseEvent"

changed document.add/removeEventListener to scope.domElement for consistency in code

changed == to === for js conditions

removed event.preventDefault() from onTouchStart(event) to contain default behavior of "https://developer.mozilla.org/en-US/docs/Web/API/Touch_events/Supporting_both_TouchEvent_and_MouseEvent" 

changed document.add/removeEventListener to scope.domElement for consistency in code

changed == to === for js conditions
@EAS-GUO EAS-GUO changed the title Update OrbitControls.js (#1) Update OrbitControls.js to be in compliance with default browser behavior Feb 12, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2020

removed event.preventDefault() from onTouchStart(event) to be in compliance with the behavior of "https://developer.mozilla.org/en-US/docs/Web/API/Touch_events/Supporting_both_TouchEvent_and_MouseEvent"

Can you please explain in more detail what you mean by that?

@EAS-GUO
Copy link
Author

EAS-GUO commented Feb 12, 2020

I meant to remove the preventDefault() because the eventorder (see the link) would not call the mouse/click events.
This resulted from us when we wanted to add an onclick eventhandler in our application.
This is possible on all browsers because they emulate mouse events from touchevents (as you can also see in the documentation).

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 12, 2020

This resulted from us when we wanted to add an onclick eventhandler in our application.

Okay, thanks for this bit.

So when removing preventDefault() at this place the controls will fire additional mouse events. However, this change might cause unexpected side effects for other users who don't want these events. So this PR might be useful for your use case but it might break other, existing ones.

In the past we have encountered some back and forth in context of OrbitControls. Various devs tried to change the class so it better fits to their personal requirements. However, such a change usually causes issue in other projects. Hence, changes are only merged if there is a compelling reason. I personally don't see this reason here so I don't vote to merge the PR.

However, if @mrdoob and @WestLangley think the change is appropriate, I won't block it.

@EAS-GUO
Copy link
Author

EAS-GUO commented Feb 12, 2020

Yeah i thought this could be useful because then ThreeJS would have the same (default) behavior as every browser on touch-devices.
This means it does not fire additional events, it follows the default event-firing order of the current browser running an ThreeJS application.

@WestLangley
Copy link
Collaborator

I added the preventDefault() in #13706, but I do not recall the reason for doing so. Also, there was some push-back in #13706 for adding it. So, I do not have an opinion one way or the other.

/ping @greggman Do you have an opinion on this?

@WestLangley WestLangley removed their request for review February 13, 2020 02:40
@greggman
Copy link
Contributor

greggman commented Feb 13, 2020

You can see where preventDefault is important for touchstart/touchmove in

https://threejs.org/examples/webgl_multiple_elements.html

Without preventDefault when you try to touch an object to orient it the page will scroll under your finger making it difficult to orient the objects. With preventDefault just the objects move, the page does not.

Maybe add a comment what the point is? Or maybe make that sample add it's own event handlers? Seems to me if you're usng the OrbitControls that's the behavior you'd want.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 13, 2020

Maybe add a comment what the point is?

That sounds like a good idea.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants