[HOLD for payment 2024-07-29][$250] Chat - Live markdown preview for Heading is not shown in the composer if space exist before

kbecciv commented 3 months 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!

Version Number: 1.4.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: https://expensify.testrail.io/index.php?/tests/view/4547651 Issue reported by: Applause - Internal Team

Action Performed:

Open ND Expensify
Navigate to any Chat
Enter a space and a header markdown text for example " space # Header "
Verify the preview
Send the text
Verify the message is shown as heading markdown

Expected Result:

Live markdown preview for Heading (inside the composer) is shown if space is used before

Actual Result:

If space is used before, Live Markdown preview for heading (inside the composer) is not shown. But it is correctly shown after the message is sent. Note: -It is ok that if a character (except space) is used before, live markdown is not shown because the heading is not applied even after the message is sent. -The same issue occurs also for Quotes markdown >

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/921be3b0-bc4a-42f2-b784-970ca2438913

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e69db88132b2d1b4
Upwork Job ID: 1788634876612800512
Last Price Increase: 2024-05-30
Automatic offers:

badeggg | Contributor | 102592680

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

kbecciv commented 3 months ago

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

kbecciv commented 3 months ago

@sonialiap 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.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

sonialiap commented 3 months ago

@Expensify/design I can reproduce, but do we think this is a bug we want to fix? The end result in both scenarios is that the heading formatting applies, it just doesn't show up in the preview if there's a space leading the hashtag

https://github.com/Expensify/App/assets/17131195/81b17886-4e58-4cc5-b934-c67554ba1bba

dannymcclain commented 3 months ago

Happy for others to weigh in too, but I feel like if it ends up as a header when you send it, we should reflect that in the composer.

sonialiap commented 3 months ago

Sounds good to me

melvin-bot[bot] commented 3 months ago

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

dubielzyk-expensify commented 3 months ago

I believe we feel pretty strongly about making sure the editor at all times show what you send, so I wouldn't call it a huge priority, but I would say we should fix it.

Afrin127329 commented 3 months ago

Normal users(not developers) won't understand this markdown and they can get confused easily. So I would also suggest the fix.

melvin-bot[bot] commented 3 months ago

📣 @Afrin127329! 📣 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:

Make sure you've read and understood the contributing guidelines.
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.
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.
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>
```

Afrin127329 commented 3 months ago

Contributor details Your Expensify account email: afrinnahar1999@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0198fbe075b3e822ae

melvin-bot[bot] commented 3 months ago

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

dragnoir commented 3 months ago

Not an issue, the " # Header" when sent to be saved, the space at the beginning will be removed.

So the "# Header" and the " # Header" with space are both saved similarly "# Header" and are displayed as heading.

There's no problem with the composer.

melvin-bot[bot] commented 3 months ago

@sonialiap, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

sonialiap commented 3 months ago

@dragnoir I agree that this is not really a bug, but we have two design team members saying they'd like to have the composer accurately show what will be sent. Do you think it is not possible to have # header show # **header** as it would without the leading space?

dragnoir commented 3 months ago

@sonialiap We can make changes on react-native-live-markdown to reflect the required behavior.

melvin-bot[bot] commented 3 months ago

@sonialiap, @allroundexperts Eep! 4 days overdue now. Issues have feelings too...

sonialiap commented 3 months ago

@dragnoir that would be great! Would you like to make a proposal?

dragnoir commented 3 months ago

OK, I will see if I can figure it out, 1st time with the live markdown 😅

badeggg commented 3 months ago

Proposal

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

heading1 and quote does not show preview in composer box when prefix with one or more spaces.

What is the root cause of that problem?

In composer box, all input chars of a comment are reserved, while when user hit 'enter', new comment is trimmed, and the trimmed comment will be submitted. This cause for a single comment, composer box are parsing a different string from what showing comment place are parsing. For heading1 and quote syntax, leading white spaces in a line makes a difference. Check expensify-common ExpensiMark lib: heading1 here and quote here.

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

First of all, let's talk about what we want the preview looks like. 'heading1' is sure:

There are two quote rendering results that may be what we want, I prefer the first one. quote rendering result 1:

quote rendering result 2:

[!IMPORTANT] All following changes assume we want quote rendering result 1. I have to provide my test branch, we have to make too many changes. Please check diff to view better changing.

1. We need let composer box parse trimmed comment string too

Change line here to

    const trimmedMarkdown = markdown.trim();
    const html = parseMarkdownToHTML(trimmedMarkdown);

to parse trimmed comment string.

While we trimming comment string to parse, the parse result(ranges) will have a shifted range start value. We need recover it. And we need calculate how many leading white spaces leadingWhiteSpacesCountInFirstRangeLine are trimmed and store the value in the parse result. Which will be used in iOS rendering quote syntax.

So add method recoverPositionEffectedByTrim to here, which looks like:

function recoverPositionEffectedByTrim(ranges: Range[], markdown: string): Range[] {
  const leadingWhiteSpacesAndLineTerminators = markdown.match(/^\s*/g)?.[0] || '';
  return ranges.map((range, inx) => {
    const start = range.start + leadingWhiteSpacesAndLineTerminators.length;
    let leadingWhiteSpacesCountInFirstRangeLine: number | undefined;
    if (inx === 0) {
      let tryInx = start - 1;
      while (tryInx >= 0) {
        if (markdown[tryInx] === '\n' || markdown[tryInx] === '\r' || markdown[tryInx] === '\u2028' || markdown[tryInx] === '\u2029') {
          break;
        } else {
          tryInx -= 1;
        }
      }
      leadingWhiteSpacesCountInFirstRangeLine = start - tryInx - 1;
    }
    return {
      ...range,
      start,
      leadingWhiteSpacesCountInFirstRangeLine,
    };
  });
}

change line here to

    const recoveredRanges = recoverPositionEffectedByTrim(groupedRanges, markdown);
    return recoveredRanges;

to take care of the result ranges.

2. Rending

Besides parsing trimmed comment string, we also need make some changes in rendering.

web: Nothing to do, rendering goes well
android: Rendering goes well for heading1, but does not goes well for block quote, after we change parsing. Currently, we are rendering block quote by set a leading margin to quote line and draw filled rectangle(s) in the leading margin space, check here. Which is no longer ok for quote rendering result 1. Rather than implement LeadingMarginSpan, we should use ReplacementSpan to let first > have a left-margin and draw filled rectangle in the left-margin. Check this file and this file in my test branch. Please check the diff to view better changing.
ios: Rendering goes well for heading1, but does not goes well for block quote, after we change parsing. Currently, we are rendering block quote by set paragraph indent to quote line code here and draw filled rectangle in the indented space code here. Which is no longer ok for quote rendering result 1. If there is no leadingWhiteSpacesCountInFirstRangeLine, we keep the original code that set paragraph indent to quote line to acquire room for filled rectangle drawing. If there is leadingWhiteSpacesCountInFirstRangeLine, we set kerning to the space before >, and draw filled rectangle in the kerning room. Check this file and code here in my test branch. Please check the diff to view better changing.

Video

https://github.com/Expensify/App/assets/10908996/fe75dae4-cb66-4597-ba70-b46fd3102651

What alternative solutions did you explore? (Optional)

In my primary solution to implement quote rendering result 1, there is a inconsistency between iOS and web/android.(I do think it's negligible): On android/web, we make a left-margin to glyph > to acquire room for filled rectangle drawing, while on iOS, we make a right-margin to the char before > to acquire room for filled rectangle drawing. I can not find a way to also make a left-margin to specific glyph in iOS. This will cause cursor in different place when it's between > and the char before >:

If team think this stain really bothers, we can chose quote rendering result 2. We will have to change iOS/android/web quote rendering. I have not checked too much detail, but I think it's doable.

melvin-bot[bot] commented 3 months ago

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

allroundexperts commented 3 months ago

Still waiting for proposals here.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

@sonialiap @allroundexperts 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!

allroundexperts commented 3 months ago

Not overdue. We are still waiting for proposals here.

badeggg commented 3 months ago

I have updated my proposal. By the way, fixing this issue require not only considering 'heading1' syntax, but also considering quote syntax, and have to change quote rendering in iOS and android. That's too much. So maybe more bounty? Just saying...

melvin-bot[bot] commented 3 months ago

@sonialiap, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

sonialiap commented 3 months ago

@allroundexperts what do you think of badeggg's update? https://github.com/Expensify/App/issues/41908#issuecomment-2114919703

melvin-bot[bot] commented 3 months ago

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

sonialiap commented 3 months ago

@allroundexperts what do you think of badeggg's update? https://github.com/Expensify/App/issues/41908#issuecomment-2114919703

allroundexperts commented 3 months ago

@badeggg Can't we just update the regex inside the parseMarkdownToHTML function to ignore the whitespace? That seems much simpler option. What do you think?

badeggg commented 3 months ago

That's my first think, and is actually my previous proposal. The test branch is here

The problem is that in react-native-live-markdown we will get ranges with shifted start value too, like we simply trim the leading spaces of the comment string. Related code here. We still need to recover the shifted start value in ranges, which is more complicated compared to simply trim the leading spaces of a comment string. Since we will need do the same change to parser/index.ts like change to parser/index.ts in my current proposal, plus we adjusted the regex in lib/ExpensiMark.js.

After we recovering shifted start value, you'll realize we still need considering rendering problem in iOS and android, which will be the same concern and same code changes in my current proposal.

allroundexperts commented 3 months ago

@badeggg I get that but, In here, you can use a capture group to capture the spaces and add it in the replacement. That way, we won't have to store the ranges which sounds like a much more complex solution.

badeggg commented 3 months ago

Hi, in code here from react-native-live-markdown, we have to make sure ranges must describe the original comment string correctly, to render correctly. html text and ranges must have identical meaning.

If we change here to

            {
                name: 'heading1',
                process: (textToProcess, replacement, shouldKeepRawInput = false) => {
                    const regexp = shouldKeepRawInput ? /^( *)# ( *(?! )(?:(?!<pre>|\n|\r\n).)+)/gm : /^( *)# +(?! )((?:(?!<pre>|\n|\r\n).)+)/gm;
                    return textToProcess.replace(regexp, replacement);
                },
                replacement: '<h1>$1$2</h1>',
            },

as you suggest, for example for input # dddd(4 spaces before #), ranges will be

[
    {
        "type": "syntax",
        "start": 0,
        "length": 2
    },
    {
        "type": "h1",
        "start": 2,
        "length": 8
    }
]

which is not correct.

Notice the rendering is not correct in the screen shot, # should not be bold.

allroundexperts commented 3 months ago

Hm... Why are ranges incorrect? What should be the correct range?

badeggg commented 3 months ago

For specific input # dddd(4 spaces before #), the correct ranges should be:

[
    {
        "type": "syntax",
        "start": 4,
        "length": 2
    },
    {
        "type": "h1",
        "start": 6,
        "length": 4
    }
]

allroundexperts commented 3 months ago

Why are the ranges not correct? I'm not able to understand that. We're not trimming the comment, and yet, still the ranges are incorrect?

badeggg commented 3 months ago

By word correct, we mean the rendering system should be able to render as we expect based on ranges object.

The meaning of an example ranges object:

[
    {  // this indicate the syntax sub-string, which should be not applied any specific style when render
        "type": "syntax",
        "start": 4,
        "length": 2
    },
    {  // this indicate the content of a syntax, which should be applied proper style of the syntax, e.g. for heading1, that's bold style
        "type": "h1",
        "start": 6,
        "length": 4
    }
]

I gently suggest you check the rendering logic in react-native-live-markdown, https://github.com/Expensify/react-native-live-markdown, which is what we use for composer box to 'live markdown'

badeggg commented 3 months ago

cc @sonialiap

allroundexperts commented 3 months ago

Thanks for your explanation @badeggg. I've dig into it more and I agree with your proposal. Just ignoring the whitespace in the regex seems like a straightforward solution, but unfortunately, it doesn't work.

@badeggg's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

PS: Regarding:

That's too much. So maybe more bounty? Just saying...

I think the assigned internal engineer can consider your request. Although, I would agree that the solution is not so straight forward.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

badeggg commented 3 months ago

@luacmartins @allroundexperts Hi, how do you think about the flaw that I mentioned in the last section of my proposal, I do think it's negligible:

to implement quote rendering result 1, there is a inconsistency between iOS and web/android.(I do think it's negligible): On android/web, we make a left-margin to glyph > to acquire room for filled rectangle drawing, while on iOS, we make a right-margin to the char before > to acquire room for filled rectangle drawing. I can not find a way to also make a left-margin to specific glyph in iOS. This will cause cursor in different place when it's between > and the char before >

Please check related screen shots in the original proposal.

badeggg commented 3 months ago

@luacmartins And can you consider my concern here about the compensation.

luacmartins commented 3 months ago

@badeggg I think the compensation for this issue is still fair at $250.

melvin-bot[bot] commented 2 months ago

@badeggg @sonialiap @luacmartins @allroundexperts this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

badeggg commented 2 months ago

will make a pr in ~24 hrs

badeggg commented 2 months ago

I have made pr to react-native-live-markdown We will need update react-native-live-markdown version in package.json of expensify-app after it's published.

Expensify / App