ashthornton / asscroll

Ash's Smooth Scroll 🍑
MIT License
945 stars 27 forks source link

How to properly remove event listener on window.object? #82

Open sebastianjung opened 2 years ago

sebastianjung commented 2 years ago

Hey there,

first of all thank you for the awesome library. Was actually the only one i found which fits the need of the project i'm working on.

I'm using it in conjuction with nuxt 2 / vue 2 and having a hard time making it work correctly.

Reproducing the error:

  1. Entering the site first time: Scroll works as expected
  2. going to another page where asscroll is not imported and used. Then revisit the site with asscroll: Scroll is double the speed
  3. going back in browser once more and revisit the same page once more: scroll is tripled
  4. repeat

For further information you might want to check out the bug yourself here: https://front.ausbildung.stage.sma.pippis.zone/berufsbilder-und-studiengaenge

Click one of the items and go back in browser history and revisit one item.

When i looked into it more i found out that the event listeners on the window object are not removed when using asscroll.disable() or asscroll.off('update', someRandomFunction)

Instead they get duplicated for each revisit of the site.

Is this intended behaviour? And If so: How to properly disconnect the scroll listener on window object on site-leave / beforeDestroy()

Any help is very much appreciated.

And again: Thank you for the library!

Sebastian

EDIT: Some more info: In mounted() i init asscroll with this function:

initAsscrollAndAnimations() {
      import('@ashthornton/asscroll').then((ASScroll) => {
        const constructor = ASScroll.default
        this.asscroll = new constructor({
          containerElement: this.$refs.HorizontalScrollContainer.$el,
          disableRaf: true
        })

        // use gsap.ticker instead of request animation frame
        gsap.ticker.add(this.asscroll.update)

        // setup scrolltrigger scroller proxy
        ScrollTrigger.scrollerProxy(this.asscroll.containerElement, {
          scrollLeft: (value) => {
            return arguments.length
              ? (this.asscroll.currentPos = value)
              : this.asscroll.currentPos
          },
          getBoundingClientRect: () => {
            return {
              top: 0,
              left: 0,
              width: window.innerWidth,
              height: window.innerHeight
            }
          }
        })

        // add event listener on asscroll
        this.asscroll.on('update', this.onAsscrollUpdate)
        // add resize listeners
        ScrollTrigger.addEventListener('refresh', this.asscroll.resize)

        /*
          place gsap animations here
        */
        //  STICKY APPLY NOW BTN
        if (this.jobProfile.attributes.allowApplication) {
          // get apply now button
          this.createStickyFlyOutAnimation()
        }
        // PARALLAXY FLOATING ITEMS
        this.createParallaxAnimations()
        // RELATED JOB PROFILES SECTION
        if (this.hasJobProfileRelatedJobProfiles) {
          this.createRelatedSectionPopoutAnimation()
        }
        /* 
          end gsap animations
        */
        this.asscroll.enable({
          horizontalScroll: true
        })
      })
    },

I think the gsap stuff can be ignored as uncommenting it and using asscroll with its own RAF does not solve the problem.

ashthornton commented 2 years ago

Hey @sebastianjung, glad you're enjoying the lib :)

It sounds like you're creating a new instance of ASScroll every time you navigate to another page. The idea is to create a single instance that you then disable and enable accordingly, passing your new page element to the enable() function when ready.

Here is an example using the PJAX library Highway which should explain the same concept: https://codepen.io/ashthornton/project/editor/AypMLG

Let me know if this helps.

sebastianjung commented 2 years ago

@ashthornton Thank you very much for looking into this.

sadly i was not able to import the library as it throws "self is not defined" error.

I am only able to use the library when the app is already mounted via an import('@ashthornton/ass...) statement.

Not sure what is causing this issue but i can't get it to work.

I tried a workaround where i initted only one instance and reuse this between revitis of the page using the scroll but that also leads to unexpected behaviour:

Before i was putting the containerElement inside the options object on instance creation. But now i have to put it inside asscroll.enable({ newScrollElements: '...' }) if I'm not mistaken.

Changing this leads to the content not being visible at times. Second half shows the behaviour: https://sharing.clickup.com/clip/p/t4635687/9889adad-eb7e-4a49-91a6-2755680bbe8a/screen-recording-2022-04-29-08:27.webm

Noticable things:

  1. On revisiting the page, content is not visible until i uncheck "contain: content"
  2. It seems to make the content jump in some cases when scrolling ended. Like it went from horizontal to vertical scrolling.

This is my code now: On mounted i call this:

async initAsscrollAndAnimations() {
      if (!this.$asscroll.instance) {
        await import('@ashthornton/asscroll').then((ASScroll) => {
          console.log('init asscroll', this.$asscroll)

          const constructor = ASScroll.default
          this.$asscroll.instance = new constructor({
            // containerElement: this.$refs.HorizontalScrollContainer.$el,
            disableRaf: true
          })
        })
      }

      // use gsap.ticker instead of request animation frame
      gsap.ticker.add(this.$asscroll.instance.update)

      // add event listener on asscroll
      this.$asscroll.instance.on('update', this.onAsscrollUpdate)

      // setup scrolltrigger scroller proxy
      ScrollTrigger.scrollerProxy(this.$asscroll.instance.containerElement, {
        scrollLeft: (value) => {
          return arguments.length
            ? (this.$asscroll.instance.currentPos = value)
            : this.$asscroll.instance.currentPos
        },
        getBoundingClientRect: () => {
          return {
            top: 0,
            left: 0,
            width: window.innerWidth,
            height: window.innerHeight
          }
        }
      })

      // add resize listeners
      ScrollTrigger.addEventListener('refresh', this.$asscroll.instance.resize)

      /*
        place gsap animations here
      */
      //  STICKY APPLY NOW BTN
      if (this.jobProfile.attributes.allowApplication) {
        // get apply now button
        this.createStickyFlyOutAnimation()
      }
      // PARALLAXY FLOATING ITEMS
      this.createParallaxAnimations()
      // RELATED JOB PROFILES SECTION
      if (this.hasJobProfileRelatedJobProfiles) {
        this.createRelatedSectionPopoutAnimation()
      }

      /*
        end gsap animations
      */

      this.$asscroll.instance.enable({
        newScrollElements: this.$refs.HorizontalScrollContainer.$el,
        horizontalScroll: true,
        reset: true
      })
    },

and before i change route i call this:

beforeDestroy() {
    gsap.ticker.remove(this.$asscroll.instance.update)
    ScrollTrigger.removeEventListener('refresh', this.$asscroll.instance.resize)
    ScrollTrigger.getAll().forEach((t) => {
      t.kill()
    })
    this.$asscroll.instance.off('update', this.onAsscrollUpdate)
    this.$asscroll.instance.disable()
  },

Appreciate your help a lot! Let me know if you need any more info on that.

EDIT: Is containerElement the same element as newScrollElements and the same as the one having asscroll-container attribute applied to it? I'm confused on whether i need asscroll-container attribute + newScrollElements at the same time.

sebastianjung commented 2 years ago

@ashthornton It seems to work (asscroll works as expected whereas normal scroll on other pages is not working anymore) when i set the asscroll-container to an element that does not get destroyed (like the most outer div inside a layout file of the website). So maybe there is a possibility to reset the containerElement somehow? It's only a getter on the instance right now. Would it make sense to provide a setter or am i using the library wrong here?

The thing that's conflicting in my head is the following: Instance should stay alive on route changes to not duplicate window event listeners. OK got it. But then containerElement on which the instance is mounted on gets destroyed on route change.

Wouldn't it be more logical to destroy the instance as well or provide a possibility to reset the instance values somehow? (containerElement).

I'm probably overseeing things here.

Sorry to bother you!