fix: match html block start conditions when ending a list item

[giaBaoJS]

Marked version: 18.0.11 (master, 53cb13f)

Markdown flavor: CommonMark

Description

No issue for this one. #3437 reported that an HTML tag on the line after a list item ends up inside the <li>, and #3444 fixed it for opening tags. The closing-tag half was never fixed, and the same regex has a second problem in the other direction.

src/rules.ts line 98:

htmlBeginRegex: cachedIndentRegex((indent) => new RegExp(`^ {0,${indent}}<(?:[a-z].*>|!--)`, 'i')),

Tokenizer.ts:354 uses it to decide whether the next line ends the current list item. It never matches </..., and [a-z].*> matches every tag name including ones that start a type 7 HTML block.

Expectation

<div class="some-class">

- list item
</div>

<div class="some-class"><ul>
<li>list item</li>
</ul>
</div>

Result

<div class="some-class"><ul>
<li>list item</div></li>
</ul>

The </div> is pulled inside the <li>, so the caller's wrapper is never closed and the browser closes it at the </li> instead. Minimal repro: marked.parse('- a\n</div>\n') returns <ul>\n<li>a</div></li>\n</ul>\n.

The second direction, same regex:

- list item
<b>bold</b>

marked returns <ul>\n<li>list item</li>\n</ul>\n<p><b>bold</b></p>\n. The <b> ends the list. CommonMark 0.31.2 keeps it in the item, because a type 7 block cannot interrupt the paragraph inside the list item.

What was attempted

Both cases are the same start-condition test. Spec 0.31.2, "HTML blocks":

All types of HTML blocks except type 7 may interrupt a paragraph. Blocks of type 7 may not interrupt a paragraph.

and start condition 6:

Start condition: line begins with the string < or </ followed by one of the strings (case-insensitive) address, article, ... tr, track, ul, followed by a space, a tab, the end of the line, the string >, or the string />.

(source: https://github.com/commonmark/commonmark-spec/blob/0.31.2/spec.txt, lines 2396 to 2408 and 2449 to 2450)

marked already encodes exactly that set: the paragraph rule at src/rules.ts:174 uses '</?(?:tag)(?: +|\\n|/?>)|<(?:script|pre|style|textarea|!--)' with the comment "pars can be interrupted by type (6) html blocks". This PR reuses the same expression for htmlBeginRegex instead of inventing a new one. The only change is \n becomes $, because htmlBeginRegex is tested against a single line with the newline already stripped, and the spec condition includes "the end of the line".

Two behaviours it deliberately does not change, to stay consistent with the html rule rather than be locally more correct:

• a tab after the tag name (<div\t>) still does not match, because html and the paragraph rule both use +
• start conditions 3, 4 and 5 are still absent, as they are in the paragraph rule

Blast radius

I generated 57,456 documents (7 list markers x 4 leading indents x 6 continuation indents x 56 continuation lines x 6 tails, over 5 nesting shapes) and diffed master against this branch, using the commonmark 0.31.2 package already in devDependencies as the reference. Comparison ignores whitespace, so pure newline differences do not count.

	agreement with commonmark 0.31.2
master	36,518 / 57,456 (63.56%)
this branch	49,810 / 57,456 (86.69%)

Of the 15,692 documents whose output changes, 13,292 go from disagreeing with the reference to matching it, and 0 go the other way. The remaining 2,400 disagree with the reference both before and after, for reasons this regex does not control (list tightness whitespace, tab and 4-space continuation indents, task-list markers).

Narrowing to one variable, - one\n<LINE>\n, 18 of the 56 continuation lines change behaviour:

"</div>"        before=!=ref  after=ref      "<b>"          before=!=ref  after=ref
"</div >"       before=!=ref  after=ref      "<span>"       before=!=ref  after=ref
"</p>"          before=!=ref  after=ref      "<em x='1'>"   before=!=ref  after=ref
"</ul>"         before=!=ref  after=ref      "<a href='x'>" before=!=ref  after=ref
"</li>"         before=!=ref  after=ref      "<x-widget>"   before=!=ref  after=ref
"</table>"      before=!=ref  after=ref      "</DIV>"       before=!=ref  after=ref
"</blockquote>" before=!=ref  after=ref      "<div"         before=!=ref  after=ref
"</div"         before=!=ref  after=ref      "<p"           before=!=ref  after=ref
"</p"           before=!=ref  after=ref      "<div\t>"      before=!=ref  after=!=ref

17 move to matching the reference, none regress, and <div\t> is the tab case noted above.

For contrast, the one-character fix of just adding /? to the existing pattern (^ {0,n}</?(?:[a-z].*>|!--)) produces 4,316 regressions on the same corpus, across </script>, </pre>, </style>, </b>, </span>, </em>, </a> and </x-widget>: a closing type 7 tag would start ending list items. That is why this uses the tag list rather than [a-z].

CommonMark and GFM spec completion tables are identical before and after.

ReDoS

npm run test:redos skips this regex (test/recheck.ts has // TODO: skip functions for now for the cachedIndentRegex entries), so I ran recheck against it directly at all four indent values cachedIndentRegex can produce:

indent=0 status=safe complexity={"type":"safe","summary":"safe (fuzz)"}
indent=1 status=safe complexity={"type":"safe","summary":"safe (fuzz)"}
indent=2 status=safe complexity={"type":"safe","summary":"safe (fuzz)"}
indent=3 status=safe complexity={"type":"safe","summary":"safe (fuzz)"}

The new pattern is an alternation of literal prefixes with no nested quantifier; it also removes the .* the old one had.

Tests

Two spec files in test/specs/new plus a unit test on the regex. They fail on different mutations:

mutation	html_closing_tag_following_list	html_inline_tag_in_list	html_following_list (existing, unchanged)
current master regex	fail	fail	pass
add /? only	pass	fail	pass
drop </?, keep the tag list	fail	pass	pass
this PR	pass	pass	pass

html_following_list is the control from #3444: an opening block tag must still end the list, and it does.

Local run: 1789 to 1793 spec tests, 191 to 192 unit tests, 0 failures. test:lint, test:types, test:umd and test:cjs pass.

Contributor

• Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
• no tests required for this PR.
• If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

• CI is green (no forced merge required).
• Squash and Merge PR following conventional commit guidelines.

[vercel]

@giaBaoJS is attempting to deploy a commit to the MarkedJS Team on Vercel.

A member of the Team first needs to authorize it.

[UziTech]

I think spec tests should be enough. We don't need lexer tests as well

[giaBaoJS]

Removed. The one case the specs don't cover is a block tag with no > on the line (<div with its attributes on the next line) ending a list item; want that as a spec test, or leave it?

[UziTech]

Ya you could add it as a spec test

[giaBaoJS]

Added as html_multiline_tag_following_list.

[UziTech]

Thanks! 💯