[$1000] IOS - When using multiple lines for the Welcome message, after saving and returning, the app scrolls

[lanitochka17]

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: 1.2.94.0
**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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #30238

Action Performed:

1. Log in with any acccount
2. Go to #announce or #admins chat
3. Click on header > Details > Settings > Welcome message
4. Use multiple lines for Welcome message
5. Save it
6. Go back to Welcome message again

Expected Result:

When using multiple lines for the Welcome message, after saving and returning, the app should not scroll the message from the top to the bottom

Actual Result:

When using multiple lines for the Welcome message, after saving and returning, the app scrolls the message from the top to the bottom

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6259537_1698852395381.RPReplay_Final1698850932__1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016160035c97c0cabe
• Upwork Job ID: 1719740731830575104
• Last Price Increase: 2023-11-27

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016160035c97c0cabe

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

Other issues / discussions related to similar behaviour on iOS:
#28790
#20836

Proposal

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

When using multiple lines for the Welcome message, after saving and returning back to editing, the app scrolls the multi-line input from the top to the bottom.

What is the root cause of that problem?

There's a race condition between useFocusEffect which calls welcomeMessageInputRef.current.focus() with a timeout of 300ms and the updateMultilineInputRange() which is called right after setting the TextInput's ref.

Why I'm saying this ? Because when commenting out updateMultilineInputRange(), the bug described in this issue can be reproduced with 100% success rate:

iOS: Native - Issue reproduced 100% rate

Screen.Recording.2023-11-08.at.13.57.43.mov

Note: the current implementation is also problematic on MacOS: Firefox, where the input does get focused but there's a scroll glitch and no scrolling is performed.

MacOS: Firefox

Screen.Recording.2023-11-17.at.01.44.42.mov

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

Considering the latest observations / suggestions from #30707 (comment), in order to ensure that we focus the input, as soon as possible, ensuring the cursor is at the end for both iOS and all other platforms, I'm proposing the following changes:

Update the @libs/UpdateMultilineInputRange/index.js function to use currently unused shouldFocusInput prop and adding additional prop shouldDelay in order to ensure the functionality on web works as expected:

Updated @libs/UpdateMultilineInputRange/index.js

+ import CONST from "@src/CONST";

+ let timeoutID = null;
/**
 * Place the cursor at the end of the value (if there is a value in the input).
 *
 * When a multiline input contains a text value that goes beyond the scroll height, the cursor will be placed
 * at the end of the text value, and automatically scroll the input field to this position after the field gains focus.
 * This provides a better user experience in cases where the text in the field has to be edited. The auto-
 * scroll behaviour works on all platforms except iOS native.
 * See https://github.com/Expensify/App/issues/20836 for more details.
 *
 * @param {Object} input the input element
 * @param {boolean} shouldAutoFocus
 * @param {boolean} shouldDelay
 */
- // eslint-disable-next-line no-unused-vars
- export default function updateMultilineInputRange(input, shouldAutoFocus = true) {
+ export default function updateMultilineInputRange(input, shouldAutoFocus = false, shouldDelay = false) {
    const setCursorToEnd = () => {
        if (!input) {
            return;
        }

+       if (shouldAutoFocus) {
+           input.focus();
+       }

+       // The selection set has to be called after the focus, otherwise it will not work on Firefox
        if (input.value && input.setSelectionRange) {
            const length = input.value.length;
            input.setSelectionRange(length, length);
            // eslint-disable-next-line no-param-reassign
            input.scrollTop = input.scrollHeight;
        }
    };

+   // Clear the timeout if it's already been set to prevent multiple invocations
+   if (timeoutID !== null) {
+       clearTimeout(timeoutID);
+       timeoutID = null;
+   }

+   if (shouldDelay) {
+       timeoutID = setTimeout(setCursorToEnd, CONST.ANIMATED_TRANSITION);
+   } else {
+       setCursorToEnd();
+   }
}

• shouldFocusInput default was set to false to ensure backwards compatibility
• by adding shouldDelay logic we ensure that the implementation works the same on all platforms (incl. Firefox web)

@libs/UpdateMultilineInputRange/index.ios.js remains unchanged because we need the focus() without delay to be called as soon as possible.

With this update to the function, the final code will look like this:

// src/pages/ReportWelcomeMessagePage.js

        ref={(el) => {
            if (!el) {
                return;
            }
            welcomeMessageInputRef.current = el;
-           updateMultilineInputRange(welcomeMessageInputRef.current);
+           updateMultilineInputRange(welcomeMessageInputRef.current, true, true);
        }}

and the useFocusEffect block will be completely removed from the component as it won't be needed anymore.

Note:

considering that the same code using both updateMultilineInputRange() and useFocusEffect code block are used in quite a few other instances with the same component type, we can update all those components updateMultilineInputRange() function as well and delete all useFocusEffect code block instances once updated.

What alternative solutions did you explore?

Keeping the updateMultilineInputRange() implementation as it is but replace the useFocusEffect code block with a updated version of the useDelayedInputFocus hook that will target all platforms except iOS (return null), using platform dependent files like so:

@hooks/useDelayedInputFocus/index.ts

import {useRef, useEffect,} from 'react';
import CONST from '@src/CONST';
import { getBrowser } from '@libs/Browser';
import updateMultilineInputRange from '@libs/UpdateMultilineInputRange';

/**
 * Focus a text input when a screen is navigated to, after the specified time delay has elapsed.
 */
export default function useDelayedInputFocus(inputRef: React.RefObject<HTMLInputElement>, delay: number = CONST.ANIMATED_TRANSITION) {
    const timeoutRef = useRef<NodeJS.Timeout | null>(null);

    useEffect(() => {
        timeoutRef.current = setTimeout(() => {
            if (!inputRef.current) {
                return;
            }
            inputRef.current.focus();
            if (getBrowser() === CONST.BROWSER.FIREFOX) {
                updateMultilineInputRange(inputRef.current);
            }
        }, delay);
        return () => {
            if (!timeoutRef.current) {
                return;
            }
            clearTimeout(timeoutRef.current);
        };
    // eslint-disable-next-line react-hooks/exhaustive-deps
    }, []);
}

• we setTimeout, calling focus() and only calling updateMultilineInputRange() again for Firefox to make sure selection will be at the end of last word
• @hooks/useDelayedInputFocus/index.ios.ts returns null because we don't need to focus again since updateMultilineInputRange() right after the ref does that because of the platform specific index.ios.js version of the function

By calling this hook instead of the useFocusEffect code block, together with updateMultilineInputRange() right after the ref is set we achieve the same result as the first solution.

Note:

considering this alternative method's implementation, all useFocusEffect code block instances from the other components can be replaced with this hook ensuring consistent behaviour.

Videos

iOS: Native

Screen.Recording.2023-11-02.at.00.37.09.mov

MacOS: Firefox

Screen.Recording.2023-11-17.at.02.26.56.mov

[isabelastisser]

Hey @cubuspl42, can you please review the proposal above? Thanks!

[cubuspl42]

@ikevin127 The updateMultilineInputRange is already platform-dependent (based on the .js / .ios.js file suffixes), so wrapping it with platform === CONST.PLATFORM. sounds redundant.

[cubuspl42]

This is an iOS-specific issue.

On top of this, we also have this piece of code in the same component:
...
Which does the following:

Then, you provide the non-iOS implementation (from index.js) and draw conclusions.

Doesn't this invalidate your root cause analysis?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[cubuspl42]

In the future, please don't add another comment with the proposal, as per the guidelines one should update the original comment, while adding this marker in a new comment:

## Proposal
[Updated](link to proposal)

Now, you can hide your previous comment as outdated.

[cubuspl42]

[CONST.PLATFORM.IOS].includes(getPlatform()))

Is this a challenge to write === without using ===? 🤔

I meant that, per the Expensify conventions (which I didn't invent), relying on CONST.PLATFORM.* constants is considered an anti-pattern. Please see for yourself how few usages there are in the codebase.

What is preferred, per the conventions, is relying on platform file suffixes (.js / .ios.js / etc.) and creating appropriate abstractions there.

updateMultilineInputRange(welcomeMessageInputRef.current) which is called right after setting the TextInput's ref, in the component return.

I'm aware of this line, and I feel quite uncomfortable with it. I would like to see a solution that removes it (while keeping the problem it solves solved, of course...).

[isabelastisser]

Hey @cubuspl42, can you please review the proposal above? Thanks!

[cubuspl42]

@ikevin127

Thank you for the update!

While I appreciate the efforts and adjusting the solution to the feedback, I'm still not convinced by the solution. It feels extremely specific and work-aroundish.

I have a strong intuition that we miss the bigger picture. There's a race of two pieces of logic that both affect focusing/scrolling/placing the cursor. I don't feel that adding ifs with lengthy comments saying that "another component with timeout of 300s does this and that" is the way to go.

I'd like make the race disappear completely.

when commenting out updateMultilineInputRange(welcomeMessageInputRef.current), the bug described in this issue can be observed [100% of the time]

Sure, but on the other hand, commenting out the useFocusEffect call makes the issue completely disappear on iOS, am I wrong? Could this observation lead us somewhere?

[cubuspl42]

Yes, that's right but then nothing happens on Firefox and on web in general

Sure, but I didn't say that this is the solution. It's an observation.

My conclusion so far is that our cross-platform goal here is:

Focus the input, as soon as possible, ensuring the cursor is at the end.

We have various platform-dependent constraints here, and we might have to challenge the existing code.

I would expect a solid proposal that proves a full understanding of our focus-related problems and provides a clear, complex solution.

Good starting point:

• useDelayedInputFocus hook, which has no uses (?!)
• useFocusEffect hook usage across the project
  • One can perform a textual search for useFocusEffect( and put attention to the cases which integrate setTimeout + CONST.ANIMATED_TRANSITION. Some are literally copy-pasted. (around 10 cases total?)
• updateMultilineInputRange
  • both the default (which should be .web.js, I think?) and the iOS variant
• an observation that on iOS the CONST.ANIMATED_TRANSITION-delayed focus is not needed and problematic, as we already focus early to work around another issue
  • is the CONST.ANIMATED_TRANSITION-delayed focus at all needed outside of Web? Maybe this should be a Web-specific part of some cross-platform util with platform-dependent implementations?
• an observation that useFocusEffect in general means "call this thingy every time this component is focused", while in many of our uses we probably mean "we actually care about the first time this component gets focuses, and only because we can't do what we want earlier."

@isabelastisser Would you raise the bounty? Although the problem might seem trivial, solving it correctly requires a bit of engineering, as I summed up above.

[situchan]

Another option is to float this around expert contributor group like Callstack.

[melvin-bot]

@cubuspl42 @isabelastisser 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]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[melvin-bot]

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

[melvin-bot]

@cubuspl42, @isabelastisser 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

Upwork job price has been updated to $1000

[isabelastisser]

The bounty has been increased. Waiting for proposals!

[cubuspl42]

I advertised the issue on Slack

[melvin-bot]

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

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

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

[cubuspl42]

🎀 👀 🎀

[melvin-bot]

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

[cubuspl42]

Decide whether any proposals currently meet our guidelines and can be approved as-is today

I don't think so

If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities

Current assignee @cubuspl42 is eligible for the Internal assigner

@techievivek I never entered this path, does it mean that I would be responsible for solving this issue, and you would review the PR?

[mvtglobally]

Issue not reproducible during KI retests. (Third week)

[techievivek]

I never entered this path, does it mean that I would be responsible for solving this issue, and you would review the PR?

Aaah, no, that means that the internal engineer who is assigned to the GH should now start looking into the issue rather than keeping it external and hoping some contributor would work on it.

[techievivek]

It looks like this is no longer reproducible. @cubuspl42 are you still able to reproduce this issue?

[cubuspl42]

@techievivek Ouch. I checked and it's indeed not reproducible. 🙈 I should've caught up that earlier.

[techievivek]

Great, closing it then. Thanks.