-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment][Bug] After typing even a mildly long comment in the Android compose box, the whole app starts to hang #11321
Comments
Triggered auto assignment to @johncschuster ( |
Triggered auto assignment to @Beamanator ( |
Hmm I believe this should be able to be investigated by contributors so I'll make it |
Triggered auto assignment to @kadiealexander ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
Current assignee @Beamanator is eligible for the External assigner, not assigning anyone new. |
@Beamanator I was able to repro this on an asus phone with android 12, but it took quite a long message (40k characters). |
I am so confused, this is incredibly easy to reproduce with 100% consistency on my Pixel 5 within about 10 seconds. Just:
Are you saying that those steps do not reproduce it for you? |
Hmm I'm on a Samsung Galaxy A72 and I followed your exact steps, didn't see anything hang or really slow down at all 🤷 |
Just did a fresh uninstall/reinstall on my Pixel 5, created a new account, and was able to reproduce in about 15 seconds: The keyboard is still working in that I am still able to press keys, but the input box is hung and no new input actually goes into it. I suspect it has something to do with saving the draft too quickly? Do we batch changes to the draft, or save fresh with every keystroke? |
Interesting, I was able to reproduce on a Pixel 4 simulator! Though after mashing the keyboard of my Samsung Galaxy A72 for literally a minute, I still didn't see any slowdown 🤷 As for saving the draft to Onyx, it looks like we do debounce here:
That ^ debounce looks like it's working properly, so I'm not sure what else would be causing this, I think this should be part of the WhatsApp Quality project so I'll add it there |
Waiting on proposals, I think we can double this to make it more enticing @kadiealexander |
Wow, very interesting!
…On Thu, Nov 17, 2022 at 9:07 AM Hanno J. Gödecke ***@***.***> wrote:
The underlying problem
To achieve the inverted list effect react native simply flips the FlatList
vertically with scaleY: -1. It then also flips all items with the same
transformation.
On the native side this translates to the following transformation
commands, which get applied to the view:
- transformX: 0
- transformY: 0
- rotate: 180
- rotateY: 0
- rotateX: 180 (now the view looks as usual again as we reverted the
previous rotation)
- scaleX: 0
- scaleY: -1
The problem now is, that on android 13 a bug was introduced that causes
ANRs when rotateX and scaleY are set and we simultaneously render stuff
(animations, entering text). It's a bit hard to tell exactly what's going
wrong on the android side, but that's also nothing we can really help with.
*I am currently in the process of filing a bug report to the android OSS
project. Will link soon.*
How the workaround works
We manually set the styles for the FlatList and its items to achieve the
inverted effect. However, we add a scaleX: -1, which changes the
transform operations on the native side to skip rotateX: 180 (it's then
only rotateY: -180). This way we circumvent the bug.
When doing this the only issue is, that as we have mirrored the FlatList
on the X-Axis the ScrollBar will be on the wrong side. That's why I added a
prop called verticalScrollbarPosition to the ScrollView, so we can change
its position.
Long term
I think it would be best to get the issue fixed on android and then revert
the workaround. Another solution could be to modify how react native
applies transformations, something I want to look into as well (its a bit
redundant to apply transformations that are not really needed).
—
Reply to this email directly, view it on GitHub
<#11321 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEMNUSGV6FKY6KEIBRTRFDWIZQ6TANCNFSM6AAAAAAQWMQIPE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I'm curious if you have an ETA for when this workaround can be merged? |
@Beamanator, @hannojg, @sobitneupane, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Closing the loop. PR is here. Discussion also happening in slack. |
Yep 👍 PR is approved, just need to get a quick change to our |
This is a temporary addition to help circumvent a bug on android. Long term we want to fix the root cause of the problem. - RN issue: facebook#35350 - Expensify issue: Expensify/App#11321 (comment)
The PR has been merged 🎉 lets wait to see how this will perform in staging |
Looks like this is on production at this point. @quinthar Has mobile performance improved for this case? |
yes, this seems to be fixed, it's miraculous, thank you!
…On Sat, Nov 26, 2022 at 6:27 PM Jason Mills ***@***.***> wrote:
Looks like this is on production at this point. @quinthar
<https://github.com/quinthar> Has mobile performance improved for this
case?
—
Reply to this email directly, view it on GitHub
<#11321 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEMNUUJIL65N4XQUCH6RO3WKLBIDANCNFSM6AAAAAAQWMQIPE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Woo! Go team. @kadiealexander We should make sure that contributors and related are paid out before closing the issue. Thanks! |
This comment was marked as off-topic.
This comment was marked as off-topic.
The old Upwork job closed, so please apply here @hannojg and @sobitneupane and I'll issue payment for you both. :) https://www.upwork.com/jobs/~015cde3e5ee4ee738f |
Sorry @kadiealexander, but I can't open any of those links 😅 can you help me get access to it? (I am part of #expensify-margelo and #expensify-open-source on slack) |
Ah! To receive this bonus you may need to set up a profile in Upwork, which is how we typically pay our freelance contributors. |
@kadiealexander I didn't review the proposal and PR. So, I am not eligible for payment. |
@kadiealexander I think we have some confusion here, sorry if I caused it. @hannojg is working with on a special engagement. And then it sounds like @sobitneupane did not review the linked PR, so when I say "make sure" I mainly meant let's double check we're all good here before closing. |
@hannojg after internal discussion, you're eligible for the $500 bonus for this issue if you wish. You'll just need to create an Upwork profile to process the payment. Please let me know how you wish to proceed! |
Discussing payment delivery here. |
Payment is handled separately through a invoice by Margelo! |
In that case we can close this one, nice job @hannojg! |
@hannojg Thank you so much for your work on this, it solved a major problem we were having with our messenger for Android 13 devices. |
Woo! Thanks everyone. |
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:
Expected Result:
App should respond quickly
Actual Result:
After typing even a mildly long comment in the Android compose box, the whole app starts to hang.
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.3-0
Reproducible in staging?:
Reproducible in production?:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: @JmillsExpensify @quinthar
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1663172358453739
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: