iamdustan / smoothscroll

Scroll Behavior polyfill
http://iamdustan.com/smoothscroll/
MIT License
3.85k stars 339 forks source link

handle detached elements #148

Closed JohnnyFun closed 4 years ago

JohnnyFun commented 4 years ago

Scenario: I have a call to smoothscroll in a callback. So if I nav away from the page prior to the callback running, the element is no longer attached to the DOM.

JohnnyFun commented 4 years ago

Build error is "Failed at the smoothscroll-polyfill@0.4.4 preminify script." My change is pretty simple, so guessing this is an existing issue?

jeremenichelli commented 4 years ago

@JohnnyFun you actually have linting errors, check the Travis details link.

Besides that, I'm thinking if JavaScript works synchronously how this situation could happen inside the polyfill code, and not actually a check not being done on the outer code.

LIke, if you execute the polyfill's code, nothing can happen between:

var scrollableParent = findScrollableParent(this);

... and the smooth scroll call that would actually solve the issue, I would actually queue the scroll call on your application with requestAnimationFrame and later hard check if the element you want to scroll (or its parent) to exist.

Like:

requestAnimationFrame(() => {
  if (element) element.scrollIntoView({ behavior: 'smooth' });
});

An isolated case working in a codesandbox for example could help a lot :)

JohnnyFun commented 4 years ago

You are correct. Upon starting to build out a codesandbox example I realized my example could be written better to avoid this issue.

I had a function that accepted a wait param and an element parameter like:

export function scrollToEl(element, wait = 0) {
  setTimeout(() => {
    if (element == null) return // but if you pass in an element reference, this closure object could be detached...derp
    el.scrollIntoView({ behavior: 'smooth' }) // native implementation actually handles detached silently...but like you said, there really isn't a reasonable scenario where you should pass it a detached element.
  }, wait)
}

I now just do the setTimeout in the calling code. I'm scrolling to one element, then expand it, then scroll to it again to make sure the top of it is in view. I now check the element is still there directly before calling scrollTo throughout that flow.

I'll close this. Thanks!

Btw, https://codesandbox.io/ is awesome. I didn't know about it.

JohnnyFun commented 4 years ago

To explain further, if you're doing a multi-step scroll flow like the one I mentioned, you'll either want to re-query the dom for your element or, in the case of svelte or react framework, just check that the bound element object is still not null.

Alternatively, if you have a normal dom object reference, you can also check if it's still attached to the dom like so:

function elementInDocument(element) {
    while (true) {
        if (element == null) return false
        if (element === document) return true
        element = element.parentNode
    }
}
jeremenichelli commented 4 years ago

Glad to hear it helped! And yes, codesandbox.io is amazing ✨