[HOLD] [$500] Web - Pressing the up arrow, the scroll bar is going down.

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to https://staging.new.expensify.com
2. Press the Tab key until the focus appears in Chat.

Expected Result:

When pressing the down arrow, the scrollbar should follow the down arrow command. When pressing the up arrow, the scrollbar should go up.

Actual Result:

When pressing the down arrow, the scroll bar is going up. When pressing the up arrow, the scroll bar is going down

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.54.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): n/a

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5523437_2022-04-07_10-42-59.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @Julesssss (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Julesssss]

I can confirm this is occurring, but believe the real solution is to resolve this issue as this is a side effect of our current solution.

It also seems to occur regardless of using tabs. I can see the same issue just by clicking into the chat window, is that not the same for you @kbecciv?

[kbecciv]

@Julesssss There is another issue, we logged recently #8608. Is this the same issue?

[melvin-bot]

@Julesssss Eep! 4 days overdue now. Issues have feelings too...

[Julesssss]

@kbecciv that seems slightly different to me, lets keep both open.

[melvin-bot]

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[trjExpensify]

So @Julesssss, is this not a side effect of bidirectional scrolling as you mentioned here, or does it need to go on [hold] for now?

[Julesssss]

So I think this is a side effect yes, we can either put it on hold or just keep it open in the off chance that someone finds a fix outside of bi-directional scrolling. I'd lean towards the second option.

Also, the other mentioned issue is totally different IMO.

[trjExpensify]

Got it. Would that fix on the off chance effectively be a workaround though, whereas our preference is to solve this properly with bidirectional scrolling which we're now making progress on? Going to CC: @roryabraham as well

[Julesssss]

I don't think we can answer that without seeing a proposal. I don't know enough to say whether it's fixable outside of the bidirectional fix personally -- but maybe Rory does. Either way, I'm happy to keep this open or just put it on hold.

[trjExpensify]

Cool. @roryabraham, can you give us an opinion on this before moving it to Upwork please?

[roryabraham]

I don't think this is related to bidirectional scrolling / React Native's FlatList, but instead it's related to this code, which IIRC has been around since before E/App (previously, E/Expensify.cash, previously Expensify/ReactNativeChat) was open-source :D

[roryabraham]

Looks like this HACK ALERT originates here

[roryabraham]

But also looks like it was fixed in react-native-web in 17.7, which is not yet released.

Sounds like we should file a bug report in react-native-web and let them know that their unreleased inverted scroll fix doesn't work if you focus the scrollbar and use the ArrowUp and ArrowDown keys.

[roryabraham]

Also, do we really care to fix this? Seems like a pretty extreme edge case to handle, with consequences that aren't that bad. Maybe we can just live with it? 🤷

[Julesssss]

Nice spot! I don't see why we wouldn't let a contributor do the work, but as this requires one of us to chase another team perhaps that doesn't really work in this case...

[roryabraham]

Unclear what the status is here – the solution we were working on is definitely a "hack", but it a similar "hack" (for the wheel event) was merged in the upstream. That said, it sounded like it was unlikely that this particular hack would ever be merged in the upstream.

So I guess my take is that we have three choices:

1. Wait for updates and reviews on [Draft] Fix reverted virtualized list necolas/react-native-web#2151, which gets rid of the negative transform and might fix a few related issues with inverted lists. There hasn't been any movement here, so if that's the plan should we try and jump-start discussions again?
2. Move forward with the hack to fix the issue
3. Do nothing, close this issue

Thoughts on which path we want to take?

[parasharrajat]

PR was last updated on Oct 2021. I guess it was created for one of our issues and never completed. I doubt that it would be completed in near future. I will go with option 2 as we are already controlling the scroll via code so adding these listeners should be fine too.

[marcaaron]

I will go with option 2 as we are already controlling the scroll via code so adding these listeners should be fine too.

Yeah, I'm going to disagree with this kind of thinking and say that it is especially dangerous when it comes to lower priority issues.

We should practice zero tolerance unless the feature is something highly valuable. It doesn't matter what exists in the code now or if this is "similar" to some bad practice we left in the code while we were putting the MVP together. What we have might be already bad, but we are actively choosing to make the code worse than it is.

Granted there might be times when we have no other option and we can't live without a feature. But is this one of those times? Just my opinion here and I would encourage others to chime in - but it really feels like no.

[parasharrajat]

Thanks for the explanation. I agree with your point of view now. This is not a kind of urgent issues. We should not put more hacks over hacks.

[roryabraham]

Sure, I'm fine with saying we aren't going to move forward with the "hack". I gave the 🟢 for it, so I think we should pay @b1tjoy for their time.

As for the way forward, there seems to be consensus that this isn't very important. While we could create a bounty and try to get a better fix merged upstream, maybe we should just pick our battles and close this?

[marcaaron]

While we could create a bounty and try to get a better fix merged upstream, maybe we should just pick our battles and close this?

I'm curious to hear more opinions on why we should or should not fix this in the long term. There is a trail of evidence that show there is an issue with FlatList's implementation in react-native/react-native-web so seems like it would benefit us to put energy into the upstream fix.

[arielgreen]

What, if any, action should I be taking here?

[arielgreen]

What, if any, action should I be taking here?

[roryabraham]

Sorry for the delay here @arielgreen. I'm going to suggest that we pay @b1tjoy for their time, then put this issue on HOLD. I think the best thing for us to do might be to try to create a tracking issue for all the problems we have with the FlatList component as a "wish list" to get fixed upstream.

[arielgreen]

Done.

[roryabraham]

No update here, not a pressing priority

[roryabraham]

Still not a priority, going to drop this in case anyone else wants to work on it

[roryabraham]

Actually, I'm just going to close this, because I don't think it's worth fixing. If anyone feels strongly otherwise, feel free to reopen