Beta fix stub cells #460

pradeepnschrodinger · 2019-06-05T23:03:25Z

(Side effect / regression from #454)

Description

Cells can have stale props.

Our buffered rows renders stub/fake rows to avoid mounts/unmounts. These rows have temporary props that don't reflect the table's props since the rows won't anyway be visible to the user. But the props propagate to the cell.
The cells have a shouldComponentDidUpdate to skip renders, so even when the rows become "real" the cells aren't re-rendered with the updated props.

Added an example to easily catch these cases in the future.

Motivation and Context

Fixes the issue seen in the screen shot. You'll see blank cells.

How Has This Been Tested?

Using auto scroll example

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.

pradeepnschrodinger · 2019-06-06T06:11:54Z

examples/AutoScrollExample.js

+        rowsCount={dataList.getSize()}
+        width={1000}
+        height={500}
+        scrollLeft={scrollLeft}


scrollLeft and scrollTop specified since we make use of controlled scrolling

pradeepnschrodinger · 2019-06-06T06:12:13Z

examples/AutoScrollExample.js

+        height={500}
+        scrollLeft={scrollLeft}
+        scrollTop={scrollTop}
+        onVerticalScroll={this.onVerticalScroll}


these are needed since the user can also scroll the table directly outside of our "autoscroll" logic

pradeepnschrodinger · 2019-06-06T06:12:48Z

examples/AutoScrollExample.js

+        scrollTop: prevState.scrollTop + (prevState.verticalScrollDelta || 0),
+        scrollLeft: prevState.scrollLeft + (prevState.horizontalScrollDelta || 0),
+      }));
+    }, 16);


pradeepnschrodinger · 2019-06-06T06:13:36Z

site/examples/examplesPageStyle.less

+    justify-content: space-around;
+    align-items: baseline;
+  }
+  .autoScrollCell {


center text both horizontally and vertically

pradeepnschrodinger · 2019-06-06T06:13:51Z

site/examples/examplesPageStyle.less

+
+.autoScrollContainer {
+  .autoScrollControls {
+    display: flex;


just space the controls evenly (both between and outside)

pradeepnschrodinger · 2019-06-06T06:14:16Z

src/FixedDataTableBufferedRows.js

-      // if the row doesn't exist in the buffer, assign a fake row to it.
-      // this is so that we can get rid of unnecessary row mounts/unmounts
+    // render each row from the buffer into the static row array
+    for (let i = 0; i < this._staticRowArray.length; i++) {


refactored so that there's only a single loop over the rows

pradeepnschrodinger · 2019-06-06T06:14:56Z

src/FixedDataTableBufferedRows.js

-          this._staticRowArray[i] = this.getStubRow(i, true, -1);
-        }
-        continue;
+        rowIndex = this._staticRowArray[i] && this._staticRowArray[i].props.index;


here we take the previous rendered row to avoid unmounts

if it is still undefined (aka, fake row), it will be rendered as null

pradeepnschrodinger · 2019-06-06T06:16:40Z

src/FixedDataTableBufferedRows.js

+    let rowProps = {};
+
+    // if row exists, then calculate row specific props
+    if (!fake) {


this was were we had trouble (actual issue)

previously rows outside the viewport were seen as stub/fake, and hence given bad props

pradeepnschrodinger · 2019-06-06T06:17:21Z

src/FixedDataTableBufferedRows.js

+        showScrollbarY={props.showScrollbarY}
+        visible={visible}
+        fake={fake}
+        {...rowProps}


apply the row specific props (will be an empty object for fake/stub rows)

wcjordan

lgtm. Is there anyway to write a unit test which would have caught this?

wcjordan · 2019-06-10T19:48:32Z

It might be worth having @mycrobe & @novakps look at this as well

pradeepnschrodinger · 2019-06-17T01:49:48Z

lgtm. Is there anyway to write a unit test which would have caught this?

Good idea. No idea if it's possible though, but I'll investigate...
Made an issue (#466) for tracking since I think we should write the test after we merge this PR (it's a important bug fix)

mycrobe · 2019-06-18T13:31:54Z

src/FixedDataTableBufferedRows.js

-        />;
+      this._staticRowArray[i] = this.renderRow({
+        rowIndex,
+        key: i,


out of interest, why is this the key not set to rowIndex?

because if the key is given as a function of rowIndex, rows will get mounted/unmounted whenever row indexes change. So we keep the keys constant, form 0 to no:rows present.

FDT uses IntegerBufferSet to manage row index to key mappings, so that when you scroll and it leads to a new row getting visible in the viewport, it'll replace the old one that went outside the viewport.

So basically if we scroll down by 1 row, for an initial viewport of 10 rows, then row 0 at key 0 might gets replaced by row 10, and the key should still be 0 to prevent react mounting/unmounting.

mycrobe

LGTM

novakps

lgtm

alecf · 2019-06-29T00:57:13Z

This broke body scrolling + resizing for a project I'm working on (sadly, not open source) - I don't have a simplified test case but maybe symptoms can help flush this out:

basically happened is the body area stopped scrolling in any direction, while the header happily scrolled left and right. Resizing the container that the table was in was also not recognized - when our table is first rendered it has 3 rows, but when you drag it taller, only the original 3 rows stay on the screen.

We have our own custom cells which wrap the included Cells - i.e. they are stateless function components whose only child is the Cell imported here - they worked fine until 1.0.0-beta.21, and reverting back to 1.0.0-beta.20 fixed the problem for us. (we had to use our own wrappers because of something in a very old version, it may be that we can use the new cells... I will experiment and report back)

pradeepnschrodinger · 2019-06-30T22:34:04Z

@alecf , that's unfortunate to hear. Any updates here? I'm not seeing the issue mentioned...

cmoad · 2019-08-19T15:33:17Z

We are experiencing the same problem as @alecf. Scrolling stopped working completely at beta.21 and is still not working with beta.25. The virtual scroll bar moved up and down, the shadows appear and disappear at appropriate times, but the actual content inside the table doesn't move at all. Any recommendation on where to start with debugging this?

pradeepnschrodinger · 2019-08-20T07:32:47Z

Hey @cmoad, could you also specify these:

browser name
device used
whether touch scrolling is used
direction of scrolling and whether it is used through the scrollbar thumb
table props

A reproducible example will also really help in nailing the exact issue.

For debugging, you can check FixedDataTableCell, which is responsible for rendering your cell (also translates it). Maybe the shouldComponentUpdate method returns false for some reason?

Making up a new issue for this would be better, and we can continue the discussion over there.

pradeepnschrodinger · 2019-09-18T12:55:14Z

@alecf and @cmoad, we released a fix for the scroll issue you guys had mentioned. Please use the latest beta v1.0.0-beta.28.

pradeepnschrodinger added 6 commits June 3, 2019 21:25

Fix cells having stale props

6bf3e9a

Refactor FixedDataTableBufferedRows

de2c67b

Remove shouldComponentUpdate

d9f3420

Remove debug statements

22de3eb

Add 'AutoScroll' Example

250ac37

Fix stale props

e4c3d0a

pradeepnschrodinger requested review from wcjordan and somavara June 5, 2019 23:04

Nits

0968b72

pradeepnschrodinger commented Jun 6, 2019

View reviewed changes

wcjordan approved these changes Jun 10, 2019

View reviewed changes

pradeepnschrodinger requested review from mycrobe and novakps June 11, 2019 16:04

pradeepnschrodinger added bug regression issue that was "fixed" but got reopened later on labels Jun 13, 2019

pradeepnschrodinger mentioned this pull request Jun 17, 2019

Write Unit Test for #460 #466

Open

mycrobe reviewed Jun 18, 2019

View reviewed changes

mycrobe approved these changes Jun 18, 2019

View reviewed changes

novakps approved these changes Jun 24, 2019

View reviewed changes

pradeepnschrodinger merged commit 5fc1b71 into v1.0-beta Jun 25, 2019

pradeepnschrodinger mentioned this pull request Jun 30, 2019

Touch scrolling on Android triggers page refresh #471

Closed

This was referenced Sep 16, 2019

Rows not scrolling in beta-26 #482

Closed

Fix scrolling - Beta #483

Merged

pradeepnschrodinger deleted the beta-fix-stub-cells branch January 5, 2021 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beta fix stub cells #460

Beta fix stub cells #460

pradeepnschrodinger commented Jun 5, 2019 •

edited

Loading

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

pradeepnschrodinger Jun 6, 2019

wcjordan left a comment

wcjordan commented Jun 10, 2019

pradeepnschrodinger commented Jun 17, 2019 •

edited

Loading

mycrobe Jun 18, 2019

pradeepnschrodinger Jun 18, 2019

mycrobe left a comment

novakps left a comment

alecf commented Jun 29, 2019

pradeepnschrodinger commented Jun 30, 2019

cmoad commented Aug 19, 2019

pradeepnschrodinger commented Aug 20, 2019

pradeepnschrodinger commented Sep 18, 2019

Beta fix stub cells #460

Beta fix stub cells #460

Conversation

pradeepnschrodinger commented Jun 5, 2019 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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

wcjordan left a comment

Choose a reason for hiding this comment

wcjordan commented Jun 10, 2019

pradeepnschrodinger commented Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mycrobe left a comment

Choose a reason for hiding this comment

novakps left a comment

Choose a reason for hiding this comment

alecf commented Jun 29, 2019

pradeepnschrodinger commented Jun 30, 2019

cmoad commented Aug 19, 2019

pradeepnschrodinger commented Aug 20, 2019

pradeepnschrodinger commented Sep 18, 2019

pradeepnschrodinger commented Jun 5, 2019 •

edited

Loading

pradeepnschrodinger commented Jun 17, 2019 •

edited

Loading