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

Fix only left click draggable bar #4613

Merged
merged 21 commits into from Nov 16, 2017
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/js/control-bar/progress-control/seek-bar.js
Expand Up @@ -113,6 +113,10 @@ class SeekBar extends Slider {
* @listens mousedown
*/
handleMouseDown(event) {
if (!Dom.isSingleLeftClick(event)) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I find this series of if blocks confusing. Can it be simplified to:

if ((event.button !== undefined && event.button !== 0) || (event.buttons !== undefined && event.buttons !== 1)) {
  return;
}

Does that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know :/, what I did was trial by combat, didn't have device to test right now, think I will stick with what work, maybe change to that later if I have device to test (or yours)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beside, I think if we do the current style, we have a chance to handle/explain what is going on with particular device, OS or stuff

Copy link
Member

Choose a reason for hiding this comment

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

You could potentially do something like:

  // only allow left-click seeking
if (
  // touch screen, not apply, do nothing
  !(event.button === undefined && event.buttons === undefined) &&
  // touch screen, safari on ios
  !(event.button === 0 && event.buttons === undefined) &&
  (event.button !== 0 || event.buttons !== 1)
) {
 return;
}

Also, we probably should pull this out into a tester function because keeping this copied between the two places is bound to cause issues down the line. Maybe moved to dom.js as a isNotLeftClick(event) or something like that and can be used something like so:

if (isNotLeftClick(event)) {
  return;
}


this.player_.scrubbing(true);

this.videoWasPlaying = !this.player_.paused();
Expand All @@ -130,6 +134,10 @@ class SeekBar extends Slider {
* @listens mousemove
*/
handleMouseMove(event) {
if (!Dom.isSingleLeftClick(event)) {
return;
}

let newTime = this.calculateDistance(event) * this.player_.duration();

// Don't let video end while scrubbing.
Expand Down
21 changes: 21 additions & 0 deletions src/js/control-bar/volume-control/volume-bar.js
Expand Up @@ -3,6 +3,7 @@
*/
import Slider from '../../slider/slider.js';
import Component from '../../component.js';
import * as Dom from '../../utils/dom.js';

// Required children
import './volume-level.js';
Expand Down Expand Up @@ -45,6 +46,22 @@ class VolumeBar extends Slider {
});
}

/**
* Handle mouse down on volume bar
*
* @param {EventTarget~Event} event
* The `mousedown` event that caused this to run.
*
* @listens mousedown
*/
handleMouseDown(event) {
if (!Dom.isSingleLeftClick(event)) {
return;
}

super.handleMouseDown(event);
}

/**
* Handle movement events on the {@link VolumeMenuButton}.
*
Expand All @@ -54,6 +71,10 @@ class VolumeBar extends Slider {
* @listens mousemove
*/
handleMouseMove(event) {
if (!Dom.isSingleLeftClick(event)) {
return;
}

this.checkMuted();
this.player_.volume(this.calculateDistance(event));
}
Expand Down
46 changes: 46 additions & 0 deletions src/js/utils/dom.js
Expand Up @@ -727,6 +727,52 @@ export function insertContent(el, content) {
return appendContent(emptyEl(el), content);
}

/**
* Check if event was a single left click
*
* @param {EventTarget~Event} event
* Event object
*
* @return {boolean}
* - True if a left click
* - False if not a left click
*/
export function isSingleLeftClick(event) {
/* Note: if you create something draggable, be sure to
Copy link
Member

Choose a reason for hiding this comment

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

Minor stylistic concern, but could we stick to using /* */ comments for JSDoc only and use // for all inline comments (even if they span multiple lines)?

call it on both `mousedown` and `mousemove` event,
otherwise `mousedown` should be enough for a button */

if (event.button === undefined && event.buttons === undefined) {
/* Why do we need `butttons` ?
Because, middle mouse sometimes have this:
e.button === 0 and e.buttons === 4
Furthermore, we want to prevent combination click, something like
HOLD middlemouse then left click, that would be
e.button === 0, e.buttons === 5
just `button` is not gonna work

Alright, then what this block does ?
this is for chrome `simulate mobile devices`
I want to support this as well */

return true;
} else if (event.button === 0 && event.buttons === undefined) {
// Touch screen, sometimes on some specific device, `buttons`
// doesn't have anything (safari on ios, blackberry...)

return true;
} else if (event.button !== 0 || event.buttons !== 1) {
/* This is the reason we have those if else block above
if any special case we can catch and let it slide
we do it above, when get to here, this definitely
is-not-left-click */

return false;
}

return true;
}

/**
* Finds a single DOM element matching `selector` within the optional
* `context` of another DOM element (defaulting to `document`).
Expand Down