App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Android - Live mark down is not rendered when Description field is opened after saving it

Open lanitochka17 opened this issue 1 year ago β€’ 12 comments

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.72-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://github.com/Expensify/App/pull/53367 Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch ND or hybrid app.
  2. Go to FAB > Submit expense > Manual
  3. Enter amount > Next
  4. Select a recipient
  5. Tap Description
  6. Enter text with mark down and save
  7. Tap Description again

Expected Result:

The description field will show live mark down for the content with mark down

Actual Result:

Live mark down is not rendered when Description field is opened after saving it The same issue also happens in Private notes, room description and workspace description

Workaround:

Unknown

Platforms:

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

  • [x] Android: Standalone
  • [x] Android: HybridApp
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Standalone
  • [ ] iOS: HybridApp
  • [ ] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/7b361c8a-89de-47ef-b3a7-ff253ec10fcb

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021864791346170591685
  • Upwork Job ID: 1864791346170591685
  • Last Price Increase: 2024-12-05
Issue OwnerCurrent Issue Owner: @eVoloshchak

lanitochka17 avatar Dec 05 '24 18:12 lanitochka17

Triggered auto assignment to @arosiclair (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] avatar Dec 05 '24 18:12 melvin-bot[bot]

πŸ’¬ A slack conversation has been started in #expensify-open-source

melvin-bot[bot] avatar Dec 05 '24 18:12 melvin-bot[bot]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

github-actions[bot] avatar Dec 05 '24 18:12 github-actions[bot]

I reproduced on Android v9.0.72-0. Looks like iOS is unaffected.

arosiclair avatar Dec 05 '24 19:12 arosiclair

None of the PRs in the checklist seem to be the issue.

I reproduced in dev with the latest on main, but I also reproduced with the last version, v9.0.71.2 checked out. I'm getting the sense that this never worked as expected. I already updated my staging app from the Play Store so I'm having trouble finding a way to revert it to the last version to confirm.

arosiclair avatar Dec 05 '24 20:12 arosiclair

Looks like markdown doesn't render at all in the Description field on v9.0.70-9. This looks like a new feature and not a regression. Removing the blocker label

arosiclair avatar Dec 05 '24 20:12 arosiclair

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

melvin-bot[bot] avatar Dec 05 '24 20:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 05 '24 21:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 05 '24 21:12 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-12-11 09:47:37 UTC.

Proposal

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

Live markdown is not rendered when Description field is opened after saving it (the same thing happens in Private notes, room description and workspace description fields).

What is the root cause of that problem?

The root of this issue comes from the Android: Native implementation of the applyMarkdownFormatting function call here in the MarkdownTextWatcher.java, specifically the fact that we only call it on afterTextChanged:

@Override
public void afterTextChanged(Editable editable) {
  if (editable instanceof SpannableStringBuilder) {
    mMarkdownUtils.applyMarkdownFormatting((SpannableStringBuilder) editable);
  }
}

the problem with this being evident from the method name -> the live markdown styling is only applied when the after input changes and not when the input is mounted as it happens on iOS: Native for example.

This is also confirmed by the fact that if we change the text or remove a markdown symbol and add it back -> the live markdown will be applied again and show up as expected.

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

[!note] I don't have a lot of experience with android native code, but I'll give this a shot and hopefully thereact-native-live-markdown maintainers would be kind enough to chime in and correct any technical oversight if we are to move forward with this solution.

What we want here is to apply the live markdown formatting when the input is mounted (regardless of its focus state), to do this we need to apply the following change in the @expensify/react-native-live-markdown repo:

// add the imports at the top
import android.text.Editable;
import android.text.SpannableStringBuilder;

// add this inside, at the end of this if block
if (previousSibling instanceof ReactEditText) {
  AssetManager assetManager = getContext().getAssets();
  MarkdownUtils.maybeInitializeRuntime(assetManager);
  mMarkdownUtils = new MarkdownUtils(assetManager);
  mMarkdownUtils.setMarkdownStyle(mMarkdownStyle);
  mReactEditText = (ReactEditText) previousSibling;
  mTextWatcher = new MarkdownTextWatcher(mMarkdownUtils);
  mReactEditText.addTextChangedListener(mTextWatcher);
  Editable editable = mReactEditText.getText();     // <- new line
  if (editable instanceof SpannableStringBuilder) {     // <- new line
      mMarkdownUtils.applyMarkdownFormatting((SpannableStringBuilder) editable);     // <- new line
  }     // <- new line
}

The reason we add it there is because at that point the input is mounted and the next logical step would be to apply the markdown formatting. What the new logic does is it extracts the text from the input and applies the markdown formatting.

Or, to make things simpler we can add 1 line instead of the ones above, as suggested in https://github.com/Expensify/App/issues/53645#issuecomment-2535309095:

// add this inside, at the end of this if block
if (previousSibling instanceof ReactEditText) {
  AssetManager assetManager = getContext().getAssets();
  MarkdownUtils.maybeInitializeRuntime(assetManager);
  mMarkdownUtils = new MarkdownUtils(assetManager);
  mMarkdownUtils.setMarkdownStyle(mMarkdownStyle);
  mReactEditText = (ReactEditText) previousSibling;
  mTextWatcher = new MarkdownTextWatcher(mMarkdownUtils);
  applyNewStyles();     // <- new line
}

[!important] This solution will fix the issue in all inputs where the issue is present, as required by the Expected result.

If there are any other details I missed or some technical oversight, I think that can be further discussed during review.

cc @tomekzaw @BartoszGrajdek for visibility since this is react-native-live-markdown related

Result video

Android: Native

https://github.com/user-attachments/assets/d5c08639-f5e2-423e-8488-348fc9f524cc

ikevin127 avatar Dec 06 '24 04:12 ikevin127

I agree with solution proposed by @ikevin127 in https://github.com/Expensify/App/issues/53645#issuecomment-2522101175 but I'd like to understand the issue a bit more since in the live-markdown example app, the Markdown formatting is already applied immediately during first render even without this change.

tomekzaw avatar Dec 06 '24 23:12 tomekzaw

What I'm thinking is that on native, the screen which contains this input remains in the stack (for navigation purposes) and when reopening it and since there was no change, the android native code does not include logic to applyMarkdownFormatting when the screen containing the input is mounted again (revived from the stack).

This is the only explanation that would make sense to me when comparing our case with the live-markdown example app where there's no navigation stack involved as the markdown input is added directly in the App.tsx component.

It's just a hunch though, not entirely sure how to debug this to see why, other than looking at the code and figuring that it's because applyMarkdownFormatting is currently only called in afterTextChanged as mentioned in the proposal.

ikevin127 avatar Dec 06 '24 23:12 ikevin127

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

melvin-bot[bot] avatar Dec 09 '24 09:12 melvin-bot[bot]

Someone from SWM will also take a look at this to make sure we understand the root cause properly

cc @j-piasecki @Kicu

tomekzaw avatar Dec 11 '24 07:12 tomekzaw

@arosiclair, @eVoloshchak, @zanyrenney Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] avatar Dec 11 '24 09:12 melvin-bot[bot]

but I'd like to understand the issue a bit more since in the live-markdown example app, the Markdown formatting is already applied immediately during the first render even without this change.

It seems like in the live-markdown example, there's a (native) state update happening immediately after onAttachedToWindow is called. This state update causes the text to change, which in turn triggers the markdown text watcher.

The difference in the New Dot is that the state update doesn't happen or is received before onAttachedToWindow is called, not sure which. Though, I can see navigation delaying onAttachedToWindow call.

It may make sense to apply formatting in onAttachedToWindow as well, as not to rely on the state update triggering a text change.

j-piasecki avatar Dec 11 '24 09:12 j-piasecki

Thanks @j-piasecki for checking this!

@ikevin127 We can proceed with your solution proposed in https://github.com/Expensify/App/issues/53645#issuecomment-2522101175.

One more thing – what do you think if instead of

  Editable editable = mReactEditText.getText();     // <- new line
  if (editable instanceof SpannableStringBuilder) {     // <- new line
      mMarkdownUtils.applyMarkdownFormatting((SpannableStringBuilder) editable);     // <- new line
  }     // <- new line

we simply call

  applyNewStyles();

? This way we prevent code duplication as well as make sure that text selection is preserved.

tomekzaw avatar Dec 11 '24 09:12 tomekzaw

Looks good to me, thanks a lot everybody for chiming in and helping validate the root cause and improve the solution!

Updated proposal

cc @eVoloshchak for proposal review and if everything checks out, assignment

Once that happens, I'll be able to move forward with opening a PR on react-native-live-markdown then once merged there, will follow-up with the version bump on E/App.

ikevin127 avatar Dec 11 '24 09:12 ikevin127

πŸ“£ 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] avatar Dec 12 '24 16:12 melvin-bot[bot]

@arosiclair, @eVoloshchak, @zanyrenney 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] avatar Dec 13 '24 09:12 melvin-bot[bot]

@eVoloshchak can you review please

arosiclair avatar Dec 13 '24 16:12 arosiclair

@ikevin127's proposal looks mighty good

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed!

eVoloshchak avatar Dec 16 '24 21:12 eVoloshchak

Current assignee @arosiclair is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] avatar Dec 16 '24 21:12 melvin-bot[bot]

πŸ“£ @ikevin127 πŸŽ‰ 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[bot] avatar Dec 17 '24 13:12 melvin-bot[bot]

Will open PR today!

ikevin127 avatar Dec 17 '24 18:12 ikevin127

Opened https://github.com/Expensify/react-native-live-markdown/pull/590 PR, tagged @tomekzaw as I'm not sure how review is handled there. Once that PR is merged, will sync up with the team in order to follow up with the version bump PR on E/App.

ikevin127 avatar Dec 18 '24 05:12 ikevin127

E/App PR https://github.com/Expensify/App/pull/54326 is open and ready for review! πŸš€

Note: From react-native-live-markdown version 0.1.209 to 0.1.210 there's only 1 PR and it's the fix mentioned in the comment above, therefore this is a straight-forward bump with only 1 change on the library's side.

@arosiclair I think an Android: Native build on the PR would facilitate testing and help ensure the fix works as expected before we merge the PR.

ikevin127 avatar Dec 18 '24 20:12 ikevin127

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

melvin-bot[bot] avatar Dec 31 '24 21:12 melvin-bot[bot]

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

  • https://github.com/Expensify/App/pull/54326

If no regressions arise, payment will be issued on 2025-01-07. :confetti_ball:

For reference, here are some details about the assignees on this issue:

  • @eVoloshchak requires payment through NewDot Manual Requests
  • @ikevin127 requires payment automatic offer (Contributor)

melvin-bot[bot] avatar Dec 31 '24 21:12 melvin-bot[bot]

@eVoloshchak / @ikevin127 @zanyrenney @eVoloshchak / @ikevin127 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Dec 31 '24 21:12 melvin-bot[bot]