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: changes 'mousedown' to the 'mouseup' event in the player #5992

Merged
merged 22 commits into from
May 21, 2019

Conversation

gjanblaszczyk
Copy link
Member

Description

All buttons in the player use 'mouseup' instead of 'mousedown' event.
This player behavior is very useful for accessibility because the user can cancel the action by clicking outside the button area.
However, in the player, there is one place that the player uses a 'mousedown' instead of 'mouseup' event. I think that is just a mistake.
You can check it by clicking on the video area during the playback. You will see that the playback pauses/resumes after 'mousedown' event but if you click on the play/pause button over the controlbar it uses 'mouseup' event.
The WCAG 2.1 "2.5.2 Pointer Cancellation" spec. also suggests using "mouseup" instead of "mousedown" event.

Specific Changes proposed

I changed "mousedown' to 'mouseup" event inside the player.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@gjanblaszczyk gjanblaszczyk changed the title Changes 'mousedown' to the 'mouseup' event in the player Fix: changes 'mousedown' to the 'mouseup' event in the player May 13, 2019
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Yeah, switching to mouseup is probably best unless it ends up breaking something, though, I can't imagine it will.

src/js/utils/dom.js Show resolved Hide resolved
@gkatsev gkatsev requested a review from OwenEdwards May 17, 2019 20:22
@gkatsev gkatsev added the a11y This item might affect the accessibility of the player label May 17, 2019
@OwenEdwards
Copy link
Member

Looks like you also need to change the comment/documentation at:

* @listens Tech#mousedown

@Listens Tech#mouseup

Copy link
Member

@OwenEdwards OwenEdwards left a comment

Choose a reason for hiding this comment

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

LGTM

@gkatsev gkatsev merged commit 075a504 into videojs:master May 21, 2019
@gkatsev
Copy link
Member

gkatsev commented May 21, 2019

Thanks for helping improve the accessibility of Video.js @gjanblaszczyk!

@gjanblaszczyk
Copy link
Member Author

Thanks @gkatsev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y This item might affect the accessibility of the player
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants