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

[JW8-10549] Fix the controls not autohiding on mobile #3536

Merged
merged 1 commit into from Nov 26, 2019

Conversation

@mamaddox
Copy link
Contributor

mamaddox commented Nov 22, 2019

This PR will...

  • Fix the issue where controls were not automatically hiding when the user was inactive for 4 seconds on mobile.

Why is this Pull Request needed?

  • Bug fix

Are there any points in the code the reviewer needs to double check?

Are there any Pull Requests open in other repos which need to be merged with this?

Addresses Issue(s):

JW8-10549

Checklist

  • Jenkins builds and unit tests are passing
  • I have reviewed the automated results
@mamaddox mamaddox requested a review from robwalch as a code owner Nov 22, 2019
@jnatalzia

This comment has been minimized.

Copy link
Collaborator

jnatalzia commented Nov 22, 2019

@mamaddox the changes here make sense but could i get a quick explainer of the issue underlying it? was the initial mouse handler causing some unintentional state with regards to the controls hiding?

@jwplayer-robot

This comment has been minimized.

Copy link

jwplayer-robot commented Nov 22, 2019

⚠️ MULTI Build for commit 5c6baa3 is unstable (FAILURE).
🏗 jwplayer build SUCCESS
🏗 jwplayer unit tests SUCCESS
🏗 jwplayer-commercial build SUCCESS
🏗 jwplayer-commercial unit tests SUCCESS
🥒.js Allure Report FAILURE
🥒 Allure Report UNSTABLE
🍆 Manual Tests
📺 Views

@mamaddox

This comment has been minimized.

Copy link
Contributor Author

mamaddox commented Nov 22, 2019

As far as my understanding goes, this issue was occurring only if the user selected a control in the control bar after hitting play, but before the first userInactive event occurred.
When this happened, a mousemove event was triggered inside the controlbar that set the activeTimeout to zero and didn't invoke the userInactiveTimeout. However, when the user tapped the screen, the mouseevent listeners were removed so things went back to normal. It seemed like those event listeners were not needed since they always got removed whenever the screen was tapped.

But now that I'm thinking about it, is there a case where mobile needs both mouse and tap events?

@jnatalzia

This comment has been minimized.

Copy link
Collaborator

jnatalzia commented Nov 22, 2019

But now that I'm thinking about it, is there a case where mobile needs both mouse and tap events?

I can't think of one, especially if we were immediately removing those listeners on tap anyway.

@jnatalzia

This comment has been minimized.

Copy link
Collaborator

jnatalzia commented Nov 22, 2019

Hmm thinking about it, there's the possibility that older devices don't support the touch events and therefore would never hit the execution path where they are removed.

Support looks really good across mobile browsers though so I don't think this concern is justified
https://developer.mozilla.org/en-US/docs/Web/API/Touch_events

@mamaddox mamaddox merged commit 0070a2f into master Nov 26, 2019
2 of 3 checks passed
2 of 3 checks passed
jw7-pr-multi-opensource Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mamaddox mamaddox added this to the 8.12.0 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.