[HOLD for payment 2023-03-13] [$1000] No spacing/margins above and below a Codeblock

[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. Open a chat
2. Send one message - a normal word/sentence, then space and send a codeblock. Then space and send another normal word/sentence.

Expected Result:

There should be space between the normal messages and codeblock.

Actual Result:

There's no spacing around the codeblock.

However, we want an 8 px margin above and below a codeblock.

Workaround:

unknown

Platforms:

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

• [ x] Android / native
• [ x] Android / Chrome
• [x ] iOS / native
• [x ] iOS / Safari
• [x ] MacOS / Chrome / Safari
• [ x] MacOS / Desktop

Version Number: 1.2.74-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:
Issue reported by: @cead22
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1676670453605579

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018d44cb79f9fd71df
• Upwork Job ID: 1630342851487846400
• Last Price Increase: 2023-02-27

[MelvinBot]

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

[jliexpensify]

Posted a comment here - https://expensify.slack.com/archives/C049HHMV9SM/p1676957142442749?thread_ts=1676670453.605579&cid=C049HHMV9SM

[jliexpensify]

Slack comment here from Bondy saying it's not related to his PR. Back to the drawing board!

[jliexpensify]

Alright, I've identified a previous issue and PR (Slack comment). Asking about the next steps in-thread.

[jliexpensify]

Still digging in - https://expensify.slack.com/archives/C049HHMV9SM/p1677452918315599?thread_ts=1676670453.605579&cid=C049HHMV9SM

[jliexpensify]

Alright, this is not a regression. Moving to External!

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[daraksha-dk]

Proposal

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

No spacing/margins above and below a Codeblock

What is the root cause of that problem?

We aren't not defining any margin for the codeblock message which is resulting in this problem.

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

We can fix this by setting marginTop & marginBottom to some value like 8px here

What alternative solutions did you explore? (Optional)

[MelvinBot]

📣 @daraksha-dk! 📣

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>

[allroundexperts]

Proposal

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

Missing spacing above and below the code block.

What is the root cause of that problem?

The root cause for this is missing margins for the code block.

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

We have to carefully add the margins in order to preserve the tab behaviour. Adding it here will sort of break the tab selection (screen shot attached).

We should add the margins here instead. This will preserve the default tab behaviour that we have right now.

Works well after the fix.

What alternative solutions did you explore? (Optional)

None

[daraksha-dk]

We have to carefully add the margins in order to preserve the tab behaviour. Adding it here will sort of break the tab selection

I believe displaying outlines for messages is itself a bug, and it's not intentional to show them only for codeblocks within a message. There is no reason to use an outline here as those items are not actionable/interactable. However, since this is ultimately an accessibility issue, we are currently putting it on hold until we can address it properly.

For reference:

Production (NewDot)

tab-bug.mp4

Slack's treatment

slack-tab.mp4

[MelvinBot]

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

[allroundexperts]

We have to carefully add the margins in order to preserve the tab behaviour. Adding it here will sort of break the tab selection

I believe displaying outlines for messages is itself a bug, and it's not intentional to show them only for codeblocks within a message. There is no reason to use an outline here as those items are not actionable/interactable. However, since this is ultimately an accessibility issue, we are currently putting it on hold until we can address it properly.

For reference:

Production (NewDot)

Slack's treatment

Thank you for your deep insight @daraksha-dk!

I think this is needed because on mobile, we're making code blocks pressable.

[mountiny]

Thanks @eVoloshchak for the review

thanks also to @daraksha-dk for the proposal but going with @allroundexperts feel free to raise a PR! Thanks!

[allroundexperts]

PR created.
#15570

[jliexpensify]

Hired @eVoloshchak and @allroundexperts

[mountiny]

@jliexpensify merged, this should be available for the bonus if no regression

[MelvinBot]

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

[MelvinBot]

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

• fix: added vertical margin to the code block #15570

If no regressions arise, payment will be issued on 2023-03-13. 🎊

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

[MelvinBot]

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:

• [@eVoloshchak / @mountiny] The PR that introduced the bug has been identified. Link to the PR: Judjing by [HOLD #13807][$1000] No line-height provided to normal text when written in combination with code block (3 backtick block) #14261 (comment) comment this was fixed by Set default lineHeight for webViewStyles(HTML rendered messages) #14360. I've tested that PR, but it still isn't fixed.
  I think that this [HOLD #13807][$1000] No line-height provided to normal text when written in combination with code block (3 backtick block) #14261 (comment) contains wrong testing steps. The issue appears if you have a single message that contains both plain text and code. I might be wrong, but on the image code blocks and plain text are on separate messages.
  I've also tested a couple of old commits that change BasePreRenderer, the issue still persisted, so it seems like that's how it was implemented initially.
• [@eVoloshchak / @mountiny] 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:
• [@eVoloshchak / @mountiny] 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:
• [@allroundexperts] Propose regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/269850

[allroundexperts]

Regression Test Proposal

1. Go to https://staging.new.expensify.com/ and open any chat.
2. Enter any comment such that the first line is plain text, second line is a code block and third line is plain text again.
3. Verify that equal amount of margin appears between the lines.
   

Do we agree 👍 or 👎

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR:
  Judjing by this comment this was fixed by Set default lineHeight for webViewStyles(HTML rendered messages) #14360. I've tested that PR, but it still isn't fixed.
  I think that this image contains wrong testing steps. The issue appears if you have a single message that contains both plain text and code. I might be wrong, but on the image code blocks and plain text are on separate messages.
  I've also tested a couple of old commits that change BasePreRenderer, the issue still persisted, so it seems like that's how it was implemented initially.

• 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 is a visual bug that could be caught during review, I don't think there are additional checklist items we should add

[jliexpensify]

Sorry, I was at a conference the last 3 days - will look at payments now!

[jliexpensify]

Paid @allroundexperts and @eVoloshchak

[jliexpensify]

I'll get @mountiny to cast his eyes on #15299 (comment) and I'll whip up that GH

[mountiny]

@jliexpensify I think that looks good, but I also think we should include a screenshot to the steps if we can to make sure its easier for the tester to confirm it looks correct

[jliexpensify]

@allroundexperts - could you take a look at Vit's feedback and incorporate a screenshot into your Proposal? I'll get that GH sorted after that.

[allroundexperts]

@jliexpensify I'm not sure if I am understanding this correctly. Can you please explain what sort of screenshot you're looking for? TIA!
I see that my proposal does contain a screenshot of how it looks after the fix.

[mountiny]

@allroundexperts sorry, I cannot see any screenshot in the comment

Basically any screenshot of how the spacing should look like correctly now would be good.

[allroundexperts]

@mountiny I thought you wanted that in my original issue Proposal 😅 . I've added it to my regression test proposal as well 🙏

[mountiny]

Ahahah, nono, int he regression steps proposal, thank you!

I think this is good to go now @jliexpensify