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

HTML comment handling doesn't match spec 0.31.2? #285

Closed
robinst opened this issue Mar 2, 2024 · 3 comments · Fixed by #286
Closed

HTML comment handling doesn't match spec 0.31.2? #285

robinst opened this issue Mar 2, 2024 · 3 comments · Fixed by #286

Comments

@robinst
Copy link
Contributor

robinst commented Mar 2, 2024

I'm updating commonmark-java to version 0.31.2 of the spec and was wondering about HTML comment handling. Namely, given this spec:

An HTML comment consists of <!-->, <!--->, or <!--, a string of
characters not including the string -->, and --> (see the
HTML spec).

I think these two should be parsed as comments:

foo <!-- test --->
foo <!-- test ---->

However, with the current dingus they're not (example 1, example 2):

<p>foo &lt;!-- test ---&gt;</p>
<p>foo &lt;!-- test ----&gt;</p>

The reason is probably this regex:

var HTMLCOMMENT = "<!-->|<!--->|<!--(?:[^-]|-[^-]|--[^>])*-->"

I think <!-->|<!--->|<!--.*?--> would work as expected.

@jgm
Copy link
Member

jgm commented Mar 2, 2024

OK, this looks like a holdover from our old XML-ish definition of comment.
Do you want to make PR?

@jgm
Copy link
Member

jgm commented Mar 2, 2024

I think cmark may also need to be fixed.
scanners.re has

  htmlcomment = "--" ([^\x00-]+ | "-" [^\x00-] | "--" [^\x00>])* "-->";

@robinst
Copy link
Contributor Author

robinst commented Mar 3, 2024

Do you want to make PR?

Yeah can do later. Do we want some additional examples in the spec as well?

robinst added a commit to robinst/commonmark.js that referenced this issue Mar 3, 2024
@jgm jgm closed this as completed in #286 Mar 6, 2024
jgm pushed a commit that referenced this issue Mar 6, 2024
robinst added a commit to commonmark/commonmark-java that referenced this issue Mar 9, 2024
Looks like commonmark.js has a bug in its handling:
commonmark/commonmark.js#285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants