Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues in scrollable pane #4055

Merged
merged 6 commits into from
Mar 1, 2018

Conversation

MaxLustig
Copy link
Contributor

@MaxLustig MaxLustig commented Feb 21, 2018

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description 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

@dzearing
Copy link
Member

@ThomasMichon are you ok with this?

@@ -167,7 +167,6 @@ export class ScrollablePaneBase extends BaseComponent<IScrollablePaneProps, {}>
}
}, 1);
}
this.notifySubscribers();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should just move this call into the timeout.

@MaxLustig MaxLustig changed the title Fix infinite recursion in scrollable pane Fix issues in scrollable pane Feb 27, 2018
@MaxLustig MaxLustig force-pushed the maxlus/stickyContainer branch from c7c112e to 9176014 Compare February 28, 2018 00:03
@ThomasMichon ThomasMichon merged commit 5c53827 into microsoft:master Mar 1, 2018
chrismohr pushed a commit to chrismohr/office-ui-fabric-react that referenced this pull request Apr 17, 2018
* Fix infinite recursion in scrollable pane

* Rush change

* Fix root cause of infinite loop

* Fixed issue with sticky headers not being added to stickyAbove

* Remove ref for stickyContainer

* Fix bad rebase
@elliottsj
Copy link
Contributor

elliottsj commented Apr 19, 2018

Re-wording the description here for clarity for future reference:

It's possible to enter an infinite recursion in ScrollablePane:

     forceLayoutUpdate()
->   _onWindowResize()
-> * notifySubscribers()
->   Sticky::_onScrollEvent()
->   (React state update)
->   Sticky::componentDidUpdate()
->   scrollablePane.addStickyHeader()
->   _addSticky()
-> * notifySubscribers()
->   ...

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).

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants