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

converter.makeHtml with dashes terminated with nbsp takes a long time #312

Closed
rolandliwag opened this issue Dec 16, 2016 · 10 comments
Closed
Assignees
Labels

Comments

@rolandliwag
Copy link

To reproduce:

const converter = new require('showdown').Converter();
return converter.makeHtml('- - - - - - - - - - - - - - - - - - - - - - - - - - - - ');

As dashes are added to the left, it takes more and more time. Removing the nbsp (char code 160) from the end makes it behave much better.

@tivie tivie self-assigned this Dec 17, 2016
@tivie tivie added the bug label Dec 17, 2016
@tivie
Copy link
Member

tivie commented Dec 17, 2016

The bug was verified. For some reason, nbsp breaks the hr algorithm and dashes are interpreted as list marks! I'm looking into it

@tivie
Copy link
Member

tivie commented Dec 17, 2016

Upon further inspection, it seems the bug occurs with any character (and not only with nbsp) after a series of dashes or asterisks.

From what I could gather, this bug is present from the beginning of showdown and is common to other markdown parsers (such as marked or commonmark).

The problem lies with the list parser. Basically it interprets the line as a series of nested lists (which is wrong).

I'm currently working on a fix, but I have to rewrite the list parser.

@tivie
Copy link
Member

tivie commented Dec 17, 2016

10b3410 and 6e90f7c should address this issue

@tivie tivie closed this as completed in 10b3410 Dec 17, 2016
@rolandliwag
Copy link
Author

Thanks very much. Would it be possible to also publish a new npm version?

@tivie
Copy link
Member

tivie commented Dec 17, 2016

already did, :)

@rolandliwag
Copy link
Author

rolandliwag commented Dec 19, 2016

Wasn't sure whether to create a new issue but it seems this is still a problem when more characters are added to the right of nbsp this time. eg:

const converter = new require('showdown').Converter();
return converter.makeHtml('- - - - - - - - - - - - - - - - - - - - - - - - - - - - A');

@tivie
Copy link
Member

tivie commented Dec 19, 2016

Yes, it seems that, although the output is now correct, something is still slowing down the library in this specific context.

I'm investigating. However, I would ask you to open a new issue

@rolandliwag
Copy link
Author

Will do.

@yurikhan
Copy link

Replacing all non-breaking spaces with normal spaces breaks their intended use.

To reproduce:

(new showdown.Converter()).makeHtml('Enough text to\u00a0make the\u00a0following wrap to\u00a0a\u00a0new line')

Expected result: A paragraph that never wraps immediately after an article or preposition.

Observed result: A paragraph that wraps wherever it pleases.

Please consider revising the fix, e.g. by deleting non-breaking spaces at the end of line only (which is never the intended use of a non-breaking space anyway).

@tivie
Copy link
Member

tivie commented May 28, 2017

@yurikhan

nbsp causes trouble in older browsers and some regex flavors

Actually, I can rollback the "nbsp fix", since the issue was not caused by nbsp but rather a logic bug in the parser, which was fixed.

I ask you to open a new issue with this, and I will roll a option for allowing nbsp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants