MM-14949 Correct scroll when new posts are loaded #2687

sudheerDev · 2019-04-22T21:20:41Z

Summary

Main changes are part of mattermost/react-window#9

Change to scroll correction logic

Previous logic used for correction of scroll is based on mount of every post in virt list which determines if post is added below the fold or above and corrects accordingly.
This has couple of downside mainly because it is hard to distinguish if the change of height is caused by user or the post itself.
i.e collpasing an attachment and change of post height because post header is removed automatically when more posts are loaded because it can be bunched.

These both contradict each other because when collapsing images we have to scroll up in the view. where as when a posts header changes because of new posts we have to keep it in place and move the newly added posts down.

The previous advantage of scroll correction is that it is easy to keep track of changes in heights of posts and keeping to the bottom of postlist is easy. Along with scroll pop correction caused by any random height change because it tries to auto correct itself

New logic works on two main principles for correction

When new posts are loaded use getSnapshotBeforeUpdate and correct based on it.
When new post arrives or scroll pops when at the bottom of channel use handleNewMeasurements from previous logic to keep correcting the scroll position

We still use handleNewMeasurements in a really specific scenario of keeping the scroll position locked when RHS with centre channel not at bottom. To achieve this we need to know the height changes of all the posts above the fold when RHS is opened as posts wrap to multiple lines causing content in center channel to be pushed. We use handleNewMeasurements and force correction when width of posts change i.e like resize of RHS opens.

https://mattermost.atlassian.net/browse/MM-13702
https://mattermost.atlassian.net/browse/MM-14947
https://mattermost.atlassian.net/browse/MM-14948

Absolute position of virt posts to relative positioning

To make changes to scroll correction logic as described above one of key changes was to change from absolute position to relative position because with absolute positioning we don't have a way of checking if all posts are in correct position to take a snapshot for scroll correction. Absolute position also causes weird issues as post overlap etc zoom flickers etc

This also gives us with an option to optimise different parts of the virt list such as measuring items, listening to changes of height changes by delaying them as the content flows with relative positioning and does not cause to overlap posts.

Virtualised lists are always done with absolute positioning and it makes sense only when using fixed height elements as it gives us an option to guess the positions of various elements without even mounting.

https://mattermost.atlassian.net/browse/MM-14831

Change from bottom to top positioning

It is hard to lock the scroll position when there are frequent messages coming into in virt list that is because if we have bottom positioning it tends to move to bottom and we have to intentionally correct to lock position. With changing it to top positioning we don't have to correct to lock but we have to correct to bottom when needed.

This also enables us to use the default browser behaviour of keeping scroll locked when content height below the fold changes so there is no extra logic of adding scroll corrections when an attachment is collapsed

https://mattermost.atlassian.net/browse/MM-14946
https://mattermost.atlassian.net/browse/MM-14949

esethna · 2019-04-22T23:21:44Z

To do: https://community-daily.mattermost.com/core/pl/8skheibzofrj8cdxjw6nuwz7oe

* Check for state change in posts instead of props * Correct scroll when posts change at top or when header changes * use snapshot for taking height before updating postslist

sudheerDev · 2019-04-29T18:41:33Z

components/post_view/post_list.jsx

@@ -116,10 +130,41 @@ export default class PostList extends React.PureComponent {
        window.addEventListener('resize', this.handleWindowResize);
    }

-    componentDidUpdate(prevProps) {
+    getSnapshotBeforeUpdate(prevProps, prevState) {


I was unable to add a test case for this @saturninoabril any suggestions?

One approach is to mock return value of postlistRef and watch for the snapshot at componentDidUpdate whenever it's called. Maybe something like,

test('should return previous scroll position from getSnapshotBeforeUpdate', () => { const wrapper = shallow(<PostList {...baseProps}/>); const instance = wrapper.instance(); instance.componentDidUpdate = jest.fn(); instance.postlistRef = {current: {scrollTop: 10, scrollHeight: 100}}; wrapper.setState({atEnd: true}); expect(instance.componentDidUpdate).toHaveBeenCalledTimes(1); expect(instance.componentDidUpdate.mock.calls[0][2]).toEqual({previousScrollTop: 10, previousScrollHeight: 100}); instance.postlistRef = {current: {scrollTop: 30, scrollHeight: 200}}; wrapper.setState({atEnd: false}); expect(instance.componentDidUpdate).toHaveBeenCalledTimes(2); expect(instance.componentDidUpdate.mock.calls[1][2]).toEqual({previousScrollTop: 30, previousScrollHeight: 200}); // may add test combination of state changes necessary in computing getSnapshotBeforeUpdate });

Added test cases but as discussed offline downgrading enzyme because of enzymejs/enzyme#2027

enahum · 2019-04-29T18:56:39Z

Hey @sudheerDev I think you need to rebase this branch in order for the CircleCI stuff runs

sudheerDev · 2019-04-29T18:57:43Z

@enahum I rebased it today

enahum

non-blocking comments

enahum · 2019-04-29T18:57:13Z

components/post_view/post_list.jsx

@@ -25,6 +26,18 @@ const OVERSCAN_COUNT_BACKWARD = window.OVERSCAN_COUNT_BACKWARD || 50; // Exposin
 const OVERSCAN_COUNT_FORWARD = window.OVERSCAN_COUNT_FORWARD || 100; // Exposing the value for PM to test will be removed soon
 const HEIGHT_TRIGGER_FOR_MORE_POSTS = window.HEIGHT_TRIGGER_FOR_MORE_POSTS || 1000; // Exposing the value for PM to test will be removed soon

+const postListHeightChangeForPadding = 21;


can you add a comment as of why this value is 21.

nvm, saw the comment below

nit: maybe move this constant closer to postListStyle.

I moved the postListStyle closer as postListHeightChangeForPadding is an integer assignment i wanted it to be at the top

enahum · 2019-04-29T18:59:13Z

components/post_view/post_list.jsx

@@ -196,7 +241,9 @@ export default class PostList extends React.PureComponent {
        if (error) {
            if (this.autoRetriesCount < MAX_NUMBER_OF_AUTO_RETRIES) {
                this.autoRetriesCount++;
-                this.loadMorePosts();
+                debounce(() => {


I think you can write this as

debounce(this.loadMorePosts);

Yup true. Removed the arrow func

enahum · 2019-04-29T19:01:25Z

@sudheerDev then I don't know why is it reporting a build failure

saturninoabril

Awesome 🎉, added comments but non-blocking.

saturninoabril · 2019-04-29T22:30:44Z

components/post_view/post_list.jsx

@@ -25,6 +26,18 @@ const OVERSCAN_COUNT_BACKWARD = window.OVERSCAN_COUNT_BACKWARD || 50; // Exposin
 const OVERSCAN_COUNT_FORWARD = window.OVERSCAN_COUNT_FORWARD || 100; // Exposing the value for PM to test will be removed soon
 const HEIGHT_TRIGGER_FOR_MORE_POSTS = window.HEIGHT_TRIGGER_FOR_MORE_POSTS || 1000; // Exposing the value for PM to test will be removed soon

+const postListHeightChangeForPadding = 21;


nit: maybe move this constant closer to postListStyle.

saturninoabril · 2019-04-29T23:15:03Z

components/post_view/post_list.jsx

@@ -116,10 +130,41 @@ export default class PostList extends React.PureComponent {
        window.addEventListener('resize', this.handleWindowResize);
    }

-    componentDidUpdate(prevProps) {
+    getSnapshotBeforeUpdate(prevProps, prevState) {


One approach is to mock return value of postlistRef and watch for the snapshot at componentDidUpdate whenever it's called. Maybe something like,

test('should return previous scroll position from getSnapshotBeforeUpdate', () => { const wrapper = shallow(<PostList {...baseProps}/>); const instance = wrapper.instance(); instance.componentDidUpdate = jest.fn(); instance.postlistRef = {current: {scrollTop: 10, scrollHeight: 100}}; wrapper.setState({atEnd: true}); expect(instance.componentDidUpdate).toHaveBeenCalledTimes(1); expect(instance.componentDidUpdate.mock.calls[0][2]).toEqual({previousScrollTop: 10, previousScrollHeight: 100}); instance.postlistRef = {current: {scrollTop: 30, scrollHeight: 200}}; wrapper.setState({atEnd: false}); expect(instance.componentDidUpdate).toHaveBeenCalledTimes(2); expect(instance.componentDidUpdate.mock.calls[1][2]).toEqual({previousScrollTop: 30, previousScrollHeight: 200}); // may add test combination of state changes necessary in computing getSnapshotBeforeUpdate });

saturninoabril · 2019-04-29T23:16:36Z

components/post_view/post_list.test.jsx

@@ -152,4 +153,13 @@ describe('PostList', () => {
            expect(wrapper.state('atEnd')).toEqual(true);
        });
    });
+
+    describe('onItemsRendered', () => {
+        test('should set state atBottom false when visibleStopIndex is not 0', async () => {


Might not need to be async

* Downgrade enzyme

sudheerDev · 2019-04-30T08:57:06Z

components/file_attachment/__snapshots__/file_attachment.test.jsx.snap

@@ -9,8 +9,17 @@ exports[`component/FileAttachment should match snapshot, after change from file
    href="#"
    onClick={[Function]}
  >
-    <div
-      className="post-image__load"


This should be the snapshot. This was caused by the enzyme issue with 3.9.0 enzymejs/enzyme#2027

sudheerDev changed the title ~~Mm 14949~~ MM-14949 Correct scroll when new posts are loaded Apr 22, 2019

sudheerDev added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Apr 22, 2019

amyblais added this to the v5.12.0 milestone Apr 23, 2019

jasonblais assigned esethna Apr 23, 2019

sudheerDev added the Setup Old Test Server Triggers the creation of a test server label Apr 24, 2019

sudheerDev force-pushed the MM-14949 branch from 74d07b0 to 8ff6fd8 Compare April 25, 2019 22:34

sudheerDev removed the Setup Old Test Server Triggers the creation of a test server label Apr 25, 2019

sudheerDev force-pushed the MM-14949 branch from 8ff6fd8 to 7fce367 Compare April 25, 2019 22:36

sudheerDev added the Setup Old Test Server Triggers the creation of a test server label Apr 25, 2019

sudheerDev force-pushed the MM-14949 branch from 7fce367 to a803ea5 Compare April 29, 2019 10:21

sudheerDev removed the Setup Old Test Server Triggers the creation of a test server label Apr 29, 2019

sudheerDev force-pushed the MM-14949 branch from 094e543 to 0512bd0 Compare April 29, 2019 10:26

MM-14949 Correct scroll when new posts are loaded

904e300

* Check for state change in posts instead of props * Correct scroll when posts change at top or when header changes * use snapshot for taking height before updating postslist

sudheerDev force-pushed the MM-14949 branch from 0512bd0 to 904e300 Compare April 29, 2019 15:08

sudheerDev added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Apr 29, 2019

update window hash

e3fa852

sudheerDev force-pushed the MM-14949 branch from 81dfc3e to e3fa852 Compare April 29, 2019 16:52

sudheerDev added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Apr 29, 2019

sudheerDev force-pushed the MM-14949 branch from fa56bd0 to 6edf3fd Compare April 29, 2019 17:17

Remove test case for snapshot

4b9e817

sudheerDev force-pushed the MM-14949 branch from 6edf3fd to 4b9e817 Compare April 29, 2019 17:24

sudheerDev added the Setup Old Test Server Triggers the creation of a test server label Apr 29, 2019

esethna removed the 1: PM Review Requires review by a product manager label Apr 29, 2019

esethna approved these changes Apr 29, 2019

View reviewed changes

esethna requested review from enahum and saturninoabril April 29, 2019 17:59

sudheerDev added 2: Dev Review Requires review by a core commiter and removed Work in Progress Not yet ready for review labels Apr 29, 2019

sudheerDev commented Apr 29, 2019

View reviewed changes

enahum approved these changes Apr 29, 2019

View reviewed changes

saturninoabril approved these changes Apr 29, 2019

View reviewed changes

saturninoabril added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Apr 29, 2019

sudheerDev added 3 commits April 30, 2019 13:54

* Address review comments

eb9fde6

* Downgrade enzyme

Remove uneeded arrow func for debounce

deca543

Update snapshot

e3c6be1

sudheerDev commented Apr 30, 2019

View reviewed changes

sudheerDev removed the Setup Old Test Server Triggers the creation of a test server label Apr 30, 2019

saturninoabril approved these changes Apr 30, 2019

View reviewed changes

sudheerDev merged commit 9f47208 into master Apr 30, 2019

sudheerDev deleted the MM-14949 branch April 30, 2019 09:13

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 16, 2019

ogi-m added the Tests/Done Release tests have been written label May 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-14949 Correct scroll when new posts are loaded #2687

MM-14949 Correct scroll when new posts are loaded #2687

sudheerDev commented Apr 22, 2019 •

edited

esethna commented Apr 22, 2019

sudheerDev Apr 29, 2019

saturninoabril Apr 29, 2019

sudheerDev Apr 30, 2019

enahum commented Apr 29, 2019

sudheerDev commented Apr 29, 2019

enahum left a comment

enahum Apr 29, 2019

saturninoabril Apr 29, 2019

sudheerDev Apr 30, 2019 •

edited

enahum Apr 29, 2019

sudheerDev Apr 30, 2019

enahum commented Apr 29, 2019

saturninoabril left a comment

saturninoabril Apr 29, 2019

saturninoabril Apr 29, 2019

saturninoabril Apr 29, 2019

sudheerDev Apr 30, 2019

sudheerDev Apr 30, 2019

MM-14949 Correct scroll when new posts are loaded #2687

MM-14949 Correct scroll when new posts are loaded #2687

Conversation

sudheerDev commented Apr 22, 2019 • edited

Summary

Change to scroll correction logic

Absolute position of virt posts to relative positioning

Change from bottom to top positioning

esethna commented Apr 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enahum commented Apr 29, 2019

sudheerDev commented Apr 29, 2019

enahum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudheerDev Apr 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enahum commented Apr 29, 2019

saturninoabril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudheerDev commented Apr 22, 2019 •

edited

sudheerDev Apr 30, 2019 •

edited