[HOLD for payment 2023-06-07] [$1000] The horizontal line (for deleted message) is not in the center for the text 'edited' but is center for the 'chat messages' on web chrome (offline mode)

[kavimuru]

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. Go to web chrome (online mode)
2. Go to any chat
3. Send a text message
4. Edit the message
5. Click on save
6. Now turn off your wifi or click on force offline from the preference page
7. Now delete the message
8. Notice that the horizontal line for the text 'edited' is not at the center unlike other chat messages
9. But if you follow the same steps on android , you'll see the horizontal line for the text 'edited' at the center only

Expected Result:
The horizontal line for the edited text should be at the center for the word 'edited' in a similar how android shows it

Actual Result:

The horizontal line for the word 'edited' is not at the center unlike android

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.2.97-2
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

error-2023-04-07_15.48.05.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680862318324249

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0118d19fc456de16df
• Upwork Job ID: 1645467006413078528
• Last Price Increase: 2023-04-17

[MelvinBot]

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

[MelvinBot]

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

[JmillsExpensify]

Assigning myself since I was also pulled into the linked Slack convo.

[JmillsExpensify]

I'm able to reproduce the issue, and while a small visual issue, let's see if the community has ideas on how we can fix this. If we find a great root cause that's fixable we should do something.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~0118d19fc456de16df

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[pubudu-ranasinghe]

Proposal

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

What is the root cause of that problem?

If we inspect this in the browser we can see the following elements and styles that render the message and edited label.

We have a div with larger text and line-through applied which displays the message. Inside it there's a span with smaller text which displays edited label. As line-through is applied to the outer div, the browser tries to render the line on the entire block. But because the font sizes are different, the line is not properly aligned. (AFAIK css spec does not have instructions on how to handle line-through with varying font sizes)

This behavior is a common issue and can be reproduced outside of Expensify. We can fix this by making the inner smaller text to be a separate block and apply line-through to that separately.

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

Following is the component that renders the message and edited label.

App/src/pages/home/report/ReportActionItemFragment.js

Lines 126 to 140 in d196a12

	<Text
	family="EMOJI_TEXT_FONT"
	selectable={!DeviceCapabilities.canUseTouchScreen() || !props.isSmallScreenWidth}
	style={[EmojiUtils.containsOnlyEmojis(text) ? styles.onlyEmojisText : undefined, styles.ltr, ...props.style]}
	>
	{StyleUtils.convertToLTR(Str.htmlDecode(text))}
	{props.fragment.isEdited && (
	<Text
	fontSize={variables.fontSizeSmall}
	color={themeColors.textSupporting}
	>
	{` ${props.translate('reportActionCompose.edited')}`}
	</Text>
	)}
	</Text>

We can make the Text compoent in L133 to have styles inline-block and also include the line-through. To do that we just need to update the styles as follows in L136

style={[...props.style, {display: 'inline-block'}]}

Handling Markdown

As pointed out by @sobitneupane the above solution alone doesn't address markdown comments. They have the same root cause described above but is rendered separately. So we need to add a fix there as well.

Markdown is rendered by following lines of code

App/src/pages/home/report/ReportActionItemFragment.js

Lines 112 to 124 in 2d0ef06

	if (!differByLineBreaksOnly) {
	const isPendingDelete = props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && props.network.isOffline;
	const editedTag = props.fragment.isEdited ? '<edited></edited>' : '';
	const htmlContent = applyStrikethrough(html + editedTag, isPendingDelete);
	return (
	<RenderHTML
	html={props.source === 'email'
	? `<email-comment>${htmlContent}</email-comment>`
	: `<comment>${htmlContent}</comment>`}
	/>
	);
	}

This renders a custom <edited> element defined in src/components/HTMLEngineProvider/HTMLRenderers/EditedRenderer.js. We can add the same styles inline-block and line-through mentioned above to fix this. But this needs to be added under the condition of app being offline and has pending delete. Since there's no direct way to pass a prop into the EditedRenderer we can use an attribute to pass this information from the ReportActionItemFragment

This is how we can pass this attribute in ReportActionItemFragment L114

const editedTag = props.fragment.isEdited ? `<edited ${isPendingDelete ? 'deleted="true"' : ''}></edited>` : '';

EditedRenderer needs to be updated as follows to read this attribute and apply the styles

// Component Code
const isPendingDelete = !!props.tnode.attributes.deleted;
// component jsx
style={isPendingDelete ? [styles.offlineFeedback.deleted, { display: 'inline-block'}] : undefined}

These changes will not have any impact on the web because span is already inline and there's no visual difference between inline and inline-block. Also it will not have any impact on native apps as RN does not support inline-block and so will be ignored. And yes, this works fine with edited single emoji.

Before

After

What alternative solutions did you explore? (Optional)

Setting the strike through using a pseudo element rather than browser native strike through was another option. This would give greater flexibility in positioning but I'd prefer to use browser features as much as possible.

[MelvinBot]

📣 @pubudu-ranasinghe! 📣

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. 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.
2. 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.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[pubudu-ranasinghe]

Contributor details
Your Expensify account email: pubudutr@gmail.com
Upwork Profile Link: upwork.com/freelancers/~019eaf0e2e67ff1536

[MelvinBot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[sobitneupane]

Thanks for the proposal @pubudu-ranasinghe

Can you please add more explanation in "What is the root cause of that problem?" section. Please make use of permalink to link the places where the issue exists.

Can you please add more details in "What changes do you think we should make in order to solve the problem?" section as well. Please do include how you are going to achieve the suggested change. Also please let us know how will it impact native apps. Have you tested your solution with edited single emoji?

[pubudu-ranasinghe]

Proposal

Updated

@sobitneupane

[sobitneupane]

@pubudu-ranasinghe Thanks for the update.

Your proposal won't solve the issue for all the cases. It won't work in case of markdowns. Try your solution with inline codeblock (`abc`).

[pubudu-ranasinghe]

@sobitneupane yes you are correct. This solution alone was not handling html comments. I've updated the original proposal to include a fix for this.

Proposal

Updated

[trjExpensify]

@sobitneupane thoughts?

[parasharrajat]

To be clear on this, is this issue caused by #17704? @sobitneupane

[sobitneupane]

No, it is not. We were going to make use of changes made by #17704. But looks like it is reverted.

[sobitneupane]

Waiting for #18523 PR to merge before proceeding with our PR as we will be using the change introduced in former PR.

[melvin-bot]

@JmillsExpensify, @pubudu-ranasinghe, @trjExpensify, @NikkiWines, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@JmillsExpensify, @pubudu-ranasinghe, @trjExpensify, @NikkiWines, @sobitneupane Still overdue 6 days?! Let's take care of this!

[sobitneupane]

The associated PR has now merged to main. We will be continuing with our PR.

[trjExpensify]

Dope!

[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.20-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix Edited Line Through Alignment #17781

If no regressions arise, payment will be issued on 2023-06-07. 🎊

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

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

[trjExpensify]

👋 @sobitneupane can you run through the checklist please so we can move on with processing payments? Thanks!

[sobitneupane]

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:

I tried to go back to few commits to find the exact PR which introduced the issue. But could not point to the exact PR Looks like it was present from the very beginning when edited label was introduced in this PR.

[sobitneupane]

Regression Test Proposal

1. Open any chat
2. Send few messages like normal text, emoji, and markdown
3. Edit the messages
4. Disable your network or force offline from preferences
5. Delete the edited messages
6. Verify that the line through in edited label is aligned properly for all message types

Do we agree 👍 or 👎

[trjExpensify]

I tried to go back to few commits to find the exact PR which introduced the issue. But could not point to the exact PR Looks like it was present from the very beginning when edited label was introduced in #2320.

Cool, fair enough. We comment there then! :)

As for the regression test, we have this one for editing a comment. We could pop it in the offline section. That said, I'd love to offer more direction than just "aligned properly", because who knows if the tester knows what's proper or not. Center aligned? What do you think?

Online

1. Navigate a conversation (with another user you have access to) in the main testing device
2. Hover over an owned message (Web/Desktop) / Tap and hold (Mobile) to view the action menu
3. Click/Tap on the Pencil icon to activate the edit message box
4. Edit the message to "0" and save the changes
5. Verify the edited message is still displayed in the conversation viewport (it should not scroll out of view)
6. Verify the message is displaying the new text, has a "(edited)" status in the conversation history and that the text is not grayed out
7. Send a message that includes a hyperlink i.e google.com
8. Edit the message with the hyperlink (do not modify the link) and save the changes
9. Verify the link is still hyperlinked in the modified message
10. Now edit the domain part of the link, while keeping the text the same but changing the capitalization of the domain letters i.e google.com
11. Verify the link is still hyperlinked in the modified message and the domain part maintains its original capitalization
12. Log in as the other user in the conversation
13. Verify the edited message is displaying the new text and has a "(edited)" status in the conversation history

Offline

1. Disable the internet connection in the main testing device
2. Hover over an owned message (Web/Desktop) / Tap and hold (Mobile) to view the action menu
3. Click/Tap on the Pencil icon to activate the edit message box
4. Verify you're able to edit the message while there's no internet connection
5. Edit the message to "0" and save the changes
6. Verify the comment text is updated it's greyed out indicating it's pending
   7. Verify that the line through in edited label is aligned properly
7. Click/tap the pencil icon again
8. Edit the message to something new
9. Verify the “save changes” button is enabled, hit the button
10. Verify the comment text is updated it's greyed out indicating it's pending
11. Refresh the page or force-kill and relaunch the app
12. Enable the internet connection in the device - in web you might need to refresh the page after going back online
13. Verify the comment is not be grayed out anymore indicating it was updated successfully

[trjExpensify]

Bump on this @sobitneupane:

That said, I'd love to offer more direction than just "aligned properly", because who knows if the tester knows what's proper or not. Center aligned? What do you think?

[sobitneupane]

Yup. Center aligned will make it clear.

As for the regression test, we have this one for editing a comment. We could pop it in the offline section.

I don't think we can add it in the section you mentioned above. We need to delete the message after editing in offline.

[trjExpensify]

Ah yeah fair, okay.. I'll run it by applause. It could go in the delete offline tests perhaps. Issue created here.

Okay, let's proceed with the payments. I've sent offers for:

$1,000 to @sobitneupane for C+
$1,000 to @pubudu-ranasinghe for the fix
$250 to @priya-zha for the bug report

[trjExpensify]

@sobitneupane paid!

[trjExpensify]

@priya-zha paid!

[trjExpensify]

@pubudu-ranasinghe paid! Closing, thanks everyone.