[Payment 2024-10-10] [$250] Chat - In mweb, deleting emoji+text in offline, emoji not striked but striked in Android

[IuliiaHerets]

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: 9.0.33-1
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team

Action Performed:

Precondition: Go offline

1. Launch both mweb and Android
2. Open a chat
3. Paste in composer 🧐 hai
4. Send the message
5. Long press the message and tap delete comment
6. Confirm delete
7. Note the appearance of strike down text in both mweb and Android

Expected Result:

Deleting emoji+text in offline, behavior must be same in both mweb and Android.

Actual Result:

In mweb, deleting emoji+text in offline, emoji is not striked through but in Android it is striked.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6600606_1726116352208.Screenrecorder-2024-09-12-10-06-55-184_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021837299922286222160
• Upwork Job ID: 1837299922286222160
• Last Price Increase: 2024-09-21
• Automatic offers:
• c3024 | Contributor | 104110872

Issue Owner

Current Issue Owner: @sobitneupane

[melvin-bot]

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

[IuliiaHerets]

@strepanier03 FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[c3024]

Edited by proposal-police: This proposal was edited at 2024-09-15 17:53:49 UTC.

Proposal

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

Emoji is not struck through in web apps when a comment with both emoji and text is deleted offline.

What is the root cause of that problem?

When the comment only contains emojis, we add the line-through style here:

App/src/pages/home/report/comment/TextCommentFragment.tsx

Line 92 in 0c618ac

styleAsDeleted ? styles.offlineFeedback.deleted : undefined,

So it works fine when the comment contains only emojis.

However, when the deleted message contains both emojis and text, we use RenderCommentHTML to show the message, which in turn uses RenderHTMLSource from react-native-render-html.

App/src/pages/home/report/comment/TextCommentFragment.tsx

Lines 60 to 72 in 0c618ac

	if (!shouldRenderAsText(html, text ?? '') && !(containsOnlyEmojis && styleAsDeleted)) {
	const editedTag = fragment?.isEdited ? `<edited ${styleAsDeleted ? 'deleted' : ''} ${containsOnlyEmojis ? 'islarge' : ''}></edited>` : '';
	const htmlWithDeletedTag = styleAsDeleted ? `<del>${html}</del>` : html;
	const htmlContent = containsOnlyEmojis ? Str.replaceAll(htmlWithDeletedTag, '<emoji>', '<emoji islarge>') : htmlWithDeletedTag;
	let htmlWithTag = editedTag ? `${htmlContent}${editedTag}` : htmlContent;
	if (styleAsMuted) {
	htmlWithTag = `<muted-text>${htmlWithTag}<muted-text>`;
	}
	return (
	<RenderCommentHTML

We add del tag here

App/src/pages/home/report/comment/TextCommentFragment.tsx

Line 62 in 0c618ac

const htmlWithDeletedTag = styleAsDeleted ? `<del>${html}</del>` : html;

and react-native-render-html adds the line-through style based on this tag.

But the custom EmojiRenderer is not passed the style prop here:

App/src/components/HTMLEngineProvider/HTMLRenderers/EmojiRenderer.tsx

Line 6 in 0c618ac

function EmojiRenderer({tnode}: CustomRendererProps<TText | TPhrasing>) {

So, the textDecorationStyle is not applied to the EmojiWithTooltip, and consequently to the emoji.

This works fine on native apps because the parent style still has the correct textDecorationStyle, and this is correctly inherited in native apps, which use only the Text element here:

App/src/components/EmojiWithTooltip/index.native.tsx

Line 5 in 0c618ac

return <Text style={style}>{emojiCode}</Text>;

However, in the index.tsx, there are interspersed View components between the Text and its parent here:

App/src/components/EmojiWithTooltip/index.tsx

Lines 18 to 36 in 0c618ac

	<View style={[styles.alignItemsCenter, styles.ph2]}>
	<View style={[styles.flexRow, styles.emojiTooltipWrapper]}>
	<Text
	key={emojiCode}
	style={styles.onlyEmojisText}
	>
	{emojiCode}
	</Text>
	</View>
	<Text style={[styles.textMicro, styles.fontColorReactionLabel]}>{`:${emojiName}:`}</Text>
	</View>
	),
	[emojiCode, emojiName, styles.alignItemsCenter, styles.ph2, styles.flexRow, styles.emojiTooltipWrapper, styles.fontColorReactionLabel, styles.onlyEmojisText, styles.textMicro],
	);
	return (
	<Tooltip renderTooltipContent={emojiTooltipContent}>
	<Text style={[style, styles.cursorDefault]}>{emojiCode}</Text>
	</Tooltip>

As a result, the textDecorationStyle is not correctly inherited in non-native apps.

Text style inheritance is only available within Text subtrees. The consequence of this is that default text styles cannot be set on View for an entire subtree. "

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

We should pass the style prop to EmojiRenderer, add necessary styles, and pass them to EmojiWithTooltip here:

App/src/components/HTMLEngineProvider/HTMLRenderers/EmojiRenderer.tsx

Lines 6 to 11 in 0c618ac

	function EmojiRenderer({tnode}: CustomRendererProps<TText | TPhrasing>) {
	const styles = useThemeStyles();
	const style = 'islarge' in tnode.attributes ? styles.onlyEmojisText : {};
	return (
	<EmojiWithTooltip
	style={[style, styles.cursorDefault, styles.emojiDefaultStyles]}

like this:

function EmojiRenderer({style, tnode}: CustomRendererProps<TText | TPhrasing>) {
    const styles = useThemeStyles();
    const mergedStyle = {...style, ...'islarge' in tnode.attributes ? styles.onlyEmojisText : {}};
    return (
        <EmojiWithTooltip
            style={[mergedStyle, styles.cursorDefault, styles.emojiDefaultStyles]}

What alternative solutions did you explore? (Optional)

[melvin-bot]

@strepanier03 Eep! 4 days overdue now. Issues have feelings too...

[strepanier03]

Okay, got this tested and easy to repro. The question is what do actually want because I think just the word being strikes is expected, but I think them both striked looks better.

I'll confirm quick.

[strepanier03]

Okay, this is good to move forward.

The end goal is consistency and the majority int he discussion voted for fully struck through, emoji and text.

Let's move forward with making that consistent across devices. If there is a limitation let's discuss it.

[melvin-bot]

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

[melvin-bot]

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

[strepanier03]

Heads up

I am out of the office from September 21 to October 3. If during that time BZ action is needed please post in Slack to ask for a hand getting it done. I'll check in when I'm back online on the 3rd and get caught up.

Thanks everyone!

[sobitneupane]

Thanks for the proposal @c3024

Proposal from @c3024 looks good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @c3024 🎉 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]

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

[melvin-bot]

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

• pass style to EmojiRenderer #49730

If no regressions arise, payment will be issued on 2024-10-10. 🎊

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

• @sobitneupane requires payment through NewDot Manual Requests
• @c3024 requires payment automatic offer (Contributor)

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

[strepanier03]

Payment is due to be released in a few days, will check back in then.

[strepanier03]

Payment Summary

• @sobitneupane - $250 via New Expensify
• @c3024 - $250 via Upwork

[strepanier03]

@sobitneupane - I'll check back later today for the checklist and once it's done I'll take the payment summary off hold.

[strepanier03]

@c3024 - I paid your contract and closed it in Upwork. Thank you!

[melvin-bot]

@yuwenmemon @strepanier03 @sobitneupane @c3024 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[strepanier03]

@sobitneupane - friendly bump on the checklist here so I can finish this up.

[strepanier03]

@sobitneupane I set ownership of this on you. Once you do the checklist please set it back to me and I'll remove the hold from the payment summary.

[melvin-bot]

@yuwenmemon, @strepanier03, @sobitneupane, @c3024 Whoops! This issue is 2 days overdue. Let's get this updated quick!

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

• The PR that introduced the bug has been identified. Link to the PR:

#37814

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

#37814 (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:
• Determine if we should create a regression test for this bug.

Yes.

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

#49053 (comment)

[sobitneupane]

Regression Test Proposal

• Go offline
• Send a message with emoji (for example, 😄 hello) in any chat
• Delete the message
• Verify that the emoji is struck through

Do we agree 👍 or 👎

[strepanier03]

https://github.com/Expensify/Expensify/issues/437795

[strepanier03]

@sobitneupane - Payment summary is posted here.

I removed the hold and made the reg test GH so you're good to request payment.

[garrettmknight]

$250 approved for @sobitneupane