Issue: CR/LF line-breaks can introduce extra line breaks during highlighting

[deniskovalchuk]

Workaround

This is a known issue and being discussed (see below). If you run into this the correct solution is to normalize your content to only \n line breaks BEFORE highlighting.

---

Describe the issue
Extra line breaks are added to comments if your DOS code uses Windows-style
line endings (CRLF). The problem is not reproduced if Unix-style line endings
are used (LF).

I'm not aware of the internal details of highlight.js, but I can assume a
problem with using \s in the COMMENT regular expression.

highlight.js/src/languages/dos.js

Line 11 in 4fb68ad

/^\s*@?rem\b/, /$/,

Which language seems to have the issue?
DOS

Are you using highlight or highlightAuto?
highlightElement

Sample Code to Reproduce
Run the samples below and see console output.

Windows-style line endings (bad):
https://jsfiddle.net/ua3om8fb/1/

Unix-style line endings (good):
https://jsfiddle.net/8xw4vq39/3/

Expected behavior

<span class="hljs-comment">rem line1</span>
<span class="hljs-comment">rem line2</span>

Actual behavior

<span class="hljs-comment">rem line1</span>
<span class="hljs-comment">
rem line2</span>

[joshgoebel]

Please confirm which web browser or browsers you are seeing this behavior with. 

[joshgoebel]

Nevermind, this is a problem with the engine itself (not browser/DOM). It can be reproduced with highlight easily:

const s = "rem line1\r\nrem line2"
r = hljs.highlight(s, {language: "dos"})

// a dance just to make things visible
console.log(r.value.replace("\r","\\r").replace("\n","\\n"));

I'm not sure it's correct to blame the grammar though... this is quite easy to do using regex and it wouldn't surprise me if there were other examples in other grammars... I think a fix might have to be higher-level (and broader).

The problem is our HTML is being inserted in-between the \r and \n which seems to make them each count separately as a line-break.

[joshgoebel]

@highlightjs/core Ever run into this before?

We use /$/ to match endings a lot, which leaves the line-ending encoding (\r or \r\n) outside the match... which is good... the problem comes when you have a match that starts with ^\s as the ^ seems to pin against the line ending yet leave the \n "after" and the \r "before" [the ^]... (ah, NO, actually both \r and \n match ^ or $ - when in multi-line mode of course) so when we insert HTML now we've split the sequence.

Ideas:

• Just forcing grammars to use [\t ] everywhere doesn't seem like much fun at all... this is an easy mistake for grammars to make (I recall we've fixed related issues in the past, but I don't ever recall line-encoding being mentioned)
• Add special handling for line endings in the parser engine (ugh)
• I'm a little tempted to just pre-normalize all content to "correct" line endings (\n) 😇, or say we don't support \r\n...

A little surprised this has never come up before. Any thoughts?

[joshgoebel]

I feel like I'm torn between two directions:

• "This is just how regex works", (that ^ and $ pin EVERY line ending) we need to suck it up and deal with it, fix these broken regex in however many grammars, add some tests... (perhaps we could easily automate the tests)
• "regex is dumb sometimes", we should "magically" just handle this in a nice way so that people never have to deal with this insanity. (normalizing solves the most common case)

Also worth noting that this also matters for repeating sequences like \n\n\n, though that's seemed to be less of a problem - thats' likely where the [ \t] bugs have been in the past.

[joshgoebel]

I think all our test content is \n... I was imagining possibly:

• Use existing markup tests content
• (A) Highlight content as-is
• (B) Highlight content with \n replaced with \r\n
• Assert the count of individual new line sequences (A == B)

IE, the number of line break sequences should not change either way. One could also perhaps just check the output for \r(?!\n) I suppose? (ie "hanging" solo \rs (without a paired \n == BAD)

[joshgoebel]

@deniskovalchuk This is not a super high priority item so if you're in a rush I'd suggest stripping all Windows line feeds before sending your content to the web and using Unix line feeds - which always have and continue to work without any issues. This could even be done post HTML in the Javascript itself with a tiny plugin or hook.

[joshgoebel]

I'm a little tempted to just pre-normalize all content to "correct" line endings (\n) 😇, or say we don't support \r\n...

I think in the browser we should do this (strip \r, normalizing the line endings before highlight), and then close this issue and see if it ever comes up again in a non-browser context. Is there any context where this would be problematic?

[joshgoebel]

Some new research:

https://stackoverflow.com/questions/14217101/what-character-represents-a-new-line-in-a-text-area

By HTML specifications, browsers are required to canonicalize line breaks in user input to CR LF (\r\n), and I don’t think any browser gets this wrong. Reference: clause 17.13.4 Form content types in the HTML 4.01 spec.

This holds true in my testing. Your example is only broken because you're using the createTextNode API which I assume allows you to slip those characters back in. This is a very unusual use case and I think I'm fine saying we simply don't support it... if you're dynamically inserting content like this, then you need to normalize the line endings yourself (same as the browser would).

Update: This says "user input" but I'm finding this true of the actual web content itself (loaded from disk)... in both Safari and Chrome (on OS X)... not sure if that is a representative sample or not. Can you provide a hosted HTML page that's actually broken on the internet without resorting to the DOM API to build a broken test case? If so then the fix here will but what I proposed above, adding normalization to highlightElement...

---

There may still be an issue here for highlight on the server-side (with \r\n content), but at this point I don't see a client-side issue anymore.

[allejo]

highlightjs/core Ever run into this before?

Yup! In highlight.php. This gave me a headache until I figured out it was line endings. However, the regex engine that PHP uses internally (PCRE) allows you to change the behavior of $ (with ANYCRLF).

https://github.com/scrivo/highlight.php/blob/f08b7326a45e1590eb7e739d4fa0ed5b031bab37/src/Highlight/RegExUtils.php#L46-L60

However, that doesn't solve the problem at hand: the JS regex engine doesn't support this natively. This is the second time I've ever seen this brought up; the first time being in the PHP context. I'd be okay with stripping out the \r, it'd create a bit of a performance hit on large code blocks but I'd imagine that's negligible.

[joshgoebel]

However, the regex engine that PHP uses internally (PCRE) allows you to change the behavior of $ (with ANYCRLF).

That's a nice trick!