[HOLD on #16660][$1000] Web – Assign Task – Whole page jitter on check and uncheck of checkbox.

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open any " Assign Task " chat.
2. Check and uncheck checkbox under title multiple times.
3. Notice after few times of check and uncheck whole page starts to jitter.

Expected Result:

Page should not start to jitter upon check and uncheck of checkbox.

Actual Result:

Page starts to jitter upon selection check and uncheck of checkbox.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.39-5
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Bug.11.mp4

Recording.3551.mp4

Expensify/Expensify Issue URL:
Issue reported by: @usmantariq96
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689069224351539

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0156490fa397b075d8
• Upwork Job ID: 1681396985675022336
2023-07-25
• Automatic offers:
• | | 0
• | | 0

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

@michaelhaxhiu Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@michaelhaxhiu 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0156490fa397b075d8

[melvin-bot]

Current assignee @michaelhaxhiu is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @robertKozik (External)

[michaelhaxhiu]

This was tough to reproduce, but I got it eventually. Make sure to check/uncheck the task until there is a scroll bar in the thead, then scroll up so that you can't see new updates at the footer if you continue to check/uncheck

[robertKozik]

Indeed very tough to repro, I'll keep it under on radar and wait for any proposals 👌🏼

[akamefi202]

Proposal

Please re-state the problem that we are trying to solve in this issue.

In task page, the page jitters when the user clicks(check & uncheck) the checkbox continuously. (Approximately, more than 10~14 times)

What is the root cause of that problem?

App/src/components/InvertedFlatList/BaseInvertedFlatList.js

Lines 136 to 138 in 8dcad16

	// Native platforms do not need to measure items and work fine without this.
	// Web requires that items be measured or else crazy things happen when scrolling.
	getItemLayout={this.props.shouldMeasureItems ? this.getItemLayout : undefined}

I think it's because of getItemLayout in FlatList.
Currently, we save size & offset of each item using measureItemLayout function while rendering and we input them using getItemLayout function in next rendering.
We use sizeMap array to save size & offset of items.

But all these code was assumed and written as if new item will be added to the last element of the item array in FlatList when the user adds a comment.
Actually, it will be added to first element of the item array.

What changes do you think we should make in order to solve the problem?

We need to update getItemLayout and measureItemLayout functions based on correct mechanism.
const sizeMapIndex = this.props.data.length - index - 1;
We need to get index again in reverse order when we read & write size information of items with sizeMap array.
And also offset of newly added item is 0.

I updated the code and I wasn't able to see jitter issue.
Screencast from 2023年07月21日 16时46分54秒.webm

What alternative solutions did you explore? (Optional)

We may not use getItemLayout in FlatList. But it could affect performance for list of many items.

[robertKozik]

Hi @akamefi202, thanks for your proposal. First of all, I've tried to test your approach on my end, but it causes an error for me. I've attached a diff which I used.

Later, I'm not sure about the root cause you have written. As, most likely, you pinpointed the faulty code correctly, but you didn't provide an answer as to why it only occurs after spamming check/uncheck. If we were wrongly saving measurements, shouldn't it cause problems way earlier, not only after clicking multiple times?

diff --git a/src/components/InvertedFlatList/BaseInvertedFlatList.js b/src/components/InvertedFlatList/BaseInvertedFlatList.js
index d3fcda0ea5..e941933cec 100644
--- a/src/components/InvertedFlatList/BaseInvertedFlatList.js
+++ b/src/components/InvertedFlatList/BaseInvertedFlatList.js
@@ -53,13 +53,14 @@ class BaseInvertedFlatList extends Component {
      * @return {Object}
      */
     getItemLayout(data, index) {
-        const size = this.sizeMap[index];
+        const sizeMapIndex = this.props.data.length - index - 1;
+        const size = this.sizeMap[sizeMapIndex];
 
         if (size) {
             return {
                 length: size.length,
                 offset: size.offset,
-                index,
+                index: sizeMapIndex,
             };
         }
 
@@ -76,7 +77,7 @@ class BaseInvertedFlatList extends Component {
             // initialRowHeight since we can only assume that all previous items
             // have not yet been measured
             offset: _.isUndefined(lastMeasuredItem) ? this.props.initialRowHeight * index : lastMeasuredItem.offset + this.props.initialRowHeight,
-            index,
+            index: sizeMapIndex,
         };
     }
 
@@ -88,20 +89,22 @@ class BaseInvertedFlatList extends Component {
      */
     measureItemLayout(nativeEvent, index) {
         const computedHeight = nativeEvent.layout.height;
+        const sizeMapIndex = this.props.data.length - index - 1;
 
         // We've already measured this item so we don't need to
         // measure it again.
-        if (this.sizeMap[index]) {
+        if (this.sizeMap[sizeMapIndex]) {
             return;
         }
 
-        const previousItem = this.sizeMap[index - 1] || {};
+        const previousItem = this.sizeMap[sizeMapIndex - 1] || {};
 
         // If there is no previousItem this can mean we haven't yet measured
         // the previous item or that we are at index 0 and there is no previousItem
         const previousLength = previousItem.length || 0;
         const previousOffset = previousItem.offset || 0;
-        this.sizeMap[index] = {
+        this.sizeMap[sizeMapIndex] = {
             length: computedHeight,
             offset: previousLength + previousOffset,
         };

[akamefi202]

@robertKozik
About the code changes, we should use original index when return size data. We should use sizeMapIndex only for reading & writing sizeMap array.

if (size) {
     return {
         length: size.length,
         offset: size.offset,
         index,
     };
}

And offset should be 0, when we return size data of newly added item.

About root cause, getItemLayout is an optional optimization to improve performance of FlatList when it contains so many items.
When FlatList has only few items, it is enough to compute size of items correctly.
After many items are added to the list, the component start to refer the size data(which is not accurate now) to reduce time for the measurement of dynamic content.
So we see this issue after many items are added to the list.

One more thing is we're adding new items to invisible viewport when we check & uncheck the checkbox.
I think this is the reason why we see this issue only in task page.
The component gives priority to compute size of visible items and we're adding new items to invisible viewport.
So it might use wrong size data from sizeMap array instead of computing size of new items.

This is just my analysis and other engineer might find more accurate reason.
But it's definitely clear that this issue is occurred by getItemLayout and we can fix by updating getItemLayout and measureItemLayout functions.

[melvin-bot]

@michaelhaxhiu, @robertKozik Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[kosmydel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

After checking/unchecking the task the page jitter.

What is the root cause of that problem?

The heights/offsets of the elements in the inverted FlatList are not calculated correctly. The full explanation for that is here. Previously, we addressed it by creating a BaseInvertedFlatList component, but it doesn't work fully as expected.

What changes do you think we should make in order to solve the problem?

Previously, we used the BaseInvertedFlatList component for inverted lists, to make it work on the web. However, the react-native-web library has addressed this issue related to the inverted FlatLists in this PR.

Solution:

1. We can update the @expensify/react-native-web with the latest changes from the necolas/react-native-web repository.
2. Remove the BaseInvertedFlatList component.
3. Replace <BaseInvertedFlatList /> usages with:

<FlatList
 inverted
 {...otherProps}
/>

However, there are some dependency-related issues here.

What alternative solutions did you explore? (Optional)

We can add to the @expensify/react-native-web fork changes only from this PR and replace BaseInvertedFlatList with FlatList with inverted option.

I created a short PoC video showing the results.

PoC video

fix1.mov

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[robertKozik]

@akamefi202 Your proposal has some strange behaviour when the message list is very long:

diff-solution.mov

@kosmydel I think we have bumping the react-native-web version in plans. If thats truly the case your proposal is better suited for this issue

[melvin-bot]

@michaelhaxhiu @robertKozik this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[robertKozik]

Working on proposals 👀

[stitesExpensify]

Still on hold

[stitesExpensify]

hold

[melvin-bot]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[michaelhaxhiu]

I'm re-assigning this to another BZ as part of my preparation for Sabbatical (starting Friday).

Next steps:

• This is on hold for [$3500] Upgrade to react-native-web v0.19.6 or greater #16660
• I'm labeling this Monthly as it's blocked, so please check to ensure we unblock when it's time.

[adelekennedy]

linked issue is still in progress

[kosmydel]

Hey,
I've investigated this issue and it looks like it is fixed now. This PR introduced changes that I suggested in my proposal. I've retested this issue and for me, it doesn't occur anymore. I think we can close this one out.

Video

fixed.mov

cc @allroundexperts, @stitesExpensify, @adelekennedy

[adelekennedy]

I can't reproduce so closing makes sense to me, gut check with you @allroundexperts

[kosmydel]

So in that case, can we close this issue?
cc @adelekennedy, @allroundexperts, @stitesExpensify