[HOLD for payment 2023-06-30] [$1000] Chat - Deleted messages with heading style not strike-through in offline

[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. Go to URL https://staging.new.expensify.com/
2. Login to ND
3. Go to a chat and add a comment, > # Example
4. Go offline
5. Delete message with quotes & heading style

Expected Result:

Deleted messages with quotes & heading style not strike-through in offline

Actual Result:

Unknown

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

Reproducible in staging?: Yes

Reproducible in production?: yes

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

https://platform.applause.com/services/links/v1/external/46a8d74188263bb1897e1e4a7749ded33d49e43661aebf91b349fb4bd53e5edf

Bug6048345_Screen_Recording_20230509_193244_Chrome.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010211de21c6d5876a
• Upwork Job ID: 1656697403382730752
• Last Price Increase: 2023-05-18

[melvin-bot]

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

[twisterdotcom]

I think this is actually just Headers that are showing with underline rather than strikethrough:

It doesn't matter if there is a quote or not.

[twisterdotcom]

Created another post for the desktop issue: https://expensify.slack.com/archives/C049HHMV9SM/p1683822276071329

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[Nikhil-Vats]

Proposal

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

When messages are deleted in offline mode, strikethrough looks like an underline for headings (# Example heading).

Note - I would also like to add that I noticed this behaviour in the web version in Chrome and Brave but not Safari. Moreover, the web version had issues on the laptop but not on the phone. I also noticed this issue in the desktop app on Mac.

What is the root cause of that problem?

The CSS property text-decoration-line: line-through; shows a strikethrough line through the middle of the text but the position of this line is calculated differently in case of different browsers.

In case of Chrome, Brave, desktop version the strikethrough appears as the underline because the strikethrough line position is being calculated based on the font-size of the h1 tag (30px on laptop) for headings but there is another div (with display set to inline) inside of it which reduces the font-size of text to 17px (on laptop). This messes up the position of the line and makes it look like an underline.

If you notice carefully, the thickness of the line is also wrong for headings since it is being calculated incorrectly. The thickness is actually more than the thickness of the letters.

<body>
    <div style="text-decoration-line: line-through;">
        <h1 style="font-size: 30px; text-decoration-line: none;">
            <div style="display: inline; font-size: 10px; text-decoration-line: none;">
                My heading
            </div>
        </h1>
    </div>
</body>

You can test this code in safari, the strikethrough will look correct but the same code will show an underline in chrome.

I tried to find out how strikethrough position is calculated but found very little information but here are my observations -

1. Safari - Strikethrough position is calculated based on the size of text (the innermost child's font-size or the actual font-size of text) hence the position is correct there.
2. Chrome - The position is calculated based on the font-size of the innermost child with display set to block. So, in our case the position was calculated assuming font-size was 30px but the div inside h1 changes it to 17px which messes up the position. I also noticed that the thickness of the line also follows the same logic.

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

In order to fix this, I think we should set font-size of h1 to match div so that even if the position is calculated based on h1 but it will be correct since font-size is the same. This will also fix the thickness.

We can't change the display of inner div to block since it will have regressions.

What alternative solutions did you explore? (Optional)

NA

Result -

[Nikhil-Vats]

@twisterdotcom

The # not only creates a header, but it also sets it's as the subject line for the email, so I think we shouldn't make the header work inside a quote?

Weird. Well, I guess we should make it work? I will ask tomorrow.

Let me know your thoughts about these.

[cristipaval]

Not overdue.

[mollfpr]

@Nikhil-Vats Could you elaborate on your solution? What and where are the changes needed to our codebase?

[twisterdotcom]

Sorry @Nikhil-Vats - I don't think the email subject is a problem at all. I was just going through the motions up there. I'll mark it as outdated.

[Nikhil-Vats]

Thanks for the clarification @twisterdotcom.
@mollfpr I was sick for the last couple of days so couldn't update it. I will update the proposal tomorrow evening.

[twisterdotcom]

How is the proposal going @Nikhil-Vats?

[Nikhil-Vats]

@twisterdotcom Upon further investigation, I can't find a solution I am satisfied with. Here's what's happening in a bit more detail -

1. For rendering these headers, we use RenderHTML component which uses BaseHTMLEngineProvider for rendering -
   

   App/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

   Lines 57 to 73 in 19e6837

   	const BaseHTMLEngineProvider = (props) => {
   	// We need to memoize this prop to make it referentially stable.
   	const defaultTextProps = useMemo(() => ({selectable: props.textSelectable, allowFontScaling: false}), [props.textSelectable]);
   	// We need to pass multiple system-specific fonts for emojis but
   	// we can't apply multiple fonts at once so we need to pass fallback fonts.
   	const fallbackFonts = {'ExpensifyNeue-Regular': fontFamily.EMOJI_TEXT_FONT};
   	return (
   	<TRenderEngineProvider
   	customHTMLElementModels={customHTMLElementModels}
   	baseStyle={styles.webViewStyles.baseFontStyle}
   	tagsStyles={styles.webViewStyles.tagStyles}
   	enableCSSInlineProcessing={false}
   	systemFonts={_.values(fontFamily)}
   	fallbackFonts={fallbackFonts}
   	>

2. In the BaseHTMLEngineProvider, we use TRenderEngineProvider for improving performance, we provide tagStyles for headings to reduce the font-size -
   

   App/src/styles/styles.js

   Lines 136 to 139 in 19e6837

   	h1: {
   	fontSize: variables.fontSizeLarge,
   	marginBottom: 8,
   	},

   
   This reduced the font-size to 17px on desktop but the font-size is actually changed for the div inside h1 rather than h1 itself. h1 gets it font size from the user agent stylesheet (2em ~ 30px).
   See screenshot for proof

   

Possible solutions -

1. Workaround - I could find no way to change the fontSize for h1 except either overriding the user agent stylesheet using CSS or using a custom renderer which I don't like even as a workaround -

   h1: (props) => (
       <h1 style={{fontSize: variables.fontSizeLarge}}>
           <TNodeChildrenRenderer tnode={props.tnode} />
       </h1>
   )

2. Potential Solution - Another way to fix this would be to use <del> tag rather than styles to apply strikethrough as we do for native apps -
   

   App/src/components/HTMLEngineProvider/applyStrikethrough/index.native.js

   Lines 1 to 8 in 021657b

   	function applyStrikethrough(html, isPendingDelete) {
   	if (isPendingDelete) {
   	return `<del>${html}</del>`;
   	}
   	return html;
   	}
   	export default applyStrikethrough;

   
   But right now, this function is only used in ReportActionItemFragment but strikethrough is used for offline deletion at other places like deleting payment methods as well.
   
   So we will need to use this function at multiple places or find out a way to do it in one place like the OfflineWithFeedback component. This was actually discussed here but skipped because of the lack of a good solution.

I am open to coming up with an approach for the Potential solution if you think it is okay to replace CSS styles in OfflineWithFeedback with the del tag in a unified way.

[melvin-bot]

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

[twisterdotcom]

@mollfpr this has now gone over my head. Could I ask for your insight please?

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

• fix offline deleted heading link style and link padding #20996

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

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:

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

[melvin-bot]

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

[twisterdotcom]

Hey @slafortune - I'm OOO for the rest of the week. Could you please handle the payment and test case if required?

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:

#17496

[@mollfpr] 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:

#17496 (comment)

[@mollfpr] 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:

This should be cached while QA is happened, so regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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.

Prerequisites:
Set offline the network by going to Settings -> Preferences and toggle on the Force offline

1. Open the app in Chrome on your desktop.
2. Go to a chat and add a heading comment. eg: # Example
3. Go offline by switching off the connection.
4. Delete the message.
5. Verify that the message is shown with a strikethrough style (line over the message) and not underlined.

Additionally, thoroughly test every markdown style.

Note -

1. Repeat the steps above for the following messages (feel free to add more) -
   Messages are separated by a new empty line.

# text header

# header with link.com

# header with emoji 😇

# 😇😇

> # header inside quote with [link](google.com)

`# try header in code`

\```
multiline code (remove \ before three backticks above and below)
😅
link.com
\```

😋😆😇😇

[normal link](google.com)

2. Also repeat the steps for different attachments - images, PDFs, files without preview like CSVs, docs. Upload them in online mode, go offline and delete them.
   a. After deleting attachments like images, PDFs, GIFs which have preview, their opacity should decrease and user should not be able to open them.
   b. For attachments without previews like CSV, docs, etc. A strikethrough should be shown similar to text comments.
3. Add any attachments in offline mode. App should keep showing "Uploading attachment...". Delete the attachment. App should show a strikethrough over the text.

[slafortune]

Offers sent @mollfpr @Nikhil-Vats 👍

[Nikhil-Vats]

@cristipaval @mollfpr @slafortune This PR was merged after 9 days but it fixes two issues - this one and #17488 and it took a lot of time and effort because it was difficult to come up with an approach that fixed everything without any regression.

So, I just wanted to request you to take this into account while deciding the payment amount.

[cristipaval]

I think @twisterdotcom and @slafortune are the ones who can decide about the payment, as they are the BugZero team members assigned to this issue.

[twisterdotcom]

I think we can bonus this for an extra $500 each. @slafortune have you already paid out? I think you can add a bonus anyway if the contract remains open.

And did you finish the test case? If so, can we close?

[parasharrajat]

On a side note, there was a regression from the first PR #18658 (comment).

[slafortune]

Cool! Paid with bonus - Thanks all for the details and info! Test case was completed.