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

Add userAction.click to prevent pause/play when player is clicked #7495

Merged
merged 4 commits into from
Nov 10, 2021

Conversation

rberger
Copy link
Contributor

@rberger rberger commented Nov 2, 2021

Description

Over the ages, many people have requested the ability to make it so clicking on the player does NOT toggle pause/play but all the other nice features such as still controlling the video via the control-bar and having the controls fade in and out based on pointer actions still work.

The most recent issue was

This is a proposed PR to fix and possibly:

The pointer-events: none; CSS suggestion does not do the same as it disables the show/hide of the control bar and other impacts.

Specific Changes proposed

Add a userAction for the single click event to override the pause/play action.

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 (Chrome, Firefox, IE)
      • [x]Tested on macOS Chrome, Firefox and Safari
    • Unit Tests updated or fixed
      • I tried to write some tests, but I am not familiar with the testing tools and failed. Would be great if someone familiar with the test setup could do it.
    • Docs/guides updated
      • Updated the Guide
    • Example created (starter template on JSBin)
      • /sandbox/userAction-click.html.example
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Nov 2, 2021

💖 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.

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.

Thanks!

It's definitely been a well requested feature. Making it a user action makes total sense.

Are you able to add some tests? Should be in here https://github.com/videojs/video.js/blob/main/test/unit/player-user-actions.test.js#L8

Also, can you revert the package-lock.json changes?

@@ -443,6 +444,40 @@ Defines the order in which Video.js techs are preferred. By default, this means

> Type: `Object`

### `userActions.click`
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for updating the documentation!

src/js/player.js Outdated
Comment on lines 1938 to 1941
this.options_ === undefined ||
this.options_.userActions === undefined ||
this.options_.userActions.click === undefined ||
this.options_.userActions.click !== false
Copy link
Member

Choose a reason for hiding this comment

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

nit: spacing

Suggested change
this.options_ === undefined ||
this.options_.userActions === undefined ||
this.options_.userActions.click === undefined ||
this.options_.userActions.click !== false
this.options_ === undefined ||
this.options_.userActions === undefined ||
this.options_.userActions.click === undefined ||
this.options_.userActions.click !== false

src/js/player.js Outdated
Comment on lines 1945 to 1947
this.options_ !== undefined &&
this.options_.userActions !== undefined &&
typeof this.options_.userActions.click === 'function'
Copy link
Member

Choose a reason for hiding this comment

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

nit: spacing

Suggested change
this.options_ !== undefined &&
this.options_.userActions !== undefined &&
typeof this.options_.userActions.click === 'function'
this.options_ !== undefined &&
this.options_.userActions !== undefined &&
typeof this.options_.userActions.click === 'function'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I rarely edit javascript and recently switched to doom emacs and don't have the auto indention working right yet.

Fixed the Spacing and reverted the package-lock.json.

I spent a couple hours trying to write the unit tests and it wasn't working out. Like I said, I don't normally write javascript and am not familiar with the test framework. And unfortunately this was already some deep Yak shaving by the Bike Shed at the end of a big detour trying to get contorted aspect ratios and css layout working on my main task that I'm already weeks behind on.

Maybe there is someone lurking who is familiar with the test framework could add that? Conceptually it should be a trivial variation on the userAction.doubeClick tests. But I was not having any luck with that.

Copy link
Member

Choose a reason for hiding this comment

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

I'll see if I can get a few minutes to write a test or two tomorrow.

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #7495 (aadd959) into main (6faad26) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7495      +/-   ##
==========================================
- Coverage   79.72%   79.61%   -0.11%     
==========================================
  Files         116      116              
  Lines        7296     7306      +10     
  Branches     1754     1763       +9     
==========================================
  Hits         5817     5817              
- Misses       1479     1489      +10     
Impacted Files Coverage Δ
src/js/player.js 86.64% <0.00%> (-0.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6faad26...aadd959. Read the comment docs.

@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Nov 10, 2021
@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label Nov 10, 2021
@gkatsev gkatsev merged commit 749105d into videojs:main Nov 10, 2021
@welcome
Copy link

welcome bot commented Nov 10, 2021

Congrats on merging your first pull request! 🎉🎉🎉

gkatsev added a commit that referenced this pull request Nov 10, 2021
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
…ed (videojs#7495)

Pass `false` as `userAction.click` to disable the default click-to-play behavior. Alternatively, pass in a function, to enable custom behavior.

Fixes videojs#7123.
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants