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: Control-bar autohide when cursor placed over it #5258 #5692

Merged
merged 7 commits into from
Dec 26, 2018

Conversation

xjoaoalvesx
Copy link
Contributor

@xjoaoalvesx xjoaoalvesx commented Dec 17, 2018

Description

Solves #5258

Specific Changes proposed

Adds two event handlers for 'mouseenter' and 'mouseleave' events on the control-bar.
When the 'mouseenter' is triggered the inactivityTimeout is set to 0 and on 'mouseleave' is set to the default value (2s).

Expected behavior:

  • cursor stops inside control bar -> sets timeout 0s so nothing happens
  • cursor leaves player bounds -> sets timeout 2s before going inactive

Requirements Checklist

  • Feature implemented / Bug fixed
  • Change has been verified in an actual browser (Chrome, Firefox)
  • Tests cases passed
  • Reviewed by Two Core Contributors

Handles the 'mouseenter' and 'mouseleave' events when triggered in the control-bar
Closes videojs#5258
Added test for expected behaviour of the player/control-bar when the events are triggered
@welcome
Copy link

welcome bot commented Dec 17, 2018

💖 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 for the PR!
I wonder if a better way of doing this would be to save the interval id and the handler and then clear the interval on mouseenter and then recreate it on mouseleave? Though, storing the previous inactivity timeout would be easiest.

src/js/player.js Outdated
});

this.on(this.getChild('controlBar'), 'mouseleave', function(event) {
this.options_.inactivityTimeout = 2000;
Copy link
Member

Choose a reason for hiding this comment

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

if someone set a custom inactivity timeout, this will reset that value.

Copy link
Member

Choose a reason for hiding this comment

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

We could store the inactivityTimeout in this.cache_ and then reset to that value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I now save the value in the cache object on mouseenter and retrieve it on mouseleave

Save previous timeout value in cache_ and retrieves it on mouseleave
now using the player's cache_ object to store inactivityTimeout value
now with the changes using the player's cache object
src/js/player.js Outdated
@@ -3444,6 +3444,18 @@ class Player extends Component {
this.on('mousemove', handleMouseMove);
this.on('mouseup', handleMouseUp);

this.on(this.getChild('controlBar'), 'mouseenter', function(event) {
Copy link
Member

Choose a reason for hiding this comment

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

we should store the control bar object in a variable (to also re-use below) and do a null-check on it.

Additionally, it would be better to call .on directly on the contrlBar itself as the handler will automatically be removed when the control bar is disposed. I don't think the current event handler gets cleared up automatically, currently.

Copy link
Contributor Author

@xjoaoalvesx xjoaoalvesx Dec 18, 2018

Choose a reason for hiding this comment

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

Edit: Actually, I think I understand what you mean: Besides creating a local variable to reuse and test for null on the player, we should create separate methods on the controlBar class to add the handlers and call them from the player.

Copy link
Member

Choose a reason for hiding this comment

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

Basically, this.on(anotherComponent, 'mouseenter', () => {}) vs anotherComponent.on('mouseenter', () => {}). I am unsure that the first one will have the mouseenter event removed when the component is disposed where-as I'm certain the second one will be done properly.

Ultimately, the code should end up something like:

const controlBar = this.getChild('controlBar');

if (controlBar) {
  controlBar.on('mouseenter', (event) => /* ... */);
  controlBar.on('mouseleave', (event) => /* ... */);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely understand what you say so now we can see that on the code.

@gkatsev gkatsev added the patch This PR can be added to a patch release. label Dec 18, 2018
event handlers for mouseenter and mouseleave are now added to the controlBar itself
@gkatsev gkatsev added the needs: LGTM Needs one or more additional approvals label Dec 19, 2018
@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Dec 19, 2018
@gkatsev gkatsev merged commit 6ebc772 into videojs:master Dec 26, 2018
@welcome
Copy link

welcome bot commented Dec 26, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@xjoaoalvesx xjoaoalvesx deleted the my-branch branch December 28, 2018 11:01
@xjoaoalvesx xjoaoalvesx restored the my-branch branch December 28, 2018 11:01
@xjoaoalvesx xjoaoalvesx deleted the my-branch branch December 28, 2018 11:01
@xjoaoalvesx xjoaoalvesx restored the my-branch branch December 28, 2018 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants