markdown: Fix bullet list indentation bug

[gaharavara]

Previously, the frontend-only javascript processor didn't show the same
output as the backend, resulting in wrong output shown in drafts.

Fixes #10966

Tested various changes to the list implementation in marked.js and the final working solution that I came up with was, since we follow 2 spaces for the next item in a list why not replace all the odd number of spaces to even by reducing 1 space from them, thus adding a replace statement.

[zulipbot]

Hello @zulip/server-markdown members, this pull request was labeled with the "area: markdown" label, so you may want to check it out!

[timabbott]

@AsociTon I'm skeptical that this won't create other bugs, but am not an expert in the marked.js implementation. @aero31aero @tommyip FYI in case you have ideas for a better solution.

[aero31aero]

I would like to see some test cases along with this.

Off the top of my head, I don't think we need the /gm flags here, and the regex seems to be specific to only these problems.

The following test case, for example, would still be different on the frontend and backend because the frontend treats anything beyond four as a 4 space indented codeblock.

- zero
 - one
  - two
   - three
    - four
     - five
      - six
       - seven
        - eight

[gaharavara]

I will add some test cases.

I checked by removing /gm flags but removal of any of the global or multiline flag makes the logic ineffective, also when we use - or + for instead of * the frontend logic doesn't treats them as list indicators while the backend does.

[gaharavara]

found this bug while adding node tests, due to the removal of the space the last list element if had odd number of spaces would have the last character left in src which was further processed into <p></p> tags causing the above bug, so added 1 to the substring method to take care of such case, another approach I thought of was to apply replace on src too but it would just increase the overall cost of the operation unnecessarily so added 1 to the substring, it works fine don't know why test has failed, will look more into it.

[gaharavara]

I don't know how I missed it at that time, but just repositioning the replace statement was enough to remove the above raised bug, finally did it : ) , and all the test cases pass it too.FYI @timabbott

[gaharavara]

Included a frontend test for the odd number of spaces mixed with even spaces for the marked.js implementation, One more case that would be great to add once #11860 is resolved would be to create a miscellaneous test case comprising of +,-,* along with odd and even spacing and that would actually cover almost everything for lists in case of frontend.

[gaharavara]

ready for a review : )

[timabbott]

@aero31aero can you give this another review?

[aero31aero]

Tested manually, this looks good to me.

…

On Mon, Apr 22, 2019, 5:39 AM Tim Abbott ***@***.***> wrote: @aero31aero <https://github.com/aero31aero> can you give this another review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11830 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB5JHVT2EVR53SVYT4XAXK3PRT64VANCNFSM4G4XDCLA> .

[aero31aero]

Actually, I'm sorry I forgot to run that earlier test case with 8 levels of indents. It is still failing. How many levels of list indents do we want to support?

Regardless, I would like to see that added as a test-case in markdown-test-cases.json, even though the backend and the frontend render it differently.

[zulipbot]

Heads up @gaharavara, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

[timabbott]

Ideally an arbitrary number of indents, unless there's some reason that's technically infeasible. You can look at babelmark for how other markdown processors handle this.

[timabbott]

Closing as we're generally thinking of standardizing our markdown processor via aligning with Djot or commonmark rather than doing custom changes like this.