[$500] Android - Note - The page is broken with a long blockcode with one backtick

izarutskaya commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Issue found when validating PR https://github.com/Expensify/App/pull/29055

Version Number: v1.3.81-6

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

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: @

Action Performed:

Open the App
Login with any account
Open a Person to Person report.
Now open Profile Details.
Press on Private Note.
Select My Note
Enter a long blockcode with one backtick ( ) and save it.

Expected Result:

The code block does not go beyond the page (the page is broken)

Actual Result:

The block of code goes outside the page

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Android: Native

![Bug6233522_1697053941825!Screenshot_2023-10-11-17-36-57-29_4f9154176b47c00da84e32064abf1c48](https://github.com/Expensify/App/assets/115492554/d751b423-c88c-46c5-a176-23acdb775cbf) https://github.com/Expensify/App/assets/115492554/b165b8a8-4dd4-48fd-9e62-e27c2cb36d4b

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01412f56d0ed1fc241
Upwork Job ID: 1712207610531987456
Last Price Increase: 2023-10-11
Automatic offers:

dhanashree-sawant | Reporter | 27538114

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

koko57 commented 1 year ago

this one looks very similar to https://github.com/Expensify/App/issues/29408

dylanexpensify commented 1 year ago

Agree @koko57! This was created first, so we'll keep this open (cc @greg-schroeder)

DaniloVlad commented 1 year ago

Proposal

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

Inline Code blocks render beyond the page/parent boundaries in private notes. Inline Code blocks render beyond page boundaries in Reports: https://github.com/Expensify/App/issues/29408#issue-1939046086

What is the root cause of that problem?

There are a few separate issues happening here. To start with the web platform, we observe that when a message is sent in a report that contains an inline code block with tons of trailing spaces, we get this result:

(Image 1)

The same message observed on an android device, gives:

(Image 2)

Now on the other hand, when dealing with Private Notes we observe that the following input:

(Image 3)

Produces the same output on web and android, where the spaces are trimmed:

(Image 4)

I should note there may also be a bug here: You can't have new lines in the code blocks, but maybe not since they are supposed to be "inline?" I used spaces to get the wrapping effect.

However, a very Private note with a ton of characters breaks on android but not web:

(Image 5)

But what we do observe is that once again, in the Private Note, long whitespace sequences were truncated and replaced with a new line.

So depending on what is the actual expected result then the root cause might slightly differ.

Assuming the goal of the product wanted to leave all whitespace the user enters in a inline-codeblock as-is, regardless of if it is a Private Note or a Message in a Report; then these would be the problem areas I found:

1. Incorrect text splitting of Report Messages/Private Notes on native apps:

https://github.com/Expensify/App/blob/91e2120e647aa9ee22171b6dbb772b4aebe64992/src/components/InlineCodeBlock/WrappedText.js#L19-L21

the result of this method on native for our above example is:

[["⁦.", "    ", "a.", "                                                                                                                                                                                                                                                "]]

2. Incorrect formatting of Report Messages on Web

The issue here is that the rendered span tag for inline code blocks is missing CSS styles to achieve the desired outcome. More on this below in the solutions section.

3. Private Notes New line in Inline code blocks bug

This happens due to the library used here: https://github.com/Expensify/App/blob/be44c93037fe165d6e3aec7445044c371bcbc921/src/libs/actions/Report.js#L1168-L1177

Which is called from: https://github.com/Expensify/App/blob/be44c93037fe165d6e3aec7445044c371bcbc921/src/pages/PrivateNotes/PrivateNotesEditPage.js#L107

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

1. Incorrect text splitting of Report Messages/Private Notes on native apps:

Again under the assumption that we want to preserve user entered white space, I think the best approach here would be to replace the existing code here: https://github.com/Expensify/App/blob/bb22e06a1cdd9b87fab7a2c1aa79bb9d9386e3b5/src/components/InlineCodeBlock/WrappedText.js#L41-L70

and instead we could nest a text element within another one:

<Text style={props.boxModelStyles}>
            <Text style={props.textStyles}>{props.children}</Text>
</Text>

Which would give this effect:

NOTE: i styled these manually, that would be improved in a PR

2. Incorrect Formatting of Report Messages on Web

Using white-space: break-spaces; CSS Property:

Alternatively I believe its better to switch from display:inline; to display:inline-block;

Which also fixed private notes on web:

And if we wanted to truly preserve the user white-space as they entered it, both properties should be used, as if i'm not mistake inline-block alone does trim leading spaces on a new line

3. Private Notes Trimming Inline code blocks

I believe this is a separate bug and it would require a lot more investigation to give a concrete answer as to why new lines prevent code blocks from being rendered.

Santhosh-Sellavel commented 1 year ago

Unassigning due to low bandwidth!

Santhosh-Sellavel commented 1 year ago

@dylanexpensify Please assign a new C+ here

b4s36t4 commented 1 year ago

Proposal

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

nline codeblock with white spaces until the end of the first line after text is displayed out of screen

What is the root cause of that problem?

We never had any styling for handling moving the things out of the screen for InlineCodeBlock component. We're just extending the styles we're getting from react-native-html-renderer

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

We need to add whiteSpace property with value break-spaces to let the message break (pre) tag into new line.

As the message ends with all the spaces (is at the end so we should render all the message) we can't go with other values ofwhiteSpace` which may contain some more issues.

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 1 year ago

📣 @rushatgabhane Please request via NewDot manual requests for the Contributor role ($500)

dylanexpensify commented 1 year ago

@rushatgabhane assigning you for takeover, lmk if that's ok!

dhanashree-sawant commented 1 year ago

Hi @dylanexpensify, #29408 was closed as dupe of this issue but that issue was raised on 10th of October on slack and internal team has raised this issue on 12th October. Can you assign me issue reporter for this issue?

melvin-bot[bot] commented 1 year ago

@rushatgabhane, @dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 year ago

@rushatgabhane @dylanexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

@rushatgabhane, @dylanexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 1 year ago

@rushatgabhane, @dylanexpensify 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

dylanexpensify commented 1 year ago

@dhanashree-sawant done!

@rushatgabhane can we get reviews today? 🙇‍♂️

melvin-bot[bot] commented 1 year ago

@rushatgabhane @dylanexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 1 year ago

@rushatgabhane, @dylanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

rushatgabhane commented 1 year ago

hmmmmm

rushatgabhane commented 1 year ago

C+ reviewed 🎀 👀 🎀 https://github.com/Expensify/App/issues/29387#issuecomment-1766404749

I like @DaniloVlad's proposal

melvin-bot[bot] commented 1 year ago

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

dylanexpensify commented 1 year ago

@rlinoz can you confirm the proposal looks good today when you have a moment? Thank youu! 🙇‍♂️

rlinoz commented 1 year ago

Thanks @rushatgabhane , I agree, let's go with @DaniloVlad!

melvin-bot[bot] commented 1 year ago

📣 @rushatgabhane Please request via NewDot manual requests for the Reviewer role ($500)

melvin-bot[bot] commented 1 year ago

📣 @DaniloVlad You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 1 year ago

📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

dylanexpensify commented 1 year ago

moving along!

melvin-bot[bot] commented 1 year ago

@rlinoz @DaniloVlad @rushatgabhane @dylanexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @rushatgabhane is eligible for the Internal assigner, not assigning anyone new.

rlinoz commented 1 year ago

Hey @DaniloVlad just making sure you saw that you were assigned this one.

DaniloVlad commented 12 months ago

Hey guys ! Sorry for the delay, I will jump on upwork now!

DaniloVlad commented 12 months ago

Alright I've submitted my application on upwork, once hired on the platform, I expect to have a PR ready by Monday or Tuesday. @rlinoz Thank you for notifying me

DaniloVlad commented 11 months ago

Hello @rlinoz @rushatgabhane @dylanexpensify , this would be my first contribution, and I have applied on UpWork, but it hasn't hired me for the job so i was just wondering how long this typically takes and/or have i missed anything in the process? (I have submitted all my details on another post previously)

DaniloVlad commented 11 months ago

Oh ok I was following the description on the posting itself "- AFTER your proposal is accepted in Upwork and you have accepted the offer, you may submit the code to implement your solution..."

I was waiting for that confirmation but i will go ahead and finish up the implementation and open a PR then.

rushatgabhane commented 11 months ago

makes sense. but to speed things up you can submit a PR

dylanexpensify commented 11 months ago

+1 on submitting a PR!

DaniloVlad commented 11 months ago

Hey folks, I opened a WIP PR so that you guys know I am working on it. There is an issue. I am currently investigating but it seems i cant get the styles to match properly between the platforms. It wont overflow the page but for example the borders on the inline block wont render on ios...

Im working on a snack to present the proof of concept. Will keep you posted.

rlinoz commented 11 months ago

@DaniloVlad thanks for the update!

Would you mind converting it to a draft so we know when we can look at it?

DaniloVlad commented 11 months ago

Hey guys, so unfortunately switching over the design to be a code-block seems to be more difficult than expected. Especially when it comes to mobile.

I would like some input on the styling of these blocks. It seems previously there was a lot more padding and I'm curious if that is still preferred.

Here is how it looks: Web:

IOS:

DaniloVlad commented 11 months ago

There are still some things I'd want to try out to support the "block" code style but i worry it may result in a lot of regressions. If anyone can point me to some information regarding where the element is used outside of in comments that could be helpful.

Expensify / App