Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.35k stars 2.77k forks source link

[HOLD for 42143] [$2000] [LOW] IOS - Keypad appears for a moment in details page on saving Private notes #35779

Open lanitochka17 opened 7 months ago

lanitochka17 commented 7 months ago

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


Version Number: Reproducible in staging?: Reproducible in production?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): applausetester+cknew@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Slack conversation:

Action Performed:

Precondition: User has not saved Private notes

  1. Open a workspace #announce room
  2. Tap header link to go details page
  3. Select Private notes
  4. Enter notes & tap on Save button

Expected Result:

Keypad should not appear in details or settings page while saving Private notes

Actual Result:

Keypad appears for a moment in details/settings page while saving Private notes

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/92cf7644-89a5-462f-b5bc-242f2db291ff

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d02b0565c21176af
  • Upwork Job ID: 1754200247556661248
  • Last Price Increase: 2024-03-25
melvin-bot[bot] commented 7 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01d02b0565c21176af

melvin-bot[bot] commented 7 months ago

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

melvin-bot[bot] commented 7 months ago

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

lanitochka17 commented 7 months ago

We think that this bug might be related to #vip-vsp CC @quinthar

mkhutornyi commented 7 months ago

Proposal

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

iOS - Keypad appears for a moment in details page on saving Private notes

What is the root cause of that problem?

https://github.com/Expensify/App/blob/3ab4e6e12d9ff38e69493321f0d31f032481b373/src/pages/PrivateNotes/PrivateNotesEditPage.tsx#L158-L164 This happens because ref callback is called every time user inputs value, which triggers updateMultilineInputRange

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

As this is supposed to call only once when page is first opened, add more condition in early return here:

     if (!el || privateNotesInput.current) { 
         return; 
     } 
getusha commented 7 months ago

Looks like a very minor issue, @bfitzexpensify i think we can close it, wdyt?

bfitzexpensify commented 7 months ago

@getusha Agreed that it's very minor, but assuming there is a clear and easy solution, we should address this. What do you think of @mkhutornyi's proposal?

getusha commented 7 months ago

Thanks! will test @mkhutornyi's proposal.

getusha commented 7 months ago

@mkhutornyi are you able to reproduce this issue?

https://github.com/Expensify/App/assets/75031127/d811ed93-8b8e-4579-bde5-50c2c672a7b2

mkhutornyi commented 7 months ago

@getusha yes, still reproducible on latest main

https://github.com/Expensify/App/assets/97676131/989d74d0-373b-49fb-8eba-59a0286d9a29

getusha commented 7 months ago

Still not able to repro, what's the your iOS version?

mkhutornyi commented 7 months ago

Still not able to repro, what's the your iOS version?

iOS 16.4 iPhone 14 Plus Simulator

getusha commented 7 months ago

asked for other c+ https://expensify.slack.com/archives/C02NK2DQWUX/p1707408881410369

situchan commented 7 months ago

@getusha this is reproducible not only on this page but also other pages which have multiline input

i.e. on room description page:

https://github.com/Expensify/App/assets/108292595/2bb4c861-51d9-4987-8cb1-05aeb1afb178

situchan commented 7 months ago

The proposed solution doesn't fix issue completely

situchan commented 7 months ago

Another reproducible case: (swipe left to go back)

This even doesn't require text input change.

https://github.com/Expensify/App/assets/108292595/0ec5e69f-3b63-4209-9713-e0589b238b6b

situchan commented 7 months ago

Please let me know if it's still not reproducible for you. If still, I can take over.

getusha commented 7 months ago

@situchan I missed your comment there! @mananjadhav offered to take over.

I'm still unable to reproduce

https://github.com/Expensify/App/assets/75031127/90b0160b-de89-4576-b7a1-3f722a3c40d0

mananjadhav commented 7 months ago

I am able to reproduce. Will check the proposal in a while.

bfitzexpensify commented 7 months ago

Were you able to review the proposal @mananjadhav?

mananjadhav commented 7 months ago

Based on the previous comment it looks like the proposal doesn't work. Open for proposals.

bfitzexpensify commented 7 months ago

No proposals yet - will leave this open for another week, but if we get no traction, I'll raise it in Slack before we double the price given how minor it is.

melvin-bot[bot] commented 7 months ago

@mananjadhav @bfitzexpensify 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!

bfitzexpensify commented 7 months ago

Still awaiting proposals

bfitzexpensify commented 7 months ago

Same update - still awaiting proposals

bfitzexpensify commented 7 months ago

Doubling this - I think the behaviour in https://github.com/Expensify/App/issues/35779#issuecomment-1934479361 is worse than the scenario in the OP and makes this something worth addressing.

melvin-bot[bot] commented 7 months ago

Upwork job price has been updated to $1000

FitseTLT commented 7 months ago

Proposal

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

IOS - Keypad appears for a moment in details page on saving Private notes

What is the root cause of that problem?

We are dismissing the keyboard and immediately navigating without waiting for the dismissing of the keyboard here https://github.com/Expensify/App/blob/29ad8787c7c889271c117dd4420109f9be8e180d/src/pages/PrivateNotes/PrivateNotesEditPage.tsx#L103-L108

What alternative solutions did you explore? (Optional)

We need to navigate under runAfterInteraction, change it to

        Keyboard.dismiss();
        InteractionManager.runAfterInteractions(() => {
            if (!Object.values<Note>({...report.privateNotes, [route.params.accountID]: {note: editedNote}}).some((item) => item.note)) {
                ReportUtils.navigateToDetailsPage(report);
            } else {
                Navigation.goBack(ROUTES.PRIVATE_NOTES_LIST.getRoute(report.reportID));
            }
        });

But to avoid unnecessary delay of navigation where there is no on screen keyboards (like for web and desktop) we will only use it only if canUseTouchScreen is true. (Or if we want it to apply to ios only we can also use it as the condition) BTW we have already used this method here https://github.com/Expensify/App/blob/29ad8787c7c889271c117dd4420109f9be8e180d/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js#L225-L230

melvin-bot[bot] commented 7 months ago

@mananjadhav @bfitzexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 7 months ago

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

bfitzexpensify commented 7 months ago

How does the proposal in https://github.com/Expensify/App/issues/35779#issuecomment-1961326402 look @mananjadhav?

mananjadhav commented 6 months ago

@FitseTLT's proposal looks promising, but I'll test it out and confirm.

mkhutornyi commented 6 months ago

@ntdiary can you confirm if your keyboard+modal refactor PR will cover this issue as well?

mananjadhav commented 6 months ago

@mkhutornyi which PR are we talking about here? @ntdiary did you get a chance to look at the previous comment?

ntdiary commented 6 months ago

My refactor PR will not cover this case, the keyboard flickering here is caused by updateMultilineInputRange. :)

mkhutornyi commented 6 months ago

My refactor PR will not cover this case, the keyboard flickering here is caused by updateMultilineInputRange. :)

ok but the root cause of https://github.com/Expensify/App/issues/35779#issuecomment-1934479361 is different. It's not related to updateMultilineInputRange

mkhutornyi commented 6 months ago

InteractionManager.runAfterInteractions

This might work but we should avoid that kind of workaround as much as possible. Also this won't fix https://github.com/Expensify/App/issues/35779#issuecomment-1934479361

melvin-bot[bot] commented 6 months ago

@mananjadhav @bfitzexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 6 months ago

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

bfitzexpensify commented 6 months ago

Where are we at with this one @mananjadhav? Sounds like we're still awaiting a proposal that doesn't use un-ideal workarounds?

FitseTLT commented 6 months ago

Where are we at with this one @mananjadhav? Sounds like we're still awaiting a proposal that doesn't use un-ideal workarounds?

Considering we are solving platform-specific issue, I don't think my solution is a workaround as the problem is being caused by immediately calling navigate after Keyboard.dismiss call and we have used the same solution to solve similar mWeb issue here but lets wait for @mananjadhav thoughts 👍

mkhutornyi commented 6 months ago

My proposal is not approved because it doesn't fix https://github.com/Expensify/App/issues/35779#issuecomment-1934479361. @FitseTLT your proposal doesn't fix it as well. If it's out of scope, my solution is better since it fixes the real root cause.

mananjadhav commented 6 months ago

Thanks @mkhutornyi @FitseTLT for keeping the discussion active. @FitseTLT can you confirm if this scenario is addressed.

bfitzexpensify commented 6 months ago

Bump here @FitseTLT - thank you!

bfitzexpensify commented 6 months ago

Another bump @FitseTLT

melvin-bot[bot] commented 6 months ago

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

bfitzexpensify commented 6 months ago

Still open for proposals (though also waiting on a clarification from @FitseTLT on their proposal to see if we can move forward with it).

bfitzexpensify commented 6 months ago

Same update

bfitzexpensify commented 6 months ago

Same update again - still waiting on proposals.

bfitzexpensify commented 6 months ago

Still open for proposals.