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.

Sign up for GitHub

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

Jump to bottom

improve: Better mouse position handling #5773

Merged

gkatsev merged 6 commits into videojs:main from iosamuel:master

Aug 31, 2020

Contributor

iosamuel commented Jan 31, 2019 •

edited

Loading

Description

It is an enhancement for mouse position handling, this will fix the issues when you scale the elements using transform instead of the default font-size scaling provided by videojs, it will work since we will use the local offset position of an element.

Specific Changes proposed

We use offsetX and offsetY of the event, so we get the local position of the element, this way we can transform:scale elements without worrying that the position will change drastically.
In the stylesheet we make the mouse events on the progress-holder children go off, because we wanna work only with the position of the progress itself, this way we avoid issues getting the offset of the inner elements, such as vjs-load-progress and such.

Requirements Checklist

Feature implemented / Bug fixed
Change has been verified in an actual browser (Chome, Firefox, IE)
Unit Tests updated or fixed
Reviewed by Two Core Contributors


          improve: Better mouse position handling

5c5089f

welcome bot commented Jan 31, 2019

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.


          use changedTouches if available

1d8b781

gkatsev reviewed

View reviewed changes

Member

gkatsev left a comment

And more discussion over in slack :)

src/css/components/_progress.scss Outdated Show resolved Hide resolved

iosamuel added 2 commits

January 31, 2019 12:40


          use offsetPositions and offsetDimensions to get positions in sliders

eac77b4


          no longer need to remove events from progress children

4871b29

iosamuel mentioned this pull request

improve: Better mouse position handling Knowbly-Archives/video.js#1

Merged

Contributor Author

iosamuel commented Feb 12, 2019

Hey @gkatsev, do you have any comment on this one?

brandonocasey added the needs: testing label

stale bot commented Jun 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot added the outdated label

gkatsev removed the outdated label

gkatsev self-assigned this

stale bot commented Aug 17, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot added the outdated label

gkatsev removed the outdated label

stale bot commented Oct 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot added the outdated label

stale bot closed this

gkatsev reopened this

stale bot removed the outdated label

stale bot commented Jan 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot added the outdated label

gkatsev removed the outdated label

stale bot commented Mar 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot added the outdated label

gkatsev removed the outdated label

stale bot commented May 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot added the outdated label

stale bot closed this

gkatsev reopened this

stale bot removed the outdated label

gkatsev mentioned this pull request

getPointerPosition returns wrong position if the container is transformed #6726

Closed

Member

gkatsev commented Aug 28, 2020

It's been a while. Seems to be working fine. Would be great if we can figure out how to fix mobile but this is an improvement still. I'll figure out how to fix the conflict and merge.

gkatsev changed the base branch from master to main

August 28, 2020 19:20

gkatsev added 2 commits

August 28, 2020 15:21


          Merge branch 'main' into master

547c873


          Update progress-control.js

92db3ea

gkatsev approved these changes

View reviewed changes

gkatsev added needs: LGTM and removed needs: testing labels

Member

gkatsev commented Aug 28, 2020 •

edited

Loading

This fixes #6726 and #1102 and the caveat is that it still doesn't work with touch events.

brandonocasey approved these changes

View reviewed changes

gkatsev merged commit 0b425be into videojs:main

Member

gkatsev commented Aug 31, 2020

Oops, the commit has the wrong issue number.

gkatsev pushed a commit that referenced this pull request


          fix: Better mouse position handling (#5773)

c4c8fc1

This uses offsetX and offsetY on the MouseEvents which helps account for transforms on the player. Unfortunately, this isn't available on TouchEvents, so, while this helps desktop devices with using a mouse, it doesn't help mobile devices using touch.

Fixes #6726, fixes #1102.

Member

gkatsev commented Aug 31, 2020

Fixed it.

gkatsev mentioned this pull request

Vertical volume bug in version 7.9.5 #6863

Closed

gkatsev added a commit that referenced this pull request


          fix(dom): vertical getPointerPosition value

53c2380

From my understand, in the changes #5773, the Y position of all the
boxes is already calculated and accounted for in the offsetY value we
get. However, because the HTML coordinate system has Y=0 at the top,
when we do offsetY/boxH, we get a position relative to the top of the
element. However, we expect that position relative to the "start" of the
slider, or the bottom of it. Therefore, we want to get the inverse
value, which is '1 - clamp(offsetY/boxH)'.

Fixes #6863

gkatsev mentioned this pull request

fix(dom): vertical getPointerPosition value #6864

Merged

gkatsev added a commit that referenced this pull request


          fix(dom): vertical getPointerPosition value (#6864)

From my understand, in the changes #5773, the Y position of all the
boxes is already calculated and accounted for in the offsetY value we
get. However, because the HTML coordinate system has Y=0 at the top,
when we do offsetY/boxH, we get a position relative to the top of the
element. However, we expect that position relative to the "start" of the
slider, or the bottom of it. Therefore, we want to get the inverse
value, which is '1 - clamp(offsetY/boxH)'.

Fixes #6863

brandonocasey mentioned this pull request

fix: play progress time tooltip from jittering during live #6968

Merged

gkatsev mentioned this pull request

Volume slider sticky and jumps on Chrome #6989

Closed

nochev mentioned this pull request

Progress bar time tooltip out of bounds on the right #7248

Closed

gkatsev mentioned this pull request

Fix mouse time tooltip when moved to far right of progress bar #7308

Open

7 tasks

misteroneill removed the needs: LGTM label

edirub pushed a commit to edirub/video.js that referenced this pull request


          fix: Better mouse position handling (videojs#5773)

2327b04

This uses offsetX and offsetY on the MouseEvents which helps account for transforms on the player. Unfortunately, this isn't available on TouchEvents, so, while this helps desktop devices with using a mouse, it doesn't help mobile devices using touch.

Fixes videojs#6726, fixes videojs#1102.

edirub pushed a commit to edirub/video.js that referenced this pull request


          fix(dom): vertical getPointerPosition value (videojs#6864)

94174a9

From my understand, in the changes videojs#5773, the Y position of all the
boxes is already calculated and accounted for in the offsetY value we
get. However, because the HTML coordinate system has Y=0 at the top,
when we do offsetY/boxH, we get a position relative to the top of the
element. However, we expect that position relative to the "start" of the
slider, or the bottom of it. Therefore, we want to get the inverse
value, which is '1 - clamp(offsetY/boxH)'.

Fixes videojs#6863

harisha-swaminathan mentioned this pull request

fix: time tooltip truncated #8527

Merged

5 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet