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

Conversation

Projects
None yet
4 participants
@kocoten1992
Contributor

kocoten1992 commented Sep 13, 2017

hi @gkatsev , this can't be fix from slider-component alone, some logic was on seek-bar and volume-bar, best preventing from there. #4611

I'm not familiar with git, do I need to do this again for v5.x ?

@kocoten1992

This comment has been minimized.

Show comment
Hide comment
@kocoten1992

kocoten1992 Sep 13, 2017

Contributor

don't merge that yet, there are potential bug with with volume control with that code

Contributor

kocoten1992 commented Sep 13, 2017

don't merge that yet, there are potential bug with with volume control with that code

@kocoten1992

This comment has been minimized.

Show comment
Hide comment
@kocoten1992

kocoten1992 Sep 14, 2017

Contributor

Will test on real device tomorrow

Contributor

kocoten1992 commented Sep 14, 2017

Will test on real device tomorrow

safari touchscreen button both 0 and undefined
touchscreen safari ios
sometime: ```button```: undefined, ```buttons```: undefined
other time: ```button```: 0, ```buttons```: undefined

```buttons``` on safari actually expected behavior, https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons

Note: we do need ```buttons``` since we not allow middle button click to be able to seek (that would be: ```button``` 0, ```buttons``` 4)
@gkatsev

code-wise, looks good.
I'd like to test it locally before merging.

Thanks!

@gkatsev gkatsev added the patch label Sep 19, 2017

@kocoten1992

This comment has been minimized.

Show comment
Hide comment
@kocoten1992

kocoten1992 Sep 19, 2017

Contributor

@gkatsev , thank you 👍

Allowed me to have few more days, need to be test on blackberry and android, and there is a very subtle bug (may not need to be fix), will report in few days

Contributor

kocoten1992 commented Sep 19, 2017

@gkatsev , thank you 👍

Allowed me to have few more days, need to be test on blackberry and android, and there is a very subtle bug (may not need to be fix), will report in few days

@kocoten1992

This comment has been minimized.

Show comment
Hide comment
@kocoten1992

kocoten1992 Sep 21, 2017

Contributor

It work on blackberry, android, and ios, think I nailed it.

.video-js .vjs-slider:focus {
    text-shadow: 0em 0em 1em white;
    -webkit-box-shadow: 0 0 1em #fff;
    -moz-box-shadow: 0 0 1em #fff;
    box-shadow: 0 0 1em #fff;
}

Previously vjs-slider never have a chance to show up, but now it does, it pop a white box-shadow, I'll leave this for vjs team.

Contributor

kocoten1992 commented Sep 21, 2017

It work on blackberry, android, and ios, think I nailed it.

.video-js .vjs-slider:focus {
    text-shadow: 0em 0em 1em white;
    -webkit-box-shadow: 0 0 1em #fff;
    -moz-box-shadow: 0 0 1em #fff;
    box-shadow: 0 0 1em #fff;
}

Previously vjs-slider never have a chance to show up, but now it does, it pop a white box-shadow, I'll leave this for vjs team.

@gkatsev

Looks good! Thanks.

kocoten1992 added some commits Oct 24, 2017

must exist on both mousedown and mousemove
These code block exist on both mousedown and mousemove will guarantee to work consistent between browser

(Bizarre bug on chrome when right click on seek bar, then left click on any where on screen will seek at position parallel to seek bar)

Some changes still necessary

@gkatsev gkatsev removed the confirmed label Oct 30, 2017

@kocoten1992

This comment has been minimized.

Show comment
Hide comment
@kocoten1992

kocoten1992 Nov 11, 2017

Contributor

I'm continuing this, will

  • rermove my old code in volume and seek bar
  • create isSingleLeftClick in dom.js
  • use isSingleLeftClick in both handleMouseDown and handleMouseMove of both volume and seek bar
Contributor

kocoten1992 commented Nov 11, 2017

I'm continuing this, will

  • rermove my old code in volume and seek bar
  • create isSingleLeftClick in dom.js
  • use isSingleLeftClick in both handleMouseDown and handleMouseMove of both volume and seek bar

kocoten1992 added some commits Nov 12, 2017

updated, needs a re-review.

@misteroneill

One tiny thing.

Show outdated Hide outdated src/js/utils/dom.js
@gkatsev

LGTM

@gkatsev gkatsev added needs: LGTM and removed needs: updates labels Nov 16, 2017

@misteroneill misteroneill added confirmed and removed needs: LGTM labels Nov 16, 2017

@gkatsev gkatsev merged commit 79b4355 into videojs:master Nov 16, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment