Fix issues in scrollable pane#4055
Conversation
|
@ThomasMichon are you ok with this? |
| } | ||
| }, 1); | ||
| } | ||
| this.notifySubscribers(); |
There was a problem hiding this comment.
I think you should just move this call into the timeout.
c7c112e to
9176014
Compare
|
Re-wording the description here for clarity for future reference: It's possible to enter an infinite recursion in ScrollablePane: The solution to this was to update This, however, unfurled a different bug in ScrollablePane which the infinite loop had masked: the top sticky header wouldn't properly be placed in |
Pull request checklist
$ npm run changeDescription of changes
It's possible to enter an infinite recursion in scrollable pane. notifySubscribers is called from _onWindowResize, which is called from forceLayoutUpdate (callable from the parent item). This calls Sticky::_onScrollEvent, which updates the Sticky's state. Sticky::componentDidUpdate can call scrollablePane.addStickyHeader, which calls _addSticky, which then calls notifySubscribers, infinitely recursing. The solution to this was to update Sticky::_onScrollEvent to not check the current of conditions of isStickyTop and isStickyBottom, since it was causing them to constantly flip, causing setState to constantly get called.
This, however, unfurled a different bug in ScrollablePane which the infinite loop had masked: the top sticky header wouldn't properly be placed in stickyAbove on creation of the ScrollablePane, but the constant changing of isStickyTop and isStickyBottom eventually caused it to snap in place. By getting rid of stickyContainer and moving stickyAbove and stickyBelow to siblings of the children of the pane, this is fixed (it also removed the need for resizeContainer).
Focus areas to test
Scrollable Pane