fix: drop the leading whitespace after a hard line break - #4075
fix: drop the leading whitespace after a hard line break#4075Kjubikstronk wants to merge 3 commits into
Conversation
CommonMark ignores leading spaces at the start of the line following a hard line break. The br rule consumed the break but not that indentation, so it stayed on the following text token. Fixes CommonMark examples 636 and 637. Neither marked's differ nor an exact comparison can see this: the differ ignores whitespace, and exact still differs on <br> versus <br />. Under the spec's own normalize.py the count goes from 28 to 26.
|
@Kjubikstronk is attempting to deploy a commit to the MarkedJS Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
||
| describe('hard line break', () => { | ||
| it('should drop the leading whitespace of the continuation line', () => { | ||
| assert.strictEqual(marked.parse('foo \n bar\n'), '<p>foo<br>bar</p>\n'); |
There was a problem hiding this comment.
Shouldn't there be a new line character after the <br>?
There was a problem hiding this comment.
marked does not emit one, and that part is not new to this PR. Its own fixtures already encode the current output:
test/specs/new/breaks.html
<p>A<br>B</p>test/specs/new/breakline.html
<p>A<br>break line test<br>Special <code>code</code>A<br>break line test</p>CommonMark's reference is <p>foo<br />\nbar</p>, so marked differs from it on both the self closing slash and the newline. The spec suite never flags either, because htmlIsEqual normalises both.
So the newline is a separate question from this fix, and adding it here would change breaks, breakline, and every other fixture with a <br> in it. Happy to open a separate issue if you think marked should match the reference there.
There was a problem hiding this comment.
We have an extension (marked-xhtml) for the self closing slash, so we don't need that in marked. I think we should add the new line after br but that can be a separate issue.
There was a problem hiding this comment.
Understood on both.
Dropped the self closing slash from the discussion entirely, since marked-xhtml covers it and it was only ever incidental to how I was measuring.
On the newline after <br>: agreed it should be separate, and I would rather not smuggle it in here since it would change breaks, breakline and every other fixture containing a <br>. Happy to open the issue and do the work if you want it, just say and I will file it with the fixture list.
There was a problem hiding this comment.
Let's add tests in test/specs/new instead of marked.test.js
There was a problem hiding this comment.
I tried that first and the fixture cannot fail.
I added test/specs/new/hardbreak_leading_whitespace.md with the three cases, reverted the br change, and reran. It still passes:
> hardbreak_leading_whitespace
ok hardbreak_leading_whitespace should pass
runTests compares with htmlIsEqual, and testutils constructs @markedjs/html-differ with only ignoreSelfClosingSlash and ignoreComments set, so ignoreWhitespaces stays at its default of true. The bug here is only whitespace, <p>foo<br> bar</p> against <p>foo<br>bar</p>, so nothing under test/specs can see it, and there is no per fixture option to turn the normalisation off.
That is the same blind spot that let examples 636 and 637 report as passing all along, which is what made me call this a non bug in #4050 in the first place.
I am happy to add the fixture anyway if you want it as documentation, but it would pass whether or not the code is correct. If marked.test.js is the wrong home for the real assertions, tell me where you would like them and I will move them.
There was a problem hiding this comment.
You can use renderExact option like test/specs/new/whiltespace_lines.md
There was a problem hiding this comment.
That is the piece I was missing, and it makes what I told you wrong. Sorry, I should have grepped test/specs for an escape hatch before claiming there wasn't one.
Moved in 382f844. test/specs/new/hardbreak_leading_whitespace.md with renderExact: true, covering all six cases in one fixture: the two space and backslash spellings, a tab, a mix of spaces and tabs, and two soft break controls. The unit tests are gone from marked.test.js.
It does bite. Reverting the br change:
> hardbreak_leading_whitespace
not ok hardbreak_leading_whitespace should pass
because renderExact goes through assert.strictEqual instead of isEqual, which is exactly the thing I said the suite could not do.
|
Should this also remove tabs or other whitespace at the beginning of the next line? |
The rule only consumed spaces, so a continuation line indented with a tab kept it. Both the reference implementation and markdown-it drop tabs there as well.
|
Yes, good catch. It should, and it did not. Fixed in 2342586. The spec has no tab example in Hard line breaks, so I checked the implementations instead. Both drop tabs there, writing
So the rule is now const br = /^( {2,}|\\)\n(?!\s*$)[ \t]*/;Three unit tests added, covering a tab, a mix of spaces and tabs, and a soft break with a leading tab as a control, since that one must keep its tab and does. Full spec suite and unit suite pass. |
Uses renderExact so the fixture compares exactly, which htmlIsEqual does not, and the assertions keep their teeth outside marked.test.js.
Marked version: 18.0.11 (53cb13f)
Markdown flavor: CommonMark
Description
CommonMark ignores leading spaces at the start of the line following a hard line break. The
brrule is:It consumes the break itself but not the next line's indentation, so that indentation stays on the following text token and is rendered.
Expectation
gives
<p>foo<br />\nbar</p>.Result
<p>foo<br> bar</p>. The five spaces survive.What was attempted
Appending
*to the rule, so the break consumes the indentation that follows it.This one is easy to measure wrongly, which is worth writing down. Neither of the comparisons normally used here can see it:
htmlIsEqual, which leavesignoreWhitespacesat its default oftrue, so the difference is invisible and examples 636 and 637 already report as passing with noshouldFailflag.<br>where the spec's reference output has<br />followed by a newline.Under the spec's own
test/normalize.py, which normalises the self-closing slash but keeps a single space that begins a text node after a tag, the whole-spec count goes from 28 to 26. The two that move are exactly 636 and 637, and nothing else changes, in both the CommonMark and GFM runs.Credit where it is due: I originally measured this with the two comparisons above, concluded it was not a bug, and said so in #4050. @exit0-run showed the
normalize.pymeasurement and was right.Contributor
Three tests in
test/unit/marked.test.jsunderhard line break, covering the two-space and backslash spellings plus a soft break as a control. The first two fail without the change; the control passes either way. Full spec suite (1789) and unit suite (194) pass.Committer
In most cases, this should be a different person than the contributor.