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

Fixes scroll lock detection in ShadowDOM #49

Merged
merged 3 commits into from
Feb 6, 2016
Merged

Conversation

cdata
Copy link
Contributor

@cdata cdata commented Jan 15, 2016

Scroll locking was broken in ShadowDOM because event.path does not
contain sufficient information when currentTarget is window.
Listeners are now being added to document instead.

@freshp86
Copy link

I just sent a similar PR (see #50). This change alone does not fix it for me. The _scrollInteractionHandler() method needs to also be modified to use event.path instead of event.target.

@@ -199,13 +199,13 @@
document.body.style.overflowY = 'hidden';

// Modern `wheel` event for mouse wheel scrolling:
window.addEventListener('wheel', this._scrollInteractionHandler, true);
document.addEventListener('wheel', this._scrollInteractionHandler, true);

Choose a reason for hiding this comment

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

Don't we need to update also the removeEventListener code a few lines below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@cdata
Copy link
Contributor Author

cdata commented Jan 15, 2016

@freshp86 Thanks for the contribution in #50. I will incorporate the extra changes needed in this PR, if that's okay with you.

@cdata cdata force-pushed the fix-scroll-lock-detection branch 2 times, most recently from 043e44e to 91419f4 Compare January 15, 2016 21:19
@freshp86
Copy link

Thanks. That is fine by me.

childTwo = parent.querySelector('#ChildTwo');
grandChildOne = parent.querySelector('#GrandchildOne');
grandChildTwo = parent.querySelector('#GrandchildTwo');
childOne = Polymer.dom(parent.root).querySelector('#ChildOne');
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is just parent.$$.ChildOne, which is less verbose, maybe? (here and below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@notwaldorf
Copy link
Contributor

@cdata this is lgtm once you do those changes :)

@freshp86
Copy link

@cdata: The PR is LGTMed already. Can you provide a status update?

Chris Joel added 3 commits February 5, 2016 16:18
Scroll locking was broken in ShadowDOM because `event.path` does not
contain sufficient information when `currentTarget` is `window`.
Listeners are now being added to `document` instead.
cdata added a commit that referenced this pull request Feb 6, 2016
@cdata cdata merged commit 39af26e into master Feb 6, 2016
@cdata cdata deleted the fix-scroll-lock-detection branch February 6, 2016 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants