Skip to content
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

[WAITING ON MIROSLAV FOR PAYMENT] [HOLD for payment 2023-10-02] [$1000] content of input jumps from top to bottom in multi-line inputs #21654

Closed
1 of 6 tasks
kavimuru opened this issue Jun 26, 2023 · 141 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jun 26, 2023

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. create a task
  2. open the task
  3. write a long multi-line description and save it
  4. now open the description and observe the input field and scrollbar in RHP

Expected Result:

content should be at bottom at the time of rendering

Actual Result:

content of input jumps from top to bottom

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.32-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

bandicam.2023-06-24.18-12-20-493.mp4
Recording.1111.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014ef411cb69302ae1
  • Upwork Job ID: 1673690266454241281
  • Last Price Increase: 2023-09-12
  • Automatic offers:
    • 0xmiroslav | Reviewer | 26723385
    • chiragxarora | Contributor | 26723386
    • chiragxarora | Reporter | 26723387
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

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

@s-alves10
Copy link
Contributor

s-alves10 commented Jun 27, 2023

Proposal

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

Content of Input jumps from top to bottom in multi-line input

What is the root cause of that problem?

We set focus on the input after transition ends as you can see below

onEntryTransitionEnd={() => inputRef.current && inputRef.current.focus()}

So focus is set after the transition ends and the modal is shown fully. This is the root cause

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

When we add autoFocus props to the TextInput component, its focusing is done in BaseTextInput component below.

input.current.focus();

We can use focusAndUpdateMultilineInputRange function in order to set cursor at the last position combining with autoFocus.

  1. Add the autoFocus props to TextInput component and remove the onEntryTransitionEnd from ScreenWrapper in TaskDescriptionPage

  2. Use the focusAndUpdateMultilineInputRange function in useEffect of BaseTextInput. Replace the below code

    if (props.shouldDelayFocus) {
    focusTimeout = setTimeout(() => input.current.focus(), CONST.ANIMATED_TRANSITION);
    return;
    }
    input.current.focus();

    with

        if (props.shouldDelayFocus) {
            focusTimeout = setTimeout(() => focusAndUpdateMultilineInputRange(input.current), CONST.ANIMATED_TRANSITION);
            return;
        }

        focusAndUpdateMultilineInputRange(input.current);

, where focusAndUpdateMultilineInputRange function is from the libs/focusAndUpdateMultilineInputRange.js

This works as expected on all platforms.

Result
21654_mac_chrome.mp4

What alternative solutions did you explore? (Optional)

@chiragxarora
Copy link
Contributor

chiragxarora commented Jun 27, 2023

Proposal

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

Content jumps from top to bottom in multi line input in task description page

What is the root cause of that problem?

Root cause is the late setting of focus using ref because initially cursor is placed at top and later it is brought down using selection prop. And when we focus after this, it shows the jump behaviour.

onEntryTransitionEnd={() => inputRef.current && inputRef.current.focus()}

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

What other proposal above suggested defeats the purpose of our logic of solving transition issue since on Android it will cause problems. To tackle this in a different way, we have another prop called shouldDelayFocus which we should pass along with autoFocus and then we can get rid of the focus via ref.

<TextInput
........
autoFocus
shouldDelayFocus = {shouldDelayFocu}
.........
>
<TextInput/>

Ofcourse we need to import the shouldDelayFocus prop first.
This will put an Android specific delay for the TextInput

Update:

There have been some changes in the file and now we are no longer using setting the selection in useEffect and we have introduced a new util focusAndUpdateMultilineInputRange.
We can use a callback ref on the textinput now to focus the field as it mounts, and we can add conditional delay for the android using shouldDelayFocus.

Updated code:

const inputRefCallback = useCallback((el) => {
        if(!el) {
            return;
        }
        focusMultilineUsingCallbackRef(el, inputRef);
        inputRef.current = el;
}, []);

..........................

<TextInput 
   ref={inputRefCallback}
>
...................
export default function focusMultilineUsingCallbackRef(el, inputRef) {
    if(inputRef.current) {
        return;
    }
    if(shouldDelayFocus) {
        setTimeout(() => {
            focusAndUpdateMultilineInputRange(el);    
        }, CONST.ANIMATED_TRANSITION);
    }
    else focusAndUpdateMultilineInputRange(el);
}
Results
bandicam.2023-07-03.16-22-12-723.mp4

What alternative solutions did you explore? (Optional)

None

@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Jun 27, 2023
@melvin-bot melvin-bot bot changed the title content of input jumps from top to bottom in multi-line inputs [$1000] content of input jumps from top to bottom in multi-line inputs Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

Job added to Upwork: https://www.upwork.com/jobs/~014ef411cb69302ae1

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

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

@abekkala
Copy link
Contributor

@0xmiroslav have you had a chance to review the proposals that have come in so far?

@abekkala
Copy link
Contributor

@0xmiroslav can you review the proposals above?

@abekkala abekkala removed their assignment Jun 30, 2023
@abekkala abekkala added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jun 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 2023

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

@Expensify Expensify deleted a comment from melvin-bot bot Jun 30, 2023
@abekkala
Copy link
Contributor

@jliexpensify Reassigning for sabbatical


CURRENT STATUS:

Waiting on a proposal to be chosen

  • Issue reported by: @chiragxarora
  • Assigned for Fix: TBD
  • C+: @0xmiroslav

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2023
@chiragxarora
Copy link
Contributor

BUMP @0xmiroslav

@0xmiros
Copy link
Contributor

0xmiros commented Jul 3, 2023

@chiragxarora can you please test if your solution doesn't break auto-focus on all platforms (especially android, mChrome, mSafari)?
For mSafari, if it already doesn't work on main, that's fine.

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2023
@s-alves10
Copy link
Contributor

@0xmiroslav
What do you think of my solution?

@0xmiros
Copy link
Contributor

0xmiros commented Jul 3, 2023

@s-alves10 your solution will break android auto-focus

@chiragxarora
Copy link
Contributor

@chiragxarora can you please test if your solution doesn't break auto-focus on all platforms (especially android, mChrome, mSafari)? For mSafari, if it already doesn't work on main, that's fine.

sure, will do that in morning

@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
  • [@0xmiroslav] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@0xmiroslav] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@0xmiroslav] Determine if we should create a regression test for this bug.
  • [@0xmiroslav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@jliexpensify
Copy link
Contributor

jliexpensify commented Oct 3, 2023

Was there a regression here?

Payment Summary:

  • Reporter / Contributor: @chiragxarora $250 + $1000 + 500 = $1750
  • C+: @0xmiroslav : $1000 + $500 = $1500

Upwork job

Miroslav, please complete the checklist.

@melvin-bot melvin-bot bot removed the Overdue label Oct 3, 2023
@jliexpensify jliexpensify changed the title [HOLD for payment 2023-10-02] [$1000] content of input jumps from top to bottom in multi-line inputs [WAITING ON MIROSLAV TO COMPLETE CHECKLIST] [HOLD for payment 2023-10-02] [$1000] content of input jumps from top to bottom in multi-line inputs Oct 3, 2023
@chiragxarora
Copy link
Contributor

I think urgency bonus applies here too since the PR was held on @thienlnam 's response for more than a day

cc @jliexpensify

@jliexpensify
Copy link
Contributor

Ok, that's fair - I can trace it to this comment. I'll adjust!

@0xmiros
Copy link
Contributor

0xmiros commented Oct 3, 2023

Was there a regression here?

No, Here's the first comment which linked this issue.

@0xmiros
Copy link
Contributor

0xmiros commented Oct 3, 2023

  • The PR that introduced the bug has been identified. Link to the PR: N/A
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • Determine if we should create a regression test for this bug.
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

This is minor UI improvement than bug and doesn't affect user input at all so I don't think we should create regression test.

@jliexpensify
Copy link
Contributor

Thanks, are you ready to take payments @0xmiroslav ?

@chiragxarora
Copy link
Contributor

Also @jliexpensify the reporting bonus offer which I just accepted in the upwork is for 50$ 😅

@0xmiros
Copy link
Contributor

0xmiros commented Oct 3, 2023

Thanks, are you ready to take payments @0xmiroslav ?

Not yet. Please close this for now after @chiragxarora's payment is done. I am tracking myself.

@jliexpensify
Copy link
Contributor

@chiragxarora no worries, I will fix this up when I pay - there's the ability to add bonuses.

@chiragxarora
Copy link
Contributor

Okay thanks

@jliexpensify
Copy link
Contributor

Oh weird, the job is now closed - @chiragxarora you haven't been paid right? I guess it was an old listing.

@chiragxarora
Copy link
Contributor

Yes I'm not yet paid @jliexpensify

@chiragxarora
Copy link
Contributor

Actually @jliexpensify both of my contracts are running in upwork, for contributor and reporter, I've accepted them, you should be able to get into those contracts and pay there I think?

@jliexpensify
Copy link
Contributor

Hmm everything is showing as inactive for me:

image

Can you share the job/contract link with me? Cheers

@chiragxarora
Copy link
Contributor

@chiragxarora
Copy link
Contributor

Are you able to find this in your active contracts?

@jliexpensify
Copy link
Contributor

@chiragxarora I can pay these ones - can you send me a link to the reporter one as well?

@chiragxarora
Copy link
Contributor

Here you go @jliexpensify

https://www.upwork.com/nx/wm/workroom/35018390/overview

@jliexpensify
Copy link
Contributor

Thanks, have paid you @chiragxarora . @0xmiroslav closing this out, but here is the payment summary.

@jliexpensify jliexpensify changed the title [WAITING ON MIROSLAV TO COMPLETE CHECKLIST] [HOLD for payment 2023-10-02] [$1000] content of input jumps from top to bottom in multi-line inputs [WAITING ON MIROSLAV FOR PAYMENT] [HOLD for payment 2023-10-02] [$1000] content of input jumps from top to bottom in multi-line inputs Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants