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 1 commit 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.
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
|
Should this also remove tabs or other whitespace at the beginning of the next line? |
2 participants
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.