Navigation Menu

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

UI preventScrolling - Only capture subsequent events if event target matches #3322

Closed
wants to merge 2 commits into from

Conversation

pajong
Copy link
Contributor

@pajong pajong commented Mar 9, 2019

This PR will...

Call setPointerCapture to ui elements with preventScrolling options only if the event target matches the ui element.

Why is this Pull Request needed?

"preventScrolling" option on desktop calls el.setPointerCapture, resulting the upcoming events to be only captured in that element.
Since _wrapperElement is the element (which contains controls), any events after pointerdown on controls that are forwarded to _wrapperElement are ignored, making the controls not clickable.

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-5643

@pajong pajong requested a review from robwalch March 9, 2019 04:29
@johnBartos
Copy link
Contributor

johnBartos commented Mar 9, 2019

Warnings
⚠️

🛠 There are modified src files, but no test changes. Add tests if you're able to.

⚠️

🔎 Assign some reviewers or assignees.

⚠️

🗿 Set a milestone. It should be the ticket's fix version in JIRA.

Generated by 🚫 dangerJS

@jwplayer-robot
Copy link

MULTI Build for commit d9484ce passed.
🏗️ jwplayer build SUCCESS
🏗️ jwplayer browserstack tests SUCCESS
🏗️ jwplayer-commercial build SUCCESS
🏗️ jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

Copy link
Contributor

@robwalch robwalch left a comment

Choose a reason for hiding this comment

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

Preventing pointer clicks on the element when you add preventScrolling is an issue with ui.js, and should be fixed in ui.js.

We want to prevent scrolling on all touch devices, not just the ones with useragent strings that match our OS.mobile check. We should either update the pointer event handling or use a different element for dragging.

@pajong pajong changed the title Add mobile check back for preventScrolling Only set pointer capture if event target matches Mar 11, 2019
@pajong pajong changed the title Only set pointer capture if event target matches Only capture subsequent pointer events if event target matches ui element Mar 11, 2019
@pajong pajong changed the title Only capture subsequent pointer events if event target matches ui element UI preventScrolling - Only capture subsequent events if event target matches Mar 11, 2019
@pajong pajong requested a review from robwalch March 11, 2019 15:24
@jwplayer-robot
Copy link

MULTI Build for commit b9f37e6 passed.
🏗️ jwplayer build SUCCESS
🏗️ jwplayer browserstack tests SUCCESS
🏗️ jwplayer-commercial build SUCCESS
🏗️ jwplayer-commercial browserstack tests SUCCESS
🥒 Automated Tests SUCCESS
🍆 Manual Tests
📺 Views

@@ -109,7 +109,7 @@ function initInteractionListeners(ui) {

removeHandlers(ui, WINDOW_GROUP);
if (type === 'pointerdown' && e.isPrimary) {
if (!passive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work everywhere?

I was thinking we defer pointer capture https://github.com/jwplayer/jwplayer/compare/wip/ui-pointer-capture-when-dragging?expand=1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robwalch I think defer is cleaner and safer. I will close this PR

@pajong pajong closed this Mar 11, 2019
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.

None yet

4 participants