Bug fix: Changing scrollElement prop on multiple WindowScrollers doesn't mount new scroll handlers

package.json

@@ -74,14 +74,15 @@
     "@babel/core": "^7.7.2",
     "@babel/plugin-external-helpers": "^7.2.0",
     "@babel/plugin-proposal-class-properties": "^7.7.0",
+    "@babel/plugin-transform-flow-comments": "^7.12.13",
     "@babel/plugin-transform-modules-commonjs": "^7.7.0",
     "@babel/plugin-transform-runtime": "^7.6.2",
-    "@babel/plugin-transform-flow-comments": "^7.12.13",
     "@babel/polyfill": "^7.7.0",
     "@babel/preset-env": "^7.7.1",
     "@babel/preset-flow": "^7.0.0",
     "@babel/preset-react": "^7.7.0",
     "@babel/preset-stage-2": "^7.0.0",
+    "@wojtekmaj/enzyme-adapter-react-17": "^0.6.1",
     "autoprefixer": "^9.7.1",
     "babel-eslint": "^10.0.3",
     "babel-jest": "^24.9.0",
@@ -92,6 +93,7 @@
     "codemirror": "^5.49.2",
     "cross-env": "^6.0.3",
     "css-loader": "^3.2.0",
+    "enzyme": "^3.11.0",
     "eslint": "^6.6.0",
     "eslint-config-fbjs": "^3.1.1",
     "eslint-config-prettier": "^6.5.0",

source/WindowScroller/WindowScroller.jest.js

@@ -1,3 +1,4 @@
+import {mount} from 'enzyme';
 import * as React from 'react';
 import {findDOMNode} from 'react-dom';
 import {render} from '../TestUtils';
@@ -473,4 +474,140 @@ describe('WindowScroller', () => {
       );
     });
   });
+
+  describe('when scrollElement prop changes', () => {
+    let scrollElementDiv;
+    let addEventListener;
+    let removeEventListener;
+
+    const generateWindowScrollerWrapper = scrollElement => {
+      return mount(
+        <WindowScroller scrollElement={scrollElement}>
+          {params => <div>&nbsp;</div>}
+        </WindowScroller>,
+      );
+    };
+
+    beforeEach(() => {
+      scrollElementDiv = document.createElement('div');
+
+      // Spy on the add/remove listener on this element so we can determine how and when it is used
+      addEventListener = jest.spyOn(scrollElementDiv, 'addEventListener');
+      removeEventListener = jest.spyOn(scrollElementDiv, 'removeEventListener');
+
+      // JSDom doesn't implement a working getBoundingClientRect()
+      // But WindowScroller requires it
+      mockGetBoundingClientRectForHeader({
+        documentOffset: 0,
+        height: 0,
+        width: 0,
+      });
+    });
+
+    it('should re-mount new scroll handlers if the scrollElement changes (single WindowScroller)', async () => {
+      const windowScroller = generateWindowScrollerWrapper(scrollElementDiv);
+
+      // detectElementResize && unregisterScrollListener
+      expect(addEventListener).toHaveBeenCalledTimes(2);
+
+      windowScroller.setProps({scrollElement: undefined}); // Change to window
+      windowScroller.update();
+
+      // detectElementResize && unregisterScrollListener
+      expect(removeEventListener).toHaveBeenCalledTimes(2);
+
+      simulateWindowScroll({scrollX: 0, scrollY: 5000});
+
+      // Allow scrolling timeout to complete so that the component computes state
+      await new Promise(resolve => setTimeout(resolve, 150));
+
+      expect(windowScroller.state('scrollTop')).toEqual(5000);
+
+      windowScroller.unmount();
+    });
+
+    it('should do nothing if you change a different prop', () => {
+      const windowScroller = generateWindowScrollerWrapper(scrollElementDiv);
+
+      // detectElementResize && unregisterScrollListener
+      expect(addEventListener).toHaveBeenCalledTimes(2);
+
+      windowScroller.setProps({onScroll: jest.fn()}); // Change random prop
+      windowScroller.update();
+
+      // detectElementResize && unregisterScrollListener
+      expect(removeEventListener).toHaveBeenCalledTimes(0);
+
+      windowScroller.unmount();
+    });
+
+    it('should retain existing handlers if they are still used (multiple WindowScrollers on different elements)', async () => {
+      const windowScrollerOnWindow = generateWindowScrollerWrapper(window);
+
+      const windowScrollerThatChanges = generateWindowScrollerWrapper(
+        scrollElementDiv,
+      );
+      const windowScrollerOnDivElement = generateWindowScrollerWrapper(
+        scrollElementDiv,
+      );
+
+      // detectElementResize && unregisterScrollListener
+      expect(addEventListener).toHaveBeenCalledTimes(2);
+
+      // Change from div to window
+      windowScrollerThatChanges.setProps({
+        scrollElement: undefined,
+      });
+
+      windowScrollerThatChanges.update();
+      windowScrollerOnDivElement.update();
+
+      // No unmounting because we still have WindowScrollers attached to the div
+      expect(removeEventListener).toHaveBeenCalledTimes(0);
+
+      simulateWindowScroll({scrollX: 0, scrollY: 5000});
+
+      // Allow scrolling timeout to complete so that the component computes state
+      await new Promise(resolve => setTimeout(resolve, 150));
+
+      expect(windowScrollerOnWindow.state('scrollTop')).toEqual(5000);
+      expect(windowScrollerThatChanges.state('scrollTop')).toEqual(5000);
+      expect(windowScrollerOnDivElement.state('scrollTop')).toEqual(0);
+
+      windowScrollerOnWindow.unmount();
+      windowScrollerThatChanges.unmount();
+      windowScrollerOnDivElement.unmount();
+    });
+
+    it('should remove old scroll handlers if the scrollElement changes (multiple WindowScrollers)', async () => {
+      const windowScroller1 = generateWindowScrollerWrapper(scrollElementDiv);
+      const windowScroller2 = generateWindowScrollerWrapper(scrollElementDiv);
+      const windowScroller3 = generateWindowScrollerWrapper(scrollElementDiv);
+
+      // detectElementResize && unregisterScrollListener added to divElement
+      expect(addEventListener).toHaveBeenCalledTimes(2);
+
+      windowScroller3.setProps({scrollElement: undefined});
+      windowScroller2.setProps({scrollElement: undefined});
+      windowScroller1.setProps({scrollElement: undefined});
+
+      windowScroller1.update();
+      windowScroller2.update();
+      windowScroller3.update();
+
+      // detectElementResize && unregisterScrollListener should be unmounted from divElement
+      expect(removeEventListener).toHaveBeenCalledTimes(2);
+
+      simulateWindowScroll({scrollX: 0, scrollY: 5000});
+
+      // Allow scrolling timeout to complete so that the component computes state
+      await new Promise(resolve => setTimeout(resolve, 150));
+
+      expect(windowScroller1.state('scrollTop')).toEqual(5000);
+
+      windowScroller1.unmount();
+      windowScroller2.unmount();
+      windowScroller3.unmount();
+    });
+  });
 });

source/WindowScroller/utils/onScroll.js

@@ -7,7 +7,13 @@ import {
 } from '../../utils/requestAnimationTimeout';
 import type WindowScroller from '../WindowScroller.js';
 
-let mountedInstances = [];
+// Checking a component's props is unreliable. Instead we store the element used at the time
+type MountedInstancesReference = {
+  component: WindowScroller,
+  element: Element,
+};
+
+let mountedInstances: MountedInstancesReference[] = [];
 let originalBodyPointerEvents = null;
 let disablePointerEventsTimeoutId = null;
 
@@ -25,7 +31,7 @@ function enablePointerEventsIfDisabled() {
 
 function enablePointerEventsAfterDelayCallback() {
   enablePointerEventsIfDisabled();
-  mountedInstances.forEach(instance => instance.__resetIsScrolling());
+  mountedInstances.forEach(mi => mi.component.__resetIsScrolling());
 }
 
 function enablePointerEventsAfterDelay() {
@@ -34,10 +40,10 @@ function enablePointerEventsAfterDelay() {
   }
 
   var maximumTimeout = 0;
-  mountedInstances.forEach(instance => {
+  mountedInstances.forEach(mi => {
     maximumTimeout = Math.max(
       maximumTimeout,
-      instance.props.scrollingResetTimeInterval,
+      mi.component.props.scrollingResetTimeInterval,
     );
   });
 
@@ -58,9 +64,9 @@ function onScrollWindow(event: Event) {
     document.body.style.pointerEvents = 'none';
   }
   enablePointerEventsAfterDelay();
-  mountedInstances.forEach(instance => {
-    if (instance.props.scrollElement === event.currentTarget) {
-      instance.__handleWindowScrollEvent();
+  mountedInstances.forEach(mi => {
+    if (mi.component.props.scrollElement === event.currentTarget) {
+      mi.component.__handleWindowScrollEvent();
     }
   });
 }
@@ -69,22 +75,24 @@ export function registerScrollListener(
   component: WindowScroller,
   element: Element,
 ) {
-  if (
-    !mountedInstances.some(instance => instance.props.scrollElement === element)
-  ) {
+  if (!mountedInstances.some(mi => mi.element === element)) {
     element.addEventListener('scroll', onScrollWindow);
   }
-  mountedInstances.push(component);
+
+  mountedInstances.push({component, element});
 }
 
 export function unregisterScrollListener(
   component: WindowScroller,
   element: Element,
 ) {
-  mountedInstances = mountedInstances.filter(
-    instance => instance !== component,
-  );
-  if (!mountedInstances.length) {
+  // Remove the given component from our known instances
+  mountedInstances = mountedInstances.filter(mi => mi.component !== component);
+
+  if (
+    !mountedInstances.length || // If current length is 0, remove listener
+    !mountedInstances.some(mi => mi.element === element) // No current mounted instances have the element
+  ) {
     element.removeEventListener('scroll', onScrollWindow);
     if (disablePointerEventsTimeoutId) {
       cancelAnimationTimeout(disablePointerEventsTimeoutId);

source/jest-setup.js

@@ -1,3 +1,8 @@
+import {configure} from 'enzyme';
+import Adapter from '@wojtekmaj/enzyme-adapter-react-17';
+
+configure({adapter: new Adapter()});
+
 jest.mock('dom-helpers/scrollbarSize', () => {
   return function getScrollbarSize() {
     return 20;

source/vendor/detectElementResize.js

@@ -192,6 +192,8 @@ export default function createDetectElementResize(nonce, hostWindow) {
         element.appendChild(element.__resizeTriggers__);
         resetTriggers(element);
         element.addEventListener('scroll', scrollListener, true);
+        // Store reference to original instantiated scrollListener so it can be unbound later
+        element.__scrollListener__ = scrollListener;
 
         /* Listen for a css animation to detect element display/re-attach */
         if (animationstartevent) {
@@ -221,7 +223,14 @@ export default function createDetectElementResize(nonce, hostWindow) {
         1,
       );
       if (!element.__resizeListeners__.length) {
-        element.removeEventListener('scroll', scrollListener, true);
+        if (element.__scrollListener__) {
+          element.removeEventListener(
+            'scroll',
+            element.__scrollListener__,
+            true,
+          );
+        }
+
         if (element.__resizeTriggers__.__animationListener__) {
           element.__resizeTriggers__.removeEventListener(
             animationstartevent,