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

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

Open
5e-Cleric opened this issue May 2, 2024 · 3 comments
Labels
bug We say this works but it doesn't ❌ Missing from V3 This should be in v3 P1 - high priority Obvious bug or popular features solution found A solution exists; just needs to be applied

Comments

@5e-Cleric
Copy link
Member

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 5e-Cleric added bug We say this works but it doesn't ❌ Missing from V3 This should be in v3 P3 - low priority Obscure tweak or fix for a single user labels May 2, 2024
@5e-Cleric
Copy link
Member Author

5e-Cleric commented May 2, 2024

@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
Copy link
Collaborator

Gazook89 commented May 3, 2024

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

@5e-Cleric 5e-Cleric changed the title Markdown tables present a weird bug Markdown variables delete bug id another parenthesis is present May 3, 2024
@calculuschild
Copy link
Member

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.

let content = match[11] ? match[11].trim().replace(/\s+/g, ' ') : null; // Trim edge spaces and shorten blocks of whitespace to 1 space

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.

if(i > -1) {
combinedRegex.lastIndex = combinedRegex.lastIndex - (content.length - i);
content = content.slice(0, i).trim().replace(/\s+/g, ' ');
}

@5e-Cleric 5e-Cleric added solution found A solution exists; just needs to be applied P1 - high priority Obvious bug or popular features and removed P3 - low priority Obscure tweak or fix for a single user labels May 7, 2024
@calculuschild calculuschild changed the title Markdown variables delete bug id another parenthesis is present Markdown variables consume too many characters if parentheses are nested with whitespace before final ) May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug We say this works but it doesn't ❌ Missing from V3 This should be in v3 P1 - high priority Obvious bug or popular features solution found A solution exists; just needs to be applied
Projects
None yet
Development

No branches or pull requests

3 participants