Missing case in Dom.isSingleLeftClick

[Mikhail1Demianenko]

Description

On mac, tapping the touchpad may be interpreted in two ways and thus, passed down to the browser in different ways (briefly discussed here), but one of them is not accounted for in Dom.isSingleLeftClick. More specifically, when button === 0 && buttons === 1. Therefore, handleMouseDown of SeekBar does not recognize such an event as click and the event gets dismissed by Dom.isSingleLeftClick(event) (line 798 here) called in the body of handleMouseDown (line 252 here).

It then may take 2-3 (up to 8, sometimes) taps to actually handle mouse down (and to change volume or current time, as consequence).

Returning true if button === 0 && buttons === 1 should do the trick 😊

Steps to reproduce

Explain in detail the exact steps necessary to reproduce the issue.

1. On a mac, try tapping (not clicking) around the video search bar or the volume bar to change volume or current time. Can be observed also on the official website's player.

Results

Expected

On each tap, the slider, be it sound or time control, changes its position accordingly

Actual

Due to the nature of the hardware (briefly discussed here), 2-3 taps are needed to actually change the position

Error output

None

Additional Information

versions

videojs

7.17.0

browsers

Tried on latest desktop versions of Chrome, Opera, Firefox, Safari and Edge

OSes

Mac 12.3.1

plugins

None

[welcome[bot]]

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can. To help make it easier for us to investigate your issue, please follow the contributing guidelines.

[mister-ben]

videojs.dom.isSingleLeftClick({button:0, buttons:1}) does return true.

The other case mentioned in SO, {button:0, buttons:0}, doesn't make sense on a mousedown. button:0 means the event was triggered by the primary button, buttons:0 that no buttons are pressed. If I enable tap to click in MacOS settings, I get the same results when tapping, either 00 or 01, and I've not noticed a correlation to how rapidly or gently I tap. Seems like an OS bug really.

[Mikhail1Demianenko]

videojs.dom.isSingleLeftClick({button:0, buttons:1}) does return true.

The other case mentioned in SO, {button:0, buttons:0}, doesn't make sense on a mousedown. button:0 means the event was triggered by the primary button, buttons:0 that no buttons are pressed. If I enable tap to click in MacOS settings, I get the same results when tapping, either 00 or 01, and I've not noticed a correlation to how rapidly or gently I tap. Seems like an OS bug really.

Yes, my bad, the {0, 0} case returns false, not {0, 1}. It might be a bug, yes. I dont get why fire the event if nothing is pressed. Should this case not be accounted for?

The logic of Dom.isSingleLeftClick is not completely clear to me, but to my understanding the first if could be converted to something like if (!e.button && !e.buttons) return true. Here is the original function for reference:

798 : export function isSingleLeftClick(event) {
799 :   // Note: if you create something draggable, be sure to
800 :   // call it on both `mousedown` and `mousemove` event,
801 :   // otherwise `mousedown` should be enough for a button
802 : 
803 :   if (event.button === undefined && event.buttons === undefined) {
804 :     // Why do we need `buttons` ?
805 :     // Because, middle mouse sometimes have this:
806 :     // e.button === 0 and e.buttons === 4
807 :     // Furthermore, we want to prevent combination click, something like
808 :     // HOLD middlemouse then left click, that would be
809 :     // e.button === 0, e.buttons === 5
810 :     // just `button` is not gonna work
811 : 
812 :     // Alright, then what this block does ?
813 :     // this is for chrome `simulate mobile devices`
814 :     // I want to support this as well
815 : 
816 :     return true;
817 :   }
818 : 
819 :   if (event.button === 0 && event.buttons === undefined) {
820 :     // Touch screen, sometimes on some specific device, `buttons`
821 :     // doesn't have anything (safari on ios, blackberry...)
822 : 
823 :     return true;
824 :   }
825 : 
826 :   // `mouseup` event on a single left click has
827 :   // `button` and `buttons` equal to 0
828 :   if (event.type === 'mouseup' && event.button === 0 &&
829 :       event.buttons === 0) {
830 :     return true;
831 :   }
832 : 
833 :   if (event.button !== 0 || event.buttons !== 1) {
834 :     // This is the reason we have those if else block above
835 :     // if any special case we can catch and let it slide
836 :     // we do it above, when get to here, this definitely
837 :     // is-not-left-click
838 : 
839 :     return false;
840 :   }
841 : 
842 :   return true;
843 : }

[Mikhail1Demianenko]

Any updates?

[misteroneill]

No updates @Mikhail1Demianenko since it was the weekend. I'm not sure when we'll get to this, but it should be a fairly straightforward fix. If you'd like to give it a try and open a PR, that would be a huge help!

[Mikhail1Demianenko]

@misteroneill Sure, I'd be glad to 😊 While going through the tests, noticed that this case must return false:

  const mouseEvent = TestHelpers.createEvent('mousedown');

  // Left mouse click
  mouseEvent.button = 0;
  mouseEvent.buttons = 0;

  assert.notOk(Dom.isSingleLeftClick(mouseEvent), 'a left mouse click on an older browser (Safari) is a single left click');

Here is output after npm test when returning true on mousedown, {0, 0}:

Not sure how to go about it at this point

[gkatsev]

Looking at the history, it seems like {0,0} was set up to account for mousemove events calling through isSingleLeftClick: https://github.com/videojs/video.js/commit/f2aedb72ecf7a4573fe7bede4e6f57fd509bfff2. Maybe the trick is to special case a {0,1} on mousedown like we do for mouseup?

[Mikhail1Demianenko]

@gkatsev well, the {0, 1} case works perfectly regardless of event.type. The problem appears when mac decides to fire mousedown, {0, 0} instead. To account for it, i added the following to the isSingleLeftClick(e):

  if (event.type === 'mousedown' && event.button === 0 && event.buttons === 0) {
    // Mac trackpad touch event that fires both {0, 1} and {0, 0}
    return true;
  }

But the above code breaks the following test case, which has nothing to do with mousemove:

  const mouseEvent = TestHelpers.createEvent('mousedown');

  // Left mouse click
  mouseEvent.button = 0;
  mouseEvent.buttons = 0;

  assert.notOk(Dom.isSingleLeftClick(mouseEvent), 'a left mouse click on an older browser (Safari) is a single left click');

[gkatsev]

Oh, I see, had it in reverse. Yeah, that's a tricky thing, and unfortunately any change here has a potential for breaking things.

It seems like your proposal is basically reverting #6138/https://github.com/videojs/video.js/commit/f2aedb72ecf7a4573fe7bede4e6f57fd509bfff2. Though, doing so may re-break #6132.

Maybe what we want is to change the if to event.type !== 'mousemove' from event.type === 'mouseup'? As for how to change that test, I'm not certain. We'd probably want to make the change and then test it in something like browserstack on various versions of Safari to make sure that it's picking it up correctly.

Unfortunately, due to our wide-browser support, changes like these end up being quite finicky because we may end up breaking things when we fix things.

[gkatsev]

A solution might end up being something like "if older safari, do what we do now, if newer safari do the new thing" and then we'll need to update our tests accordingly as well.

[Mikhail1Demianenko]

@gkatsev unfortunately, event.type !== 'mousemove' instead of event.type === 'mouseup' fails the tests, too You're right, this is becoming way more tricky than I expected, I will give it a proper though a little later 😊

[gkatsev]

yeah, I had meant with the appropriate test changes, which is the tricky part.

[sainttttt]

Are there any updates on this issue? I'm running into this problem to on macOS 13.2 Ventura with a Magic Trackpad 2.

[Mikhail1Demianenko]

@sainttttt if I recall correctly, tests were tricky to fix, so I abandoned the Idea

[sainttttt]

@Mikhail1Demianenko Is what has to be done updating the tests to check for old versions of safari?

[Mikhail1Demianenko]

@sainttttt something along these lines, I think, yes