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

[Aegz]

Hey guys,

I noticed there was a bug with the WindowScroller which stopped it from properly mounting the scroll and resize handlers to a new scrollElement if it is passed in via props. This is also considerably worse with multiple WindowScrollers and I want to fix that too.

Example: Single WindowScroller but we change the scrollElement from window to a div.
https://codepen.io/willhua/pen/zYZwygO?editors=1111

The current workaround I have seen people using is to have a key prop on the WindowScroller which forcibly remounts the whole component therefore bypassing the logic in the componentDidUpdate lifecycle handler which normally deals with unmounting the old handlers and mounting the new ones.

E.g.

<WindowScroller scrollElement={someElement} key={someElement}>

I noticed this issue also happens with multiple WindowScrollers since when you change the scrollElement on multiple WindowScrollers this series of events occurs:

1. WindowScroller 1 mounts to window - attaches scroll/resize listeners
2. WindowScroller 2 mounts to window - does nothing
3. WindowScroller 1 prop change to a div - Fails to mount the listener to the new scrollElement because the props on the Components have already changed so this condition fails. This is most likely because we simply store references to the components themselves in mountedInstances
4. WindowScroller 2 prop change to a div - does not unmount old scroll/resize listeners due to existing mountedInstances (i.e. !mountedInstances.length is never true)

The problem lies here in unregisterScrollListener:

react-virtualized/source/WindowScroller/utils/onScroll.js

Line 84 in abe0530

mountedInstances = mountedInstances.filter(

The problem in this particular example is that we remove WindowScroller 1 then immediately add it back to that array straight away in componentDidUpdate on line 158:

react-virtualized/source/WindowScroller/WindowScroller.js

Lines 146 to 163 in abe0530

	componentDidUpdate(prevProps: Props, prevState: State) {
	const {scrollElement} = this.props;
	const {scrollElement: prevScrollElement} = prevProps;
	if (
	prevScrollElement !== scrollElement &&
	prevScrollElement != null &&
	scrollElement != null
	) {
	this.updatePosition(scrollElement);
	unregisterScrollListener(this, prevScrollElement);
	registerScrollListener(this, scrollElement);
	this._unregisterResizeListener(prevScrollElement);
	this._registerResizeListener(scrollElement);
	}
	}

Once we get around to updating WindowScroller 2 we fail this condition in unregisterScrollListener and never unmount the old listeners:

react-virtualized/source/WindowScroller/utils/onScroll.js

Line 87 in abe0530

if (!mountedInstances.length) {

I also noticed that with multiple WindowScrollers we weren't unmounting the handlers provided from detectElementResize.js since the scrollListener reference we are attempting to unmount is not the same reference as the one used to mount it (i.e. we have a closure which builds a scrollListener function then we attempt to unmount from a different closure with a new scrollListener function).

See:

react-virtualized/source/vendor/detectElementResize.js

Line 194 in abe0530

element.addEventListener('scroll', scrollListener, true);

Also:

react-virtualized/source/vendor/detectElementResize.js

Line 224 in abe0530

element.removeEventListener('scroll', scrollListener, true);

The fixes:

onScroll.js - Instead of just storing a reference to a Component, we store the Component along with the Element we bound the listeners to { component, element }. That way when we change the Component (the props update before we get into the registerScrollListener and unregisterScrollListener calls which is why they fail at the moment) we instead avoid the prop comparison which is unsafe and failure prone and instead compare against the Element reference we stored against the Component and if there are no known WindowScrollers registered against the component we remove the listener.

detectElementResize.js - This fails to remove the scroll listener attached to the element if there are multiple instances of the closure and in our case if there is more than 1 WindowScroller then we have multiple instances. The problem that occurs here is that if we unmount one of the other WindowScrollers the removeEventListener call fails to do anything because the scrollListener referenced there is generated in each individual closure and will NOT match the one currently bound to the element.

Video for some more evidence:
https://youtu.be/3GOi2yL88Qk

Also, sorry but I had to add enzyme to unit test the prop changing.

[yamadapc]

Closes #1256