[HOLD for payment 2023-11-06] [$500] [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event

[marcaaron]

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0180d44e7dee5aa64d
• Upwork Job ID: 1701421752840802304
• Last Price Increase: 2023-09-12
• Automatic offers:
• tienifr | Contributor | 26646331

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot]

Triggered auto assignment to @flaviadefaria (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

[neonbhai]

This is because this patch does not pass {passive: true} to the addEventListener() function for the 'wheel' event

[melvin-bot]

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

[flaviadefaria]

@marcaaron can you add a bit more info here? Are you going to fix this? Should I add an internal label? Thanks!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0180d44e7dee5aa64d

[melvin-bot]

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

[melvin-bot]

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

[marcaaron]

Sorry @flaviadefaria. Sending it to External to fix.

[tienifr]

Proposal

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

Warning [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event

What is the root cause of that problem?

This issue comes from react-native-web. In that lib here we're not using passive options for performance improvement when adding listener for the 'wheel' event. More details on the passive option here.

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

We need to add supportsPassive ? { passive: true } : false as the third argument in here. We need to set up feature detection (the supportsPassive) to make sure it supports passive, since for older browsers, any object passed as the third argument will mean capture: true, which we don't want (see here for more details).

I noticed a couple of places in our App, we're not setting the passive option as well (for example here and here), we can consider fixing them there similarly.

What alternative solutions did you explore? (Optional)

touchstart and touchmove event listeners have the same issues and we can consider fixing them similarly.

[thesahindia]

@tienifr's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @thesahindia Please request via NewDot manual requests for the Reviewer role ($500)

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[johncschuster]

Same^

[johncschuster]

Still waiting on PR in RNW upstream to be resolved #26990 (comment)

[tienifr]

@johncschuster @danieldoglas Upstream PR was finally merged necolas/react-native-web#2601 and will be released in the next react-native-web version. But I'm not sure what's the next step here. Should we close this issue now or wait for the next react-native-web version upgrade in App?.

[johncschuster]

@tienifr great question! I'm not really sure myself. I'll let @danieldoglas weigh in on that.

[danieldoglas]

Let's wait until we have the new version of upgraded in the app!

[bernhardoj]

FYI, this is the PR that upgrades the rn-web that includes the upstream fix from this issue

[tienifr]

Thanks @bernhardoj.

[bernhardoj]

Hey, the PR is currently blocked with the upstream PR from this issue. Adding {passive: true} to the wheel listener broke the scrolling of an inverted list (report screen uses inverted list) and also giving lots of console errors. It's also reported in the upstream issue here.

Screen.Recording.2024-04-30.at.10.34.32.1.2.mov

Can someone take a look, please?

[tienifr]

@bernhardoj Thanks, I'm taking a look. However, suppose that we need another PR to fix that, what would we do next? Because as I see the maintainer's update frequency is very rare.

[bernhardoj]

@tienifr I think when you open the PR, you can ask the maintainer whether it's possible to release the PR as soon as it's merged because I think it's a critical issue.

If that's not possible, maybe I can create a patch that revert it to unblock my PR?

cc: @danieldoglas

[johncschuster]

I'm having a hard time understanding where we're at with this currently. What are we waiting on?

[bernhardoj]

@johncschuster I believe we are waiting on @tienifr to raise a new upstream fix for this issue.

[johncschuster]

Awesome. Thank you, @bernhardoj!

[bernhardoj]

@danieldoglas do you think I should create a patch to revert necolas/react-native-web#2601 to unblock #33502?

[tienifr]

Hi @bernhardoj @danieldoglas @johncschuster I spent a lot of time digging into this and found out that we couldn't fix this warning issue [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event because RNW implemented custom logic for handling scrolling for inverted list and passive event does not support it by default.

I think what's next here is we reverted the upstream PR (to prevent the default passive event behavior breaking the inverted scrolling function) and ignore the warning.

[tienifr]

Revert PR is here: necolas/react-native-web#2667.

[johncschuster]

Awesome. Thanks for the update and PR, @tienifr!

[tienifr]

PR is merged. @bernhardoj you're unblocked now. @johncschuster we can close this.

[bernhardoj]

Not released yet, so I'm still blocked.

[tienifr]

Unblocked now.

[johncschuster]

Thanks for the update, @tienifr! Just so I'm clear, we can now close this issue, right? All we need how to issue payment, correct?

[tienifr]

@johncschuster Yes, payment was issued for me already here (not sure about the C+).

After all payments are done we're good to close.

[johncschuster]

@thesahindia, it looks like you were pinged to request payment for this issue, here, can you confirm you've received payment? If so, it sounds like we can close this one out.

[thesahindia]

I will send a money request soon. Feel free to close it!

[JmillsExpensify]

$500 approved for @thesahindia based on this summary.