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 blocking all scrolling when RemoveScroll is used within ShadowRoot #98

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

csorfab
Copy link
Contributor

@csorfab csorfab commented Sep 23, 2023

This PR fixes issue #45 as per the discussion in #97

@csorfab csorfab mentioned this pull request Sep 23, 2023
@theKashey
Copy link
Owner

Looks brilliant 👍 . Give me some time to test it

@@ -90,6 +90,9 @@ export const handleScroll = (
let availableScrollTop = 0;

do {
if (target instanceof ShadowRoot) {
Copy link
Owner

Choose a reason for hiding this comment

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

"when" this happens?
Description of commit is about "mobile", but how it's related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the event comes from within a ShadowRoot, then the target's .parentNode() up the tree will eventually be the ShadowRoot, and we need to step out of it with .host (because a ShadowRoot's parentNode is null). I'm not exactly sure why this fixes it, but touch scrolling doesn't work without this in my use case. I already wanted to do this regardless, because it seems like the "right" thing to do with regards to ShadowDOM, but it also ended up fixing the issue on mobile.

@theKashey theKashey merged commit 0fe6cf9 into theKashey:master Oct 3, 2023
@theKashey
Copy link
Owner

Sorry for delay. Found a few issues with math and it took some time to find the root cause.
The problem was not with your change, but with floating point calculations if page is a little scaled, and my page was a little scaled 😅

@theKashey
Copy link
Owner

Released as a part of v 2.5.7

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.

2 participants