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

Change slider target from document to player.el_ when it listen to mouse and touch events #1962

Open
carpasse opened this issue Mar 20, 2015 · 1 comment
Labels
needs: discussion Needs a broader discussion pinned Things that stalebot shouldn't close automatically

Comments

@carpasse
Copy link
Contributor

carpasse commented Mar 20, 2015

Hello guys

I am currently experiencing a problem with the video player inside a mobile slide gallery.

The video progress can not be modified because the swipe of the progress bar triggers a swipe on the slide gallery.

I took a look at the code and the slide component listens to the mouse and touch events directly from the document.

  this.on(document, 'mousemove', this.onMouseMove);
  this.on(document, 'mouseup', this.onMouseUp);
  this.on(document, 'touchmove', this.onMouseMove);
  this.on(document, 'touchend', this.onMouseUp);

The problem is is that I can not stop propagation of these events since they are bind to the document.

I propose that we change the code to:

  this.on(player.el(), 'mousemove', this.onMouseMove);
  this.on(player.el(), 'mouseup', this.onMouseUp);
  this.on(player.el(), 'touchmove', this.onMouseMove);
  this.on(player.el(), 'touchend', this.onMouseUp);

This will make it necessary to change the code of the progressBar control and probably the volume control but it will give the the possibility to the controls that rely on the slider, to stopPropagation onMouseMove and onMouseUp handlers and therefore, prevents issues like the one I described.

I can implement this change if you guys want.

@gkatsev
Copy link
Member

gkatsev commented Nov 17, 2015

That sounds good but there is an issue with only having it work inside the player element.
If you were to press or touch the slider and then move around, by listening to the document, it means that you can move your mouse or finger anywhere on the page and have it still track your movement in the slider. This is particularly useful on mobile where you want to seek but don't want your finger over the video while you're seeking.
Not sure which one is better to keep, though, I'd lean towards better touch support than carousel support. Maybe we just need to make it configurable or something.

Also, re: carousels http://shouldiuseacarousel.com/ :P

@gkatsev gkatsev added the needs: discussion Needs a broader discussion label Nov 17, 2015
@gkatsev gkatsev added the pinned Things that stalebot shouldn't close automatically label Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion Needs a broader discussion pinned Things that stalebot shouldn't close automatically
Projects
None yet
Development

No branches or pull requests

2 participants