Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hold][$1000] Copy to clipboard for code block message adds extra space below #15195

Closed
1 task
kavimuru opened this issue Feb 16, 2023 · 15 comments
Closed
1 task
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Feb 16, 2023

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 the app on android
  2. Open any report
  3. Send code block (eg: Test)
  4. Long press and copy to clipboard
  5. Send the copied message again

Expected Result:

Message should display same as copied

Actual Result:

Extra space below the text is added inside code block and it is only visible on android app, it works fine on android chrome

Workaround:

unknown

Platforms:

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

  • Android / native

Version Number: 1.2.72-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:

android.app.code.block.issue.mp4
az_recorder_20230215_212936.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1676459478735969

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017d6f7a1cf15fa6ae
  • Upwork Job ID: 1626697741398188032
  • Last Price Increase: 2023-02-17
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 16, 2023
@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 16, 2023
@MelvinBot
Copy link

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

@isabelastisser
Copy link
Contributor

I don't have an android, so I can't reproduce this. I was unable to reproduce this on my iPhone tho. I will post to bug-zero for help!

@isabelastisser
Copy link
Contributor

isabelastisser commented Feb 17, 2023

SO I was able to reproduce it on android.

image

@isabelastisser isabelastisser added the External Added to denote the issue can be worked on by a contributor label Feb 17, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 17, 2023
@melvin-bot melvin-bot bot changed the title Copy to clipboard for code block message adds extra space below [$1000] Copy to clipboard for code block message adds extra space below Feb 17, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 17, 2023
@MelvinBot
Copy link

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

@tienifr
Copy link
Contributor

tienifr commented Feb 18, 2023

Proposal

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

Extra space below the text is added inside code block only on android/ios app, it works fine on web

What is the root cause of that problem?

This is due to the <br> tag is rendered differently on web vs native.

When we copy the code block, the following markdown will be copied:
Screenshot 2023-02-18 at 16 54 06

When we send that message, this is the HTML that's sent
<pre>codeblock<br /></pre>

On Web, that <br /> inside the pre will be collapsed and you'll not see a new line. However on native, the <br /> will show in the new line since it's the default behavior of react-native-render-html. This causes the mentioned extra space below the text only on native platforms.

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

In order to fix this, we need to enable the enableExperimentalBRCollapsing flag of react-native-render-html on native platform so that the behavior of the native platform will follow the correct HTML standard.

The documentation of that flag can be found here.

The flag should be added to this component

Please note we should only enable this on native platform, this is also mentioned in the documentation

Recommended value is true on non-web platforms

What alternative solutions did you explore? (Optional)

NA

Result

Working well after the fix:

Screen.Recording.2023-02-18.at.17.43.11.mov

@eh2077
Copy link
Contributor

eh2077 commented Feb 19, 2023

Related to #14822

@melvin-bot melvin-bot bot added the Overdue label Feb 20, 2023
@JmillsExpensify
Copy link

So perhaps we put this issue on hold since that PR predates this report and it was already known. If we can't reproduce the issue after that PR hits staging, we can close this one.

@bondydaa
Copy link
Contributor

I believe this should be fixed now that #14822 was just merged.

It looks like this PR #15241 published the expensify-common update before #14822 was done which meant the updates to the markdown parsing code went out without the changes @marktoman added to address the extra <br> tags on native.

There wasn't anything wrong done in #15241 it's that it required changes from Expensify-Common as well and since @marktoman's changes in Expensify-Common had been merged before Expensify/expensify-common#502 was merged when the hash was updated it just also included the changes from Expensify/expensify-common#496.

As for why the extra <br> tags were included it's explained here: #14822 (comment).

@kavimuru once #14822 has been deployed can you re-test and confirm for us that this isn't happening anymore?

@bondydaa bondydaa removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 20, 2023
@bondydaa bondydaa changed the title [$1000] Copy to clipboard for code block message adds extra space below [Hold][$1000] Copy to clipboard for code block message adds extra space below Feb 20, 2023
@isabelastisser
Copy link
Contributor

@kavimuru , can you please follow up on this? Thanks!

@kavimuru once #14822 has been deployed can you re-test and confirm for us that this isn't happening anymore?

@melvin-bot melvin-bot bot removed the Overdue label Feb 21, 2023
@kavimuru
Copy link
Author

@isabelastisser Not able to reproduce.

@melvin-bot melvin-bot bot added the Overdue label Feb 23, 2023
@isabelastisser
Copy link
Contributor

So perhaps we put this issue on hold since that PR predates this report and it was already known. If we can't reproduce the issue after that PR hits staging, we can close this one.

Closing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants