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

[HOLD #31705][$500] Desktop - Compose Box - When the app reloads, "Write something..." text and the cursor moves up #31744

Closed
1 of 6 tasks
lanitochka17 opened this issue Nov 22, 2023 · 30 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Nov 22, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.2-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:

Action Performed:

  1. Open New Expensify app
  2. Navigate to any conversation
  3. Reload the application(CMD+R)
  4. Click on "Go back to home page" (if required)

Expected Result:

When the application reloads, "Write something..." text and the cursor should be centered in the Compose Box

Actual Result:

When the application reloads, "Write something..." text and the cursor moves up in the Compose Box

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

Bug6287583_1700678534952.Recording__762.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014ec0e25eb59f5d17
  • Upwork Job ID: 1727422130676625408
  • Last Price Increase: 2023-11-29
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 22, 2023
@melvin-bot melvin-bot bot changed the title Desktop - Compose Box - When the app reloads, "Write something..." text and the cursor moves up [$500] Desktop - Compose Box - When the app reloads, "Write something..." text and the cursor moves up Nov 22, 2023
Copy link

melvin-bot bot commented Nov 22, 2023

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

Copy link

melvin-bot bot commented Nov 22, 2023

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

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

melvin-bot bot commented Nov 22, 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

Copy link

melvin-bot bot commented Nov 22, 2023

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

@erquhart
Copy link
Contributor

Not seeing this in dev.

@mkhutornyi
Copy link
Contributor

mkhutornyi commented Nov 22, 2023

Proposal

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

Composer style is broken when navigate back from not found page after reload

What is the root cause of that problem?

const computedNumberOfLines = ComposerUtils.getNumberOfLines(lineHeight, paddingTopAndBottom, textInput.current.scrollHeight, maxLines);

textInput.current.scrollHeight is set to 0 so this function returns 0 which makes numberOfLines also 0 and thus rows prop value
rows={numberOfLines}

I think this is race condition. scroll height is 0 because rows is 0

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

Pass rows value always bigger than 0
So either set default value of numberOfLines to 1 here or update this line to rows={numberOfLines || 1}

@melvin-bot melvin-bot bot added the Overdue label Nov 27, 2023
@sakluger
Copy link
Contributor

First of all, why does reloading the page take you to the "Hmm...it's not here" page? That seems like a bigger bug. Does that only happen on Desktop or on other platforms too?

For the compose box spacing issue, @mkhutornyi do you have any ideas why it's only happening on Desktop and not on other platforms?

@melvin-bot melvin-bot bot removed the Overdue label Nov 27, 2023
@mkhutornyi
Copy link
Contributor

First of all, why does reloading the page take you to the "Hmm...it's not here" page? That seems like a bigger bug.

It's known bug - #28495, #31592

Cursor issue happens on web as well.

Here's simple repro step:

  1. Navigate to any url that doesn't exist i.e. https://staging.new.expensify.com/test
  2. Close not found page

Copy link

melvin-bot bot commented Nov 29, 2023

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

Copy link

melvin-bot bot commented Nov 30, 2023

@sakluger, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Nov 30, 2023
@sakluger
Copy link
Contributor

sakluger commented Dec 1, 2023

Thanks for sharing those other issues! I looked but couldn't find them.

@aimane-chnaif what do you think about @mkhutornyi's proposed solution?

@melvin-bot melvin-bot bot removed the Overdue label Dec 1, 2023
@aimane-chnaif
Copy link
Contributor

set default value of numberOfLines to 1 here

This solution sounds good.

@bernhardoj did you have any reason for setting this value to 0, not 1?

@bernhardoj
Copy link
Contributor

The numberOfLines before is undefined and 0 is the equivalent of it.

We had discussed it before here whether to set it as 0 or 1.

@aimane-chnaif
Copy link
Contributor

The numberOfLines before is undefined and 0 is the equivalent of it.

We had discussed it before here whether to set it as 0 or 1.

Ah yes, I remember. But I don't see any difference between 0 or 1 even when multiline

@bernhardoj
Copy link
Contributor

But I don't see any difference between 0 or 1 even when multiline

I think we should look at the PR that implemented it. This is the commit that changed it from 1 to undefined.

But I think changing it to 1 (or any other value) is not the solution here. I checked that the issue is the numberOfLines calculation is always 0. If the composer has a draft with multiline text, having a 1 numberOfLines will only show the 1st line of the text (haven't tested it yet because the draft is always cleared after refresh, another bug?).

@aimane-chnaif
Copy link
Contributor

because the draft is always cleared after refresh

This is known bug - #31705

Maybe we can put on hold a bit since we cannot test all cases without fixing that bug

@bernhardoj
Copy link
Contributor

While waiting for the draft issue, here is my proposal

Proposal

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

Open a not found page and then press back will show a composer with 2 lines.

What is the root cause of that problem?

When we open a not found page (e.g. /asd), the nav stack looks like this:
[LHN, ReportScreen, Not Found]

In react-navigation, a previous not focused screen will have a display: none style applied to it, in our case, the ReportScreen is not focused, therefore a display: none is applied to it (except the navigator has presentation: 'transparentModal', for example, RHP). Because the display is none, getting the scrollHeight of the composer input will always return 0 (ref).

And so the number of lines calculation always returns 0. A number of lines/rows is 0 is the same as not setting the rows property to the textarea and by default, it will have 2 lines.

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

One trick to keep the report screen displayed is by setting the not found page screen presentation to transparentModal.

<RootStack.Screen
name={SCREENS.NOT_FOUND}
options={screenOptions.fullScreen}
component={NotFoundPage}
/>

options={{
    ...screenOptions.fullScreen,
    presentation: 'transparentModal',
}}

And because we made it transparent, we must set a background color for not found page.

<ScreenWrapper testID={NotFoundPage.displayName} style={[styles.appBG]}>

What alternative solutions did you explore? (Optional)

Re-calculate the number of lines every time the report screen gets focused.

In Composer/index.js

useFocusEffect(useCallback(() => {
    const timeout = setTimeout(() => updateNumberOfLines(), 300);
    return () => clearTimeout(timeout);
}, []));

Without the delay, the display is still none even after when it gets focused

If there is a not found page, don't add a report screen to the navigation.

In CustomRouter.js

+const isNotFoundPageInState = (state) => _.find(state.routes, (r) => r.name === SCREENS.NOT_FOUND)

-if (!isAtLeastOneCentralPaneNavigatorInState(partialState) && !options.getIsSmallScreenWidth()) {
+if (!isAtLeastOneCentralPaneNavigatorInState(partialState) && !options.getIsSmallScreenWidth() && !isNotFoundPageInState(partialState)) {
    addCentralPaneNavigatorRoute(partialState);

@melvin-bot melvin-bot bot added the Overdue label Dec 4, 2023
@sakluger sakluger changed the title [$500] Desktop - Compose Box - When the app reloads, "Write something..." text and the cursor moves up [HOLD #31705][$500] Desktop - Compose Box - When the app reloads, "Write something..." text and the cursor moves up Dec 4, 2023
@sakluger
Copy link
Contributor

sakluger commented Jan 3, 2024

Still on hold for #31705.

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 11, 2024
@sakluger
Copy link
Contributor

Still on hold for #31705.

@melvin-bot melvin-bot bot removed the Overdue label Jan 13, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@aimane-chnaif
Copy link
Contributor

The holding issue is on hold for #32461

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 30, 2024
@sakluger
Copy link
Contributor

Still on hold.

@melvin-bot melvin-bot bot removed the Overdue label Jan 31, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 8, 2024
@sakluger
Copy link
Contributor

sakluger commented Feb 9, 2024

#32461 is still a draft and it looks like a new PR has been raised for that same issue: #35449. Either way, we're still on hold here.

@melvin-bot melvin-bot bot removed the Overdue label Feb 9, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 19, 2024
@sakluger
Copy link
Contributor

Still on hold for #35449.

@melvin-bot melvin-bot bot removed the Overdue label Feb 21, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 29, 2024
@sakluger
Copy link
Contributor

The holding PR is merged and deployed, and I can't seem to reproduce this issue. I think it's fixed!

@aimane-chnaif I'm pretty sure no payment is due on this one, can you please confirm before I close the issue?

@melvin-bot melvin-bot bot removed the Overdue label Mar 11, 2024
@sakluger sakluger added Daily KSv2 and removed Weekly KSv2 labels Mar 11, 2024
@aimane-chnaif
Copy link
Contributor

The bug is not reproducible because of recent regression (maybe navigation refactor) which reloads report screen every time after navigating back.
The root cause is not fixed yet.

@melvin-bot melvin-bot bot added the Overdue label Mar 13, 2024
@aimane-chnaif
Copy link
Contributor

Let's close this for now. I will bump here to reopen when this is reproducible again in the future.

@melvin-bot melvin-bot bot removed the Overdue label Mar 13, 2024
@sakluger
Copy link
Contributor

Sounds good. If we need to reopen this issue in the future, can you please bump in Slack instead? I sometimes miss notifications related to closed GH issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

6 participants