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.31k stars 2.74k forks source link

[$250] iOS - Composer - "Write something..." is not in the middle of the composer #46443

Closed lanitochka17 closed 1 month ago

lanitochka17 commented 1 month 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: 9.0.14-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch New Expensify app
  2. Open any chat

Expected Result:

"Write something..." will be in the middle of the composer

Actual Result:

"Write something..." is not in the middle of the composer

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/789a4e8d-3ebe-4bb5-949f-b488d41fb344

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013fd28700fc476d2f
  • Upwork Job ID: 1818012292903295394
  • Last Price Increase: 2024-07-29
Issue OwnerCurrent Issue Owner: @marcochavezf
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @marcochavezf (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
lanitochka17 commented 1 month ago

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

roryabraham commented 1 month ago

not a back-end deploy blocker, but I think since this negatively affects a very core component of the app it should be treated as a blocker

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~013fd28700fc476d2f

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

mkhutornyi commented 1 month ago

https://github.com/Expensify/App/pull/40692 might be the culprit

roryabraham commented 1 month ago

agree, was just coming to post the same thing. Would love to see this fixed rather than just reverting that PR which took a long time to land

stephanieelliott commented 1 month ago

Cool, so @roryabraham are you thinking we treat this as if it's a regular bug and get proposals on it?

ShridharGoel commented 1 month ago

Proposal

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

iOS - Composer - "Write something..." is not in the middle of the composer.

What is the root cause of that problem?

The value of lineHeightEmojisWithTextComposer it 22, which makes the text shift downwards in the composer.

https://github.com/Expensify/App/blob/2ff7775a2910bce14d6d55e1dc9c9e558aa68721/src/styles/index.ts#L1714

The composerStyle is suitable for emojis, but not when there's only text. We need to handle:

https://github.com/Expensify/App/blob/ba25cb3400ab92ea3c2b618631da8e715b2c6644/src/components/Composer/index.native.tsx#L76-L79

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

Check if message contains text and emoji both:

function containsTextWithEmojis(message: string): boolean {
    const trimmedMessage = message.replace(/\s+/g, '');
    const emojisRegex = new RegExp(CONST.REGEX.EMOJIS, CONST.REGEX.EMOJIS.flags.concat('g'));

    const emojiMatches = trimmedMessage.match(emojisRegex);

    if (!emojiMatches) {
        return false;
    }

    const emojiCodes = [];
    emojiMatches.forEach((emoji) => {
        getEmojiUnicode(emoji).split(' ').forEach((code) => {
            if (!(CONST.INVISIBLE_CODEPOINTS as readonly string[]).includes(code)) {
                emojiCodes.push(code);
            }
        });
    });

    const messageChars = [...trimmedMessage];

    const containsText = messageChars.some((char) => {
        const unicode = getEmojiUnicode(char);
        return !emojiCodes.includes(unicode) && unicode.length > 0 && !(CONST.INVISIBLE_CODEPOINTS as readonly string[]).includes(unicode);
    });

    return containsText;
}

Add containsTextWithEmojis in Composer/index.native.ts:

    const containsTextWithEmojis = useMemo(() => EmojiUtils.containsTextWithEmojis(value ?? ''), [value]);

Update composerStyle in Composer/index.native.ts:

const composerStyle = useMemo(
        () => StyleSheet.flatten([style, doesTextContainOnlyEmojis ? styles.onlyEmojisTextLineHeight : (containsTextWithEmojis ? styles.emojisWithTextLineHeight : styles.lineHeightMarkdownEnabledInput)]),
        [style, doesTextContainOnlyEmojis, styles, containsTextWithEmojis],
    );
Screenshot 2024-07-30 at 1 56 51 AM
melvin-bot[bot] commented 1 month ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

ShridharGoel commented 1 month ago

@roryabraham @francoisl My proposal above should fix this as well.

ShridharGoel commented 1 month ago

https://github.com/user-attachments/assets/8bc77806-9038-4ac5-b2e0-7009581c52ff

marcochavezf commented 1 month ago

Hi @mkhutornyi can you look at @ShridharGoel's proposal?

mkhutornyi commented 1 month ago

Testing. @ShridharGoel can you make sure that your change doesn't cause any other regression in different text display scenarios (with/without emojis)?

hayes65 commented 1 month ago

I still dont understand how does the proposal above fixes this issue ... the issue doesn't happen when you type an emoji and two characters .. so what is the problem with typing an emoji and one character ?

melvin-bot[bot] commented 1 month ago

📣 @hayes65! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
ShridharGoel commented 1 month ago

@mkhutornyi I couldn't find any issue yet with the solution. I've tested:

mkhutornyi commented 1 month ago

@ShridharGoel Looks like you updated proposal without notifying "Proposal updated". Btw your solution isn't safe enough to CP. I want @VickyStash chime in and find proper solution as said here.

ShridharGoel commented 1 month ago

@mkhutornyi I had updated it soon after posting, before the review started. And there was no other proposal, so I thought that the "Proposal Updated" comment can be skipped.

May I know what issues can be caused by the mentioned changes?

roryabraham commented 1 month ago

Cool, so @roryabraham are you thinking we treat this as if it's a regular bug and get proposals on it?

yeah but still treat is as an hourly for now. That would be my inclination

mkhutornyi commented 1 month ago

@mkhutornyi I had updated it soon after posting, before the review started

Change lineHeightEmojisWithTextComposer to 18

I had started reviewing & testing as soon as you posted ^ 😄

Meanwhile, there was slack comment that @VickyStash would be working on fix tomorrow morning.

mkhutornyi commented 1 month ago

@ShridharGoel's proposal looks promising but let's see if QA team can find another bug related to emoji / composer in addition to 2 known bugs so far, so the fix can cover all of them together (as deploy won't happen today).

abzokhattab commented 1 month ago

Do you know why this happens only on iOS? Although iOS and Android share the same styling logic of the composer @mkhutornyi @roryabraham ?

mkhutornyi commented 1 month ago

QA reported another bugs: https://github.com/Expensify/App/issues/46451 https://github.com/Expensify/App/issues/46453

I think we should revert offending PR since it's causing too many blockers. cc: @roryabraham

melvin-bot[bot] commented 1 month ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

stitesExpensify commented 1 month ago

Personally I agree with a revert since there are now 3 deploy blockers / regressions linked

melvin-bot[bot] commented 1 month ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

marcochavezf commented 1 month ago

I created a revert here to unblock the deploy today

melvin-bot[bot] commented 1 month ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

roryabraham commented 1 month ago

We CP'd the revert, so this should be fixed now. Will retest on TF...

roryabraham commented 1 month ago

confirmed this is fixed ✅