naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.07k stars 326 forks source link

Markdown variables consume too many characters if parentheses are nested with whitespace before final ) #3448

Open 5e-Cleric opened 4 months ago

5e-Cleric commented 4 months ago

Not sure what is causing this issue, but it is a bug of some sort.

Some combination of links and parentheses inside tables merges cells together, sometimes splitting words.

the following code will work as a demonstration:

|  title 1  | title 2 | title 3 | title 4|
|-----------|---------|---------|--------|
|[foo](bar) |  Ipsum  |    )    |   )    |

results in this: image

5e-Cleric commented 4 months ago

@Gazook89 has reportedly tried this combination in other markdown editors without being able to replicate this behaviour, therefore, we are drawn to the conclusion the bug must come from our own code, most likely our table handling code in \shared\naturalcrit\markdown.js.

Pointed out by @calculuschild, the error might be new and related to variables.

Gazook89 commented 4 months ago

I think the issue is a bad regex.

in markdown.js:

    const inlineDefRegex  = /([!$]?\[((?!\s*\])(?:\\.|[^\[\]\\])+)\])\(([^\n]+)\)/;                         //Matches 8, 9[10](11)

should be changed to

    const inlineDefRegex  = /([!$]?\[((?!\s*\])(?:\\.|[^\[\]\\])+)\])\(([^\n)]+)\)/;                         //Matches 8, 9[10](11)

which adds a ) in the disallowed characters in the third matching group (([^\n)]+))

image
calculuschild commented 4 months ago

Discussed on Gitter, the real issue is this:

As easy as let content = match[11] ? match[11] : null;, just removing the trim and regex substitution, since we need to do the parenthesis matching to occur before the trim.

https://github.com/naturalcrit/homebrewery/blob/dd82a1bebdb7daa77d0d7eb787fd66a658593259/shared/naturalcrit/markdown.js#L599

The trim will then happen afterward here. Note that the if(i > -1) will always be true since there is no opportunity for i to be equal or less than -1. That was leftover from an earlier piece of code, and so we can just pull the inside of the if out.

https://github.com/naturalcrit/homebrewery/blob/dd82a1bebdb7daa77d0d7eb787fd66a658593259/shared/naturalcrit/markdown.js#L615-L618