[HOLD for payment 2023-11-22] [$500] Web - App adds extra line in edit message box

[kbecciv]

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

---

Action Performed:

1. Open the app
2. Open any report
3. Send any message
4. Edit message and write text to cover full line i.e. if you add any new letter ahead, it should move to new line
5. Save the message
6. Again hover on message and click on Edit message
7. Observe that app has added extra line in edit message box even though cursor is still at the end of previous line

Expected Result:

App should not automatically add extra line below text in edit message box

Actual Result:

App automatically adds extra below text line in edit message box if text occupies full space in previous line

Workaround:

Unknown

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

extra.line.below.text.in.edit.message.mp4

Recording.4538.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694858423819399

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01619a8ea912ef60ce
• Upwork Job ID: 1703461170205720576
• Last Price Increase: 2023-10-08
• Automatic offers:
• aimane-chnaif | Reviewer | 27546726
• bernhardoj | Contributor | 27546728
• dhanashree-sawant | Reporter | 27546732

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01619a8ea912ef60ce

[melvin-bot]

Triggered auto assignment to @peterdbarkerUK (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 @greg-schroeder (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[ahmedGaber93]

Proposal

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

App adds extra line in edit mesage box

What is the root cause of that problem?

We are using updateNumberOfLines to calculate and update the numberOfLines state, which used it textarea row.

App/src/components/Composer/index.js

Line 345 in 6e17b64

const updateNumberOfLines = useCallback(() => {

the issue here is updateNumberOfLines is trigger and calculate numberOfLines before the font applied to the text, and this useCallback dependencies not have dependency when the font applied, so it is not calculated again after the font is applied.
The updateNumberOfLines calculate 2 lines And after the font applied, the font characters seem smaller so it rendered in 1 line

Screen.Recording.2023-09-17.at.1.22.25.AM.mov

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

we need to add textInput.current as a dependency here to trigger the updateNumberOfLines method when font applied

App/src/components/Composer/index.js

Line 364 in 6e17b64

}, [value, maxLines, numberOfLinesProp, onNumberOfLinesChange, isFullComposerAvailable, setIsFullComposerAvailable]);

What alternative solutions did you explore? (Optional)

N/A

[studentofcoding]

Proposal

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

What is the root cause of that problem?

The root problem was on computedNumberOfLines & generalNumberOfLines condition that still calculated & included an extra line due to padding or margin

https://github.com/Expensify/App/blob/01c5b71e8f9d3385a583d669b98579a02936f867/src/components/Composer/index.js#L361C9-L362

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

We need to adjust the calculation of the number of lines in the updateNumberOfLines function. Specifically, if the calculated number of lines was greater than 1, we subtracted 1 from it to remove the extra line. This adjustment ensured that the extra line was not added when making edits with finalNumberOfLines condition below.

    const finalNumberOfLines = generalNumberOfLines > 1 ? generalNumberOfLines - 1 : generalNumberOfLines;

Full function

const updateNumberOfLines = useCallback(() => {
        if (textInput.current === null) {
            return;
        }
        // we reset the height to 0 to get the correct scrollHeight
        textInput.current.style.height = 0;
        const computedStyle = window.getComputedStyle(textInput.current);
        const lineHeight = parseInt(computedStyle.lineHeight, 10) || 20;
        const paddingTopAndBottom = parseInt(computedStyle.paddingBottom, 10) + parseInt(computedStyle.paddingTop, 10);
        setTextInputWidth(computedStyle.width);
    
        const computedNumberOfLines = ComposerUtils.getNumberOfLines(maxLines, lineHeight, paddingTopAndBottom, textInput.current.scrollHeight);
        const generalNumberOfLines = computedNumberOfLines === 0 ? numberOfLinesProp : computedNumberOfLines;
    
        // Subtract 1 from the number of lines if it's greater than 1
        const finalNumberOfLines = generalNumberOfLines > 1 ? generalNumberOfLines - 1 : generalNumberOfLines;
    
        onNumberOfLinesChange(finalNumberOfLines);
        updateIsFullComposerAvailable({isFullComposerAvailable, setIsFullComposerAvailable}, finalNumberOfLines);
        setNumberOfLines(finalNumberOfLines);
        textInput.current.style.height = 'auto';
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [value, maxLines, numberOfLinesProp, onNumberOfLinesChange, isFullComposerAvailable, setIsFullComposerAvailable]);

Result

https://www.loom.com/share/ff227d92866d4c2b835d07ee21299695?sid=28433597-00d6-4ee8-a89b-a5c22307090e

What alternative solutions did you explore? (Optional)

None

[bernhardoj]

Proposal

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

Editing a full-width one-line text message will "add" another line to it.

What is the root cause of that problem?

We have a function updateNumberOfLines that will calculate the number of lines by dividing the scrollHeight (subtracted by vertical padding) of the input with the text input line height.

App/src/components/Composer/index.js

Lines 350 to 369 in 6be8fca

	const updateNumberOfLines = useCallback(() => {
	if (textInput.current === null) {
	return;
	}
	// we reset the height to 0 to get the correct scrollHeight
	textInput.current.style.height = 0;
	const computedStyle = window.getComputedStyle(textInput.current);
	const lineHeight = parseInt(computedStyle.lineHeight, 10) || 20;
	const paddingTopAndBottom = parseInt(computedStyle.paddingBottom, 10) + parseInt(computedStyle.paddingTop, 10);
	setTextInputWidth(computedStyle.width);
	const computedNumberOfLines = ComposerUtils.getNumberOfLines(maxLines, lineHeight, paddingTopAndBottom, textInput.current.scrollHeight);
	const generalNumberOfLines = computedNumberOfLines === 0 ? numberOfLinesProp : computedNumberOfLines;
	onNumberOfLinesChange(generalNumberOfLines);
	updateIsFullComposerAvailable({isFullComposerAvailable, setIsFullComposerAvailable}, generalNumberOfLines);
	setNumberOfLines(generalNumberOfLines);
	textInput.current.style.height = 'auto';
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [value, maxLines, numberOfLinesProp, onNumberOfLinesChange, isFullComposerAvailable, setIsFullComposerAvailable]);

The line height of the text is 20 and the vertical padding is 10, so the scrollHeight will be 30 if it's only one line, but we get 50 in our case and it's because there is a scrollbar that reduced the width of the input which push some character to the 2nd line.

Why there is a scrollbar? Before doing the calculation, we set the height of the input to 0.

App/src/components/Composer/index.js

Lines 354 to 355 in 6be8fca

	// we reset the height to 0 to get the correct scrollHeight
	textInput.current.style.height = 0;

Setting it to 0 will show a scrollbar, so the calculation is affected.

We actually already have a solution to this scrollbar issue by setting an overflow hidden style to the input

App/src/components/Composer/index.js

Lines 442 to 444 in 6be8fca

	// We are hiding the scrollbar to prevent it from reducing the text input width,
	// so we can get the correct scroll height while calculating the number of lines.
	numberOfLines < maxLines ? styles.overflowHidden : {},

but the initial value of numberOfLines is undefined which makes the condition always false. However, this is not the real issue. The real issue is setting the height to 0. This change comes from functional migration PR.

Previously, we don't set the height to 0, but instead, we set the numberOfLines state back to 1 and do the calculation in the setState callback. But in the functional component, there is no easy and clean way to achieve the same behavior as setState callback, so now we are directly mutating the height to 0.

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

Setting the height to 0 is actually not equivalent to numberOfLines. To have the same behavior of setting numberOfLines, we should change the input rows attribute instead to 1 before doing the calculation.

textInput.current.setAttribute('rows', 1);

What alternative solutions did you explore? (Optional)

Set default value of numberOfLines to 0

[peterdbarkerUK]

Hmmm - I was unable to reproduce this. Where am I going wrong @kbecciv ?

[kbecciv]

Try to use the letters to the end of the line and save it. Use edit option and remove if doesn't fit the first line. Use edit option again and you will see the issue.
Adding another video for your review.

Recording.4656.mp4

[ahmedGaber93]

Steps to reproduce.

1. send message "test".
2. edit the message to "mmmmmmmmmmmmmmmmmmm............" to the end of the line and don't keep any space in the end of the line, then save it.
3. click on the edit icon again, you will see an empty extra line is added to the text input field.

Screen.Recording.2023-09-20.at.9.20.05.PM.mov

[peterdbarkerUK]

Hmmm, it's still not happening for me on Chrome web: https://app.screencast.com/Nzdg5mgnIlnx9

[dhanashree-sawant]

Hi @peterdbarkerUK, for me too, I wasn't able to reproduce on 2017 macbook air but was able to reproduce on 15.6 inch windows laptop and 15 inch 2015 macbook pro. It seems like it doesn't occur on all devices.

[bernhardoj]

@peterdbarkerUK the character plays an important role here as each char has a diff width. You can follow this step by @ahmedGaber93 by typing all char as "m". (I can also reproduce it with "a") (probably this is not the reason, read below)

Make sure "Show scroll bars" setting has either "Automatically based on mouse or trackpad" or "Always" selected

[tsa321]

Proposal

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

Web - Application add extra line in edit message box if lowest text input line is full when click on edit message.

What is the root cause of that problem?

The root cause of the problem is on this line:

App/src/components/Composer/index.js

Line 444 in 7bebfd3

numberOfLines < maxLines ? styles.overflowHidden : {},

The style for first created composer component won't use styles.overflowHidden because initial numberOfLines values is undefined.
So boolean value of undefined < maxLines will always be false and the styles.overflowHidden won't be added to the composer style.
This style is important to determine the scrollHeight of text input. without it, textInput will not hide vertical scrollbar and scrollHeight value will be higher. Then first created component will calls on the updateNumberOfLines based on this scrollHeight and will return inaccurate value.

The issue is really visible if there are many lines in text input, even if it is not last character of lowest line the blank line is added.
Here is a demo of the overflowHidden effect on the text input:

Overly hidden demo:

overflow_hidden_d.mp4

As you can see, the calculation of the numberOfLines is quite off if the `overflow : hidden` style is not present.

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

To properly check on numberOfLines < maxLines ? styles.overflowHidden : {}
we can modify this check into: (!numberOfLines || (numberOfLines < maxLines)) ? styles.overflowHidden : {}

[melvin-bot]

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

[melvin-bot]

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[bernhardoj]

PR is ready

cc: @aimane-chnaif

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.99-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix an extra line is added when editing a one full line of a message #30964

If no regressions arise, payment will be issued on 2023-11-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @bernhardoj requires payment offer (Contributor)
• @aimane-chnaif requires payment offer (Reviewer)
• @dhanashree-sawant requires payment offer (Reporter)

[melvin-bot]

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:

• [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif] 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:
• [@aimane-chnaif] 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:
• [@aimane-chnaif] Determine if we should create a regression test for this bug.
• [@aimane-chnaif] 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.
• [@peterdbarkerUK] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[peterdbarkerUK]

@aimane-chnaif could you take care of the checklist?

[$500] to @bernhardoj (Contributor)
[$500] to @aimane-chnaif (Reviewer)
[$50] to @dhanashree-sawant (Reporter)

All paid

[aimane-chnaif]

• The PR that introduced the bug has been identified. Link to the PR: Migrate Composer. web #23359
• 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: Migrate Composer. web #23359 (comment)
• 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 edge case and happened on Windows only. So we can skip regression test.

[peterdbarkerUK]

Thank you very much!