Fix scroll doesn't reset to top #115

quixotically · 2017-01-31T18:59:18Z

Description

This fixes the bug where if you were to scroll to the bottom (or anywhere below the top), then resize the screen bigger until there is no scrollbar, the content used to not scroll up as the 'clientHeight' was increasing.

Motivation and Context

Fixes #87

How Has This Been Tested?

Tested manually by sizing the screen smaller to create a scrollbar, scrolling to bottom, then increasing the size until there is no scrollbar. Did this several times and used logging to ensure that bad state was not happening.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

quixotically · 2017-01-31T19:03:00Z

src/FixedDataTable.js

+      var maxScrollTop = scrollContentHeight - bodyHeight;
+      // Handle the case where the scrollTop is beyond the maxScrollTop, such as when the user is completely scrolled
+      // down and resizes the viewport to be smaller vertically
+      if (scrollTop > maxScrollTop || scrollTop === 0) {


Unfortunately I couldn't come up with a way to do the scrollTop === 0 once. If the 0 logic is removed, scrolling slowly back up can cause the scrollTop to never update all the way back to 0. There may be a better way, but for now this seems to work well enough.

KamranAsif · 2017-02-01T16:27:15Z

There seems to be a bug with your zero condition. If I open the dev server and open the JSON Data example, I end up starting from the bottom without doing anything

KamranAsif · 2017-02-01T16:29:24Z

Another big bug is when I resize the page, I end up jumping to the bottom of a large table. I expect to not change my first index

KamranAsif · 2017-02-02T16:53:17Z

src/FixedDataTable.js

+    // This calculation is synonymous to Element.scrollTop
+    var scrollTop = firstRowIndex && Math.abs(firstRowOffset - this._scrollHelper.getRowPosition(firstRowIndex));
+    if (scrollTop >= 0) {
+      var maxScrollTop = maxScrollY ? scrollContentHeight - bodyHeight : 0;


maxScrollY is already this value. We don't need to recalculate maxScrollTop

Ah true, removed maxScrollTop

KamranAsif · 2017-02-02T16:57:38Z

src/FixedDataTable.js

@@ -1056,6 +1056,21 @@ var FixedDataTable = React.createClass({

    this._scrollHelper.setViewportHeight(bodyHeight);

+    // This calculation is synonymous to Element.scrollTop
+    var scrollTop = firstRowIndex && Math.abs(firstRowOffset - this._scrollHelper.getRowPosition(firstRowIndex));


Don't use && as an if statement

Changed to
var scrollTop = firstRowIndex ? Math.abs(firstRowOffset - this._scrollHelper.getRowPosition(firstRowIndex)) : 0;

Was also able to get rid of the following if statement (line 1061) because of this

KamranAsif · 2017-02-02T16:59:07Z

src/FixedDataTable.js

+      var maxScrollTop = maxScrollY ? scrollContentHeight - bodyHeight : 0;
+      // Handle the case where the scrollTop is beyond the maxScrollTop, such as when the user is completely scrolled
+      // down and resizes the viewport to be smaller vertically
+      if (scrollTop > maxScrollTop || maxScrollTop === 0) {


What case does maxScrollTop === zero catch?

Got it! Originally I couldn't come up with a way to do the maxScrollTop === 0 once. Before, if the 0 logic was removed, scrolling slowly back up can cause the scrollTop to never update all the way back to 0. At the time I couldn't figure out how to do the check so that we only set back to 0 once.

But I found out how to do it. I have the previous scrollY, so:
if (scrollTop > maxScrollY || (oldState && oldState.scrollY !== 0 && maxScrollY === 0)) {

See in my newest commit.

KamranAsif · 2017-02-03T16:38:03Z

src/FixedDataTable.js

@@ -1056,6 +1056,19 @@ var FixedDataTable = React.createClass({

    this._scrollHelper.setViewportHeight(bodyHeight);

+    // This calculation is synonymous to Element.scrollTop
+    var scrollTop = firstRowIndex ? Math.abs(firstRowOffset - this._scrollHelper.getRowPosition(firstRowIndex)) : 0;


firstRowIndex should always exist. you can get rid of the if statement here.
Also, is the absolute value necessary? When would we get a negative value here

KamranAsif · 2017-02-06T14:23:06Z

src/FixedDataTable.js

@@ -1056,6 +1056,19 @@ var FixedDataTable = React.createClass({

    this._scrollHelper.setViewportHeight(bodyHeight);

+    // This calculation is synonymous to Element.scrollTop
+    var scrollTop = firstRowIndex != null ? Math.abs(firstRowOffset - this._scrollHelper.getRowPosition(firstRowIndex)) : 0;


firstRowIndex is always defined, you don't need the null check

Ah, thanks. Removed

KamranAsif · 2017-02-06T14:23:24Z

src/FixedDataTable.js

+    // Handle the case where the scrollTop is beyond the maxScrollY, such as when the user is completely scrolled
+    // down and resizes the viewport to be smaller vertically. The other case is when resizing the viewport large enough
+    // so that a scrollbar is not necessary anymore, to make sure we set the scrollTop back to 0.
+    if (scrollTop > maxScrollY || (oldState && oldState.scrollY !== 0 && maxScrollY === 0)) {


Use scrollY instead of oldState.scrollY. We might have modified it by the time we reach this line

Not necessary anymore because of your above comment

KamranAsif · 2017-02-06T14:40:49Z

src/FixedDataTable.js

@@ -1056,6 +1056,19 @@ var FixedDataTable = React.createClass({

    this._scrollHelper.setViewportHeight(bodyHeight);


After some investigation, it seems the real bug is this line. We update the viewport height after having calculated the rows. This makes our clamping operation invalid.

I think all we really need to do is check if scrollTop === scrollY.

KamranAsif · 2017-02-06T14:41:34Z

src/FixedDataTable.js

+    var scrollTop = firstRowIndex != null ? Math.abs(firstRowOffset - this._scrollHelper.getRowPosition(firstRowIndex)) : 0;
+    // Handle the case where the scrollTop is beyond the maxScrollY, such as when the user is completely scrolled
+    // down and resizes the viewport to be smaller vertically. The other case is when resizing the viewport large enough
+    // so that a scrollbar is not necessary anymore, to make sure we set the scrollTop back to 0.


There is really only one case here. Your second comment about resizing the viewport large enough that the scrollbar isn't necessary necessitates that we hit the first case, of having scrollTop being beyond maxScrollY

Fix scroll doesn't reset to top

b63338a

quixotically commented Jan 31, 2017

View reviewed changes

Fix logic to prevent page from immediately scrolling ot bottom

6ca124a

KamranAsif suggested changes Feb 2, 2017

View reviewed changes

Felicia Wong added 2 commits February 3, 2017 11:05

Address comments

ad88b0e

Fix conditional

cfbfa34

KamranAsif suggested changes Feb 6, 2017

View reviewed changes

Address comments

21963ae

KamranAsif merged commit b2c4e2c into schrodinger:master Feb 6, 2017

quixotically deleted the SS-18521-ScrollDoesntResetToTop branch February 6, 2017 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scroll doesn't reset to top #115

Fix scroll doesn't reset to top #115

quixotically commented Jan 31, 2017

quixotically Jan 31, 2017

KamranAsif commented Feb 1, 2017

KamranAsif commented Feb 1, 2017

KamranAsif Feb 2, 2017

quixotically Feb 3, 2017

KamranAsif Feb 2, 2017

quixotically Feb 3, 2017

KamranAsif Feb 2, 2017

quixotically Feb 3, 2017

KamranAsif Feb 3, 2017

KamranAsif Feb 6, 2017

quixotically Feb 6, 2017

KamranAsif Feb 6, 2017

quixotically Feb 6, 2017

KamranAsif Feb 6, 2017

KamranAsif Feb 6, 2017

quixotically Feb 6, 2017

		@@ -1056,6 +1056,19 @@ var FixedDataTable = React.createClass({

		this._scrollHelper.setViewportHeight(bodyHeight);

Fix scroll doesn't reset to top #115

Fix scroll doesn't reset to top #115

Conversation

quixotically commented Jan 31, 2017

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

KamranAsif commented Feb 1, 2017

KamranAsif commented Feb 1, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment