[HOLD for payment 2024-04-15] [$500] iOS - Chat - Strikethrough not applied for multiline hyperlinks deleted offline

[lanitochka17]

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: 1.4.55-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4441552
Issue reported by: Applause - Internal Team

Action Performed:

1. Log in to New Expensify
2. Navigate to any chat
3. Send multiline hyperlink
4. Disable the internet connection
5. Delete the message

Expected Result:

The message should be grayed out with strikethrough formatting applied to indicate that the deletion is pending

Actual Result:

The message is grayed out but the strikethrough is not applied

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6420587_1710946305742.SUGF1463.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ef21d15df0bfd5a6
• Upwork Job ID: 1770552279000301568
• Last Price Increase: 2024-03-20
• Automatic offers:
• mollfpr | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

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

[lanitochka17]

@puneetlath 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

[lanitochka17]

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

[bernhardoj]

Proposal

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

The strikethrough/deleted styles aren't applied to multi-line links when deleting it while offline.

What is the root cause of that problem?

When we have a single line link, the anchor renderer will have 1 text as the child that contains the link text. It gets the style from the parent.

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.tsx

Lines 66 to 68 in 99169e1

	<Text
	ref={linkRef}
	style={StyleSheet.flatten([style, defaultTextStyle])}

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

Line 68 in 99169e1

style={[style, parentStyle, styles.textUnderlinePositionUnder, styles.textDecorationSkipInkNone]}

There are 2 styles that affect it, style and parentStyle. style contains the anchor tag style, and parentStyle contains the strikethrough style and the important style is textDecorationLine. style textDecorationLine value is underline, while parentStyle textDecorationLine is line-through and it overwrites the style one.

But for multi-line links, there are multiple children (MemoizedTNodeRenderer) of the component. If we have a multi-line link like this:

[multi
line](google.com)

then the children will be 3: multi, br, line.

The parent text contains the strikethrough style that I explained above, but each child here also contains the anchor tag style. Based on this upstream issue, it's not possible to have both line-through and underline styles, that's why for multi-line links, we see the underline styles.

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

We can render the children for a multi-line link by ourselves by using renderChildren props of TNodeChildrenRenderer.

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

Line 73 in 99169e1

<TNodeChildrenRenderer tnode={tnode} />

and to have both underline and strikethrough, we can use textDecorationLine: 'underline line-through' style.

const hasStrikethroughStyle = 'textDecorationLine' in parentStyle && parentStyle.textDecorationLine === 'line-through';
const textDecorationLineStyle = hasStrikethroughStyle ? {textDecorationLine: 'underline line-through'} : {};

return (
    <AnchorForCommentsOnly ...
        // add textDecorationLineStyle here
        style={[style, parentStyle, textDecorationLineStyle, styles.textUnderlinePositionUnder, styles.textDecorationSkipInkNone]}
    >
        <TNodeChildrenRenderer
            tnode={tnode}
            // renderChild is only called for multi-line link
            renderChild={(props) => {
                if (props.childTnode.tagName === 'br') {
                    return <Text>{'\n'}</Text>
                }
                if (props.childTnode.type === 'text') {
                    // and here
                    return <Text style={[props.childTnode.getNativeStyles(), parentStyle, textDecorationLineStyle, styles.textUnderlinePositionUnder, styles.textDecorationSkipInkNone]}>{props.childTnode.data}</Text>
                }
                return props.childElement;
            }}
        />
    </AnchorForCommentsOnly>
);

we can further improve this if there is any missing style

[melvin-bot]

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

[melvin-bot]

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

[mollfpr]

@bernhardoj The result on Android is missing the underline, is this expected?

Prev

After

[bernhardoj]

Oh, you're right, that's because the parentStyle overwrites the underline style. I think we should use renderChildren for iOS only. What do you think?

AnchorRenderer
|- index.tsx
|- childRenderer
|--- index.ts (will just return undefined)
|--- index.ios.ts

[mollfpr]

That might be work, but I think we need to fix the underline too on iOS.

[bernhardoj]

As mentioned in my proposal, it's an upstream issue for iOS, that's why I propose it to customize the children renderer so we prioritize the strikethrough style. It also happens for a single line without my solution.

[bernhardoj]

There is an option to use both though based on React native docs.
https://www.github.com/meliorence/react-native-render-html/issues/529

I will play around and see whether it works for react-native-render-html.

[bernhardoj]

Okay, it works, I have updated my proposal to include it for both single and multi-line.

so far I tested and it works for both web and native

[mollfpr]

The proposal from @bernhardoj looks good to me!

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[puneetlath]

Sounds good to me.

[melvin-bot]

📣 @mollfpr 🎉 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 📖

[bernhardoj]

PR is ready

cc: @mollfpr

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

• Fix anchor text can't show both underline (link style) and line through (delete style while offline) on iOS #38985

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

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

• @mollfpr requires payment automatic offer (Reviewer)
• @bernhardoj 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:

• [@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.
• [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[puneetlath]

@mollfpr bump on the checklist. And on accepting the Upwork offer.

@bernhardoj has been paid.

[mollfpr]

@puneetlath I'll do manual request in NewDot, could you create the payment summary? Thank you!

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

No offending PR.

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

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

1. Send a single and multi-line link to any chat
2. Verify the link has underline
3. Go offline
4. Delete both links
5. Verify the link has underline and line through
6. 👍 or 👎

[puneetlath]

Regression test issue here: https://github.com/Expensify/Expensify/issues/388565

Payment summary:

• C - $500 - @bernhardoj - paid on Upwork
• C+ - $500 - @mollfpr - to be paid on NewDot

Thanks everyone!

[JmillsExpensify]

$500 approved for @mollfpr