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

[$250] Tasks - Pressing enter key after a mark down doesn't show the new line #46389

Closed
1 of 6 tasks
lanitochka17 opened this issue Jul 28, 2024 · 32 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 28, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.13-3
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Click on the Green plus Icon
  3. Click on Assign task
  4. Insert a sample title
  5. Click on Description
  6. Insert multiple-line text until a scroll bar is shown on the right
  7. On a new line insert a markdown text (eg. bold)
  8. Hit the enter key on the keyboard
  9. For comparison on a new line insert a normal text (e.g text)
  10. Hit the enter key on the keyboard

Expected Result:

When hitting the enter key after a mark down the new line becomes visible with the cursor seen visibly

Actual Result:

When hitting the enter key after a mark down the cursor gets hidden on the new line which is a different behavior when compared with the behavior seen on a normal text and hitting enter key

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

Bug6550710_1721751760580.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019aa27e395e6f5aae
  • Upwork Job ID: 1819367536186479442
  • Last Price Increase: 2024-08-16
  • Automatic offers:
    • dominictb | Contributor | 103558387
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 28, 2024
Copy link

melvin-bot bot commented Jul 28, 2024

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

@lanitochka17
Copy link
Author

@johncschuster 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
Copy link
Author

We think that this bug might be related to #Live Markdown

@dominictb
Copy link
Contributor

Proposal

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

When hitting the enter key after a complex markdown element (bold, italic, quote, hashtag,...) the input doesn't scroll down to the new line at the bottom, unlike when hitting entering key after a plain text in the input.

What is the root cause of that problem?

When we hit the Enter key or change the text, the cursor will be moved around and this function will scroll the cursor into view. scrollCursorIntoView function above is called in here and here

Considering 2 following scenarios:

Hitting Enter after plain text

image

In this case the whole text node would look like 1\n\n and because 1 is a plain text, i.e there's no HTML tag like span, div wrapping around the text content, the last text node in the HTML content is \n (the final newline character), and the caret will be placed before that. We can easily verify by adding console.log(selection.getRangeAt(0)) here

image

Hitting enter key after a complex markdown element, e.g: bold, italic, quote, hashtag,...

image

Same reasoning as the first scenario, but now since the previous node is an element node, somehow the browser will join the newline after that to the last text node, and place the caret between 2 newlines. Simply put, the HTML value would look like

<span>#something</span>\n<text-caret-here>\n

We can verify by adding console.log(selection.getRangeAt(0)) here, and we can easily see that the offset of the selection range is now 1, not 0

image

So, because the selection range which represents caret is different in 2 scenarios, hence selection.getRangeAt(0).getClientRects here will return 2 different DOMRectList

  • In the first scenario, the DOMRectList contains 1 DOMRect because the last text node only contains 1 single \n character
image
  • In the second scenario, the DOMRectList contains 2 DOMRects because the last text node contains 2 \n characters (and the caret is placed between them)
image

And we only use the first DOMRect to calculate the scroll offset, but in the second scenario, the DOMRect is still positioned in the previous line, not the latest new line hence this condition holds true, and there's no scrolling

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

In case the DOMRect list of selection.getRange(0).getClientRects() returns more than 1 value, we should select the last DOMRect, or the DOMRect that maximizes the chance of this condition being false.

const caretRects = selection.getRangeAt(0).getClientRects();
 const caretRect = caretRects[caretRects.length-1] // or we can select the `DOMRect` with max `bottom` value. During testing this seems enough to make this work.

What alternative solutions did you explore? (Optional)

@johncschuster
Copy link
Contributor

This will either fit under #live-markdown or #tasks. I think it's live markdown related.

@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2024
@BartoszGrajdek
Copy link
Contributor

Hi, I'm Bartosz from SWM react-native-live-markdown team! This issue is in fact live markdown related, thanks for adding it to the project 🙌🏻

I'm going to be OOO till 11.08, so I'm tagging @tomekzaw here for visibility.

@BartoszGrajdek BartoszGrajdek moved this to MEDIUM in Live Markdown Aug 2, 2024
@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Aug 2, 2024
@melvin-bot melvin-bot bot changed the title Tasks - Pressing enter key after a mark down doesn't show the new line [$250] Tasks - Pressing enter key after a mark down doesn't show the new line Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

Job added to Upwork: https://www.upwork.com/jobs/~019aa27e395e6f5aae

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

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

Copy link

melvin-bot bot commented Aug 5, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
@johncschuster
Copy link
Contributor

Not overdue. We're waiting on @BartoszGrajdek to come back from OOO.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 5, 2024
@johncschuster
Copy link
Contributor

Bumping to keep Melvin happy

@melvin-bot melvin-bot bot removed the Overdue label Aug 8, 2024
Copy link

melvin-bot bot commented Aug 9, 2024

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

Copy link

melvin-bot bot commented Aug 11, 2024

@johncschuster @Pujan92 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 melvin-bot bot added the Overdue label Aug 11, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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

@johncschuster
Copy link
Contributor

Bumping this one. Were you able to pick this back up, @BartoszGrajdek?

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2024
@dominictb
Copy link
Contributor

Hmm for live markdown related issue, shall we wait for the live markdown team to review the proposal, or C+ can also review?

@BartoszGrajdek
Copy link
Contributor

BartoszGrajdek commented Aug 14, 2024

Bumping this one. Were you able to pick this back up, @BartoszGrajdek?

Thanks for the bump @johncschuster ! We had quite a few high-priority issues this week. 😅 @dominictb's proposal makes sense to me, so if you agree, we can assign him to this issue. He can then raise a PR to react-native-live-markdown, and we can test it there. 🙌🏻

Unfortunately, our team at SWM will be OOO on Thursday and Friday due to a public holiday (sorry about that again 🙏🏻).

In the meantime, you or any C+ member can review the PR in our repo. Just please hold off on merging it, as we’d like to test it against some of our known regressions once we're back to ensure we don't break anything. 👀

Copy link

melvin-bot bot commented Aug 16, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 16, 2024
@johncschuster
Copy link
Contributor

Sounds great, @BartoszGrajdek! Thanks for that update!

@dominictb, I've assigned the issue to you. Please review @BartoszGrajdek's comment above

@dominictb
Copy link
Contributor

PR Expensify/react-native-live-markdown#454 is ready.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 21, 2024
@dominictb
Copy link
Contributor

@Pujan92 @johncschuster @BartoszGrajdek app PR #47763 is ready for review.

@dominictb
Copy link
Contributor

@BartoszGrajdek @Skalakid it seems like in the latest main, I couldn't put a new line by pressing Enter or Shift+Enter, so I couldn't test further

Screen.Recording.2024-08-28.at.15.09.06.mov

@dominictb
Copy link
Contributor

@johncschuster I have closed the app PR since we have another PR that bump to markdown lib to 0.1.117 (#47763), however we'll still need to test on latest main to see if this issue is fixed

cc @Pujan92

@dominictb
Copy link
Contributor

@johncschuster @Pujan92 this is fixed on latest main, and since we had the production deployment recently, can we confirm that this is ready for payment?

@johncschuster
Copy link
Contributor

johncschuster commented Sep 5, 2024

Payment Summary:

Contributor: @dominictb paid $250 via Upwork - PAID! 🎉
Contributor+: @BartoszGrajdek does not require payment

@johncschuster
Copy link
Contributor

Does this require regression test steps? If so, can you please post them?

@Pujan92
Copy link
Contributor

Pujan92 commented Sep 5, 2024

@johncschuster I haven't reviewed the proposal or markdown PR for this issue so no payment for me. All done by @BartoszGrajdek. Thanks to them!

@johncschuster
Copy link
Contributor

Got it. Thanks, @Pujan92! @BartoszGrajdek / @dominictb do you feel we need some regression steps for this one?

@johncschuster
Copy link
Contributor

Bumped in Slack for regression test steps

@dominictb
Copy link
Contributor

Regression test steps:

  1. Go to NewDot app and create a new task using QAB
  2. Click on Description
  3. Insert multiple-line text until a scroll bar is shown on the right
  4. On a new line insert a markdown-format text, e.g: bold, italic, #room-mention,...
  5. Hit the enter key on the keyboard

Verify that: When hitting the enter key, the new line becomes visible with the cursor seen visibly.

@johncschuster
Copy link
Contributor

Thank you!

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Done
Development

No branches or pull requests

5 participants