Deprecate HTML auto-merging from core (plugin may be needed)

[joshgoebel]

Background from isagalaev in #2529:

I was walking by, and while I didn't read through the entire thing I wanted to say this. There were a few features added to the core at users' requests when it was all small an nimble. These days I would say it makes much more sense to be opinionated and remove corner cases like useBR, tabWidth and several different spellings of "lang-", "language-" as language prefixes in the class name (as far as I remember there is a very precise recommendation in HTML5 to use "language-"). Everyone with special cases would be expected to do pre- or post-processing.

Very early on we were perhaps a bit too open and allowed some things to slip into core that perhaps should not have - and today they are all things that could easily be handled with our new plugin API.

---

HTML merging is a large swath of complexity (see utils.js) that a few use for a corner case like:

var x;
<span class="important">var y;</span>

Which will be highlighted as (the HTML "passes thru"):

<span class="keyword">var</span> x;
<span class="important"><span class="keyword">var</span> y;</span>

Sadly, this feature also makes it impossible for us to detect HTML injection type mistakes (which we could otherwise detect and warn about)... #2886 HTML (generally) should not exist inside a code block - it should always be escaped. Its existence possibly indicates an HTML injection style vulnerability. If we remove this final edge case for HTML then we can inform users about the potential vulnerability vs letting them potentially shoot themselves in the foot.

This can now be easily achieved via a "after:highlightBlock" plugin and that's what should happen for anyone who absolutely needs this functionality.

• v10.5 Deprecate HTML merging (chore(plugins) move useBR, tabReplace, and HTML merging into internal plugins #2873)
• Someone may want to provide a plugin
• v11 Remove HTML merging

---

You can restore this functionality via a plugin. Below is plugin that we shipped as part of our own source briefly after this was ported to a plugin to make later extraction easy.

A maintainer (someone who wants to package this up as an official plugin and maintain it) is needed.

Usage:

hljs.addPlugin(mergeHTMLPlugin);

Plugin source:

var mergeHTMLPlugin = (function () {
  'use strict';

  var originalStream;

  /**
   * @param {string} value
   * @returns {string}
   */
  function escapeHTML(value) {
    return value
      .replace(/&/g, '&')
      .replace(/</g, '&lt;')
      .replace(/>/g, '&gt;')
      .replace(/"/g, '&quot;')
      .replace(/'/g, '&#x27;');
  }

  /* plugin itself */

  /** @type {HLJSPlugin} */
  const mergeHTMLPlugin = {
    // preserve the original HTML token stream
    "before:highlightElement": ({ el }) => {
      originalStream = nodeStream(el);
    },
    // merge it afterwards with the highlighted token stream
    "after:highlightElement": ({ el, result, text }) => {
      if (!originalStream.length) return;

      const resultNode = document.createElement('div');
      resultNode.innerHTML = result.value;
      result.value = mergeStreams(originalStream, nodeStream(resultNode), text);
      el.innerHTML = result.value;
    }
  };

  /* Stream merging support functions */

  /**
   * @typedef Event
   * @property {'start'|'stop'} event
   * @property {number} offset
   * @property {Node} node
   */

  /**
   * @param {Node} node
   */
  function tag(node) {
    return node.nodeName.toLowerCase();
  }

  /**
   * @param {Node} node
   */
  function nodeStream(node) {
    /** @type Event[] */
    const result = [];
    (function _nodeStream(node, offset) {
      for (let child = node.firstChild; child; child = child.nextSibling) {
        if (child.nodeType === 3) {
          offset += child.nodeValue.length;
        } else if (child.nodeType === 1) {
          result.push({
            event: 'start',
            offset: offset,
            node: child
          });
          offset = _nodeStream(child, offset);
          // Prevent void elements from having an end tag that would actually
          // double them in the output. There are more void elements in HTML
          // but we list only those realistically expected in code display.
          if (!tag(child).match(/br|hr|img|input/)) {
            result.push({
              event: 'stop',
              offset: offset,
              node: child
            });
          }
        }
      }
      return offset;
    })(node, 0);
    return result;
  }

  /**
   * @param {any} original - the original stream
   * @param {any} highlighted - stream of the highlighted source
   * @param {string} value - the original source itself
   */
  function mergeStreams(original, highlighted, value) {
    let processed = 0;
    let result = '';
    const nodeStack = [];

    function selectStream() {
      if (!original.length || !highlighted.length) {
        return original.length ? original : highlighted;
      }
      if (original[0].offset !== highlighted[0].offset) {
        return (original[0].offset < highlighted[0].offset) ? original : highlighted;
      }

      /*
      To avoid starting the stream just before it should stop the order is
      ensured that original always starts first and closes last:

      if (event1 == 'start' && event2 == 'start')
        return original;
      if (event1 == 'start' && event2 == 'stop')
        return highlighted;
      if (event1 == 'stop' && event2 == 'start')
        return original;
      if (event1 == 'stop' && event2 == 'stop')
        return highlighted;

      ... which is collapsed to:
      */
      return highlighted[0].event === 'start' ? original : highlighted;
    }

    /**
     * @param {Node} node
     */
    function open(node) {
      /** @param {Attr} attr */
      function attributeString(attr) {
        return ' ' + attr.nodeName + '="' + escapeHTML(attr.value) + '"';
      }
      // @ts-ignore
      result += '<' + tag(node) + [].map.call(node.attributes, attributeString).join('') + '>';
    }

    /**
     * @param {Node} node
     */
    function close(node) {
      result += '</' + tag(node) + '>';
    }

    /**
     * @param {Event} event
     */
    function render(event) {
      (event.event === 'start' ? open : close)(event.node);
    }

    while (original.length || highlighted.length) {
      let stream = selectStream();
      result += escapeHTML(value.substring(processed, stream[0].offset));
      processed = stream[0].offset;
      if (stream === original) {
        /*
        On any opening or closing tag of the original markup we first close
        the entire highlighted node stack, then render the original tag along
        with all the following original tags at the same offset and then
        reopen all the tags on the highlighted stack.
        */
        nodeStack.reverse().forEach(close);
        do {
          render(stream.splice(0, 1)[0]);
          stream = selectStream();
        } while (stream === original && stream.length && stream[0].offset === processed);
        nodeStack.reverse().forEach(open);
      } else {
        if (stream[0].event === 'start') {
          nodeStack.push(stream[0].node);
        } else {
          nodeStack.pop();
        }
        render(stream.splice(0, 1)[0]);
      }
    }
    return result + escapeHTML(value.substr(processed));
  }

  return mergeHTMLPlugin;

}());

This was built with:

rollup src/plugins/merge_html.js -f iife --output.name=mergeHTMLPlugin

After changing the source to have a single default export.

[yakov116]

This can now be easily achieved via a "after:highlightBlock" plugin and that's what should happen for anyone who absolutely needs this functionality.

How?

• v10.5 Deprecate HTML merging (chore(plugins) move useBR, tabReplace, and HTML merging into internal plugins #2873)

This belongs in a major version No? This broke my whole site, unless I am doing something wrong.

[joshgoebel]

How?

Someone would need to write it (or simply extract it from 10.5). The code is easily found in merge_html.js and already in plug-in form. 10.5 simplify switched from it being hard-coded to using the plugin framework.

This belongs in a major version No?

The removal yes, but we haven't removed it.

This broke my whole site, unless I am doing something wrong.

It's been deprecated, not removed. 10.5 should work just fine as 10.4 did... deprecating means we are giving people a heads up that in v11 this will be gone and then need to make other arrangements if they are using this feature.

---

If you're having a specific issue with version 10.5 please open an issue and we'll be happy to take a look. For something specific a like this a http://jsfiddle.com/ is probably best.

[joshgoebel]

@yakov116 I haven't seen any issues filed, did you sort out your problem then? 10.5 has been one of the quietest releases in a while.

[yakov116]

I did not, I am really busy now so I did not have time to debug exactly where the issue is. So I just reverted to an older version.

I wish I was able to, maybe one day this week

[joshgoebel]

FYI: You don't necessarily have to debug it yourself... if you just post a jsfiddle that reproduces the issue it in a small, controlled environment (separate from your larger project, which always has the potential of being the real issue).

But of course that also takes a small amount of time... so we'll be here whenever. ;)

The fiddle I use for small test cases (which you can fork):

https://jsfiddle.net/ajoshguy/2bmdswn6/

[yakov116]

Debugged it, It has to do with the

          if (options.useBR) {
            block.innerHTML = block.innerHTML.replace(/\n/g, '').replace(/<br[ /]*>/g, '\n');
          }

Cant figure out why but I will just stop using it (useBr) and replace it myself.

Just so you can see the difference

<code id="TextResult" class="language-sql hljs" style="display: inherit;"><span class="variable">(</span><span class="hljs-keyword">Select</span> <span contenteditable=""><span class="hljs-keyword">First</span></span>(<span contenteditable=""><span class="hljs-keyword">IIF</span>(IsNumeric(Phone),<span class="hljs-keyword">Format</span>(Phone,<span class="hljs-string">'!(&amp;&amp;&amp;) &amp;&amp;&amp;-&amp;&amp;&amp;&amp; &amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;'</span>),Phone)</span>) <span class="hljs-keyword">From</span> <span contenteditable="">MPhone</span> <span contenteditable=""><span class="hljs-keyword">Where</span> Code = Members.Code</span> <span class="hljs-keyword">and</span> <span contenteditable=""><span class="hljs-keyword">Desc</span> = <span class="hljs-string">'home'</span></span>) <span class="hljs-keyword">As</span> <span contenteditable=""></span><span contenteditable=""></span><span contenteditable="">home</span><span contenteditable="">,</span><span contenteditable=""></span><br>
<span class="hljs-keyword">(Selec</span>t<span class="hljs-keyword"> </span><br><span contenteditable=""><br><span class="hljs-keyword">Firs</span>t<br></span><br><span class="hljs-keyword">(</span><br><span contenteditable=""><br><span class="hljs-keyword">II</span>F(IsNumeric(Phone)<span class="hljs-keyword">,Forma</span>t(Phone<span class="hljs-string">,'!(&amp;&amp;&amp;) &amp;&amp;&amp;-&amp;&amp;&amp;&amp; &amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;</span>'),Phone)<br></span><br>)<span class="hljs-keyword"> Fro</span>m <br><span contenteditable=""><br>MPhone<br></span><br><span class="hljs-keyword"> </span><br><span contenteditable=""><br><span class="hljs-keyword">Wher</span>e Code = Members.Code<br></span><br><span class="hljs-keyword"> an</span>d<span class="hljs-keyword"> </span><br><span contenteditable=""><br><span class="hljs-keyword">Des</span>c =<span class="hljs-string"> 'phone</span>'<br></span><br>)<span class="hljs-keyword"> A</span>s <br><span contenteditable=""></span><span contenteditable=""></span><span contenteditable=""><br>phone<br></span><span contenteditable=""><br>,<br></span><span contenteditable=""></span><br></code

vs (old version)

<code id="TextResult" class="language-sql hljs" style="display: inherit;"><span class="variable">(</span><span class="hljs-keyword">Select</span> <span contenteditable=""><span class="hljs-keyword">First</span></span>(<span contenteditable=""><span class="hljs-keyword">IIF</span>(IsNumeric(Phone),<span class="hljs-keyword">Format</span>(Phone,<span class="hljs-string">'!(&amp;&amp;&amp;) &amp;&amp;&amp;-&amp;&amp;&amp;&amp; &amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;'</span>),Phone)</span>) <span class="hljs-keyword">From</span> <span contenteditable="">MPhone</span> <span contenteditable=""><span class="hljs-keyword">Where</span> Code = Members.Code</span> <span class="hljs-keyword">and</span> <span contenteditable=""><span class="hljs-keyword">Desc</span> = <span class="hljs-string">'home'</span></span>) <span class="hljs-keyword">As</span> <span contenteditable=""></span><span contenteditable=""></span><span contenteditable="">home</span><span contenteditable="">,</span><span contenteditable=""></span><br>(<span class="hljs-keyword">Select</span> <span contenteditable=""><span class="hljs-keyword">First</span></span>(<span contenteditable=""><span class="hljs-keyword">IIF</span>(IsNumeric(Phone),<span class="hljs-keyword">Format</span>(Phone,<span class="hljs-string">'!(&amp;&amp;&amp;) &amp;&amp;&amp;-&amp;&amp;&amp;&amp; &amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;&amp;'</span>),Phone)</span>) <span class="hljs-keyword">From</span> <span contenteditable="">MPhone</span> <span contenteditable=""><span class="hljs-keyword">Where</span> Code = Members.Code</span> <span class="hljs-keyword">and</span> <span contenteditable=""><span class="hljs-keyword">Desc</span> = <span class="hljs-string">'phone'</span></span>) <span class="hljs-keyword">As</span> <span contenteditable=""></span><span contenteditable=""></span><span contenteditable="">phone</span><span contenteditable="">,</span><span contenteditable=""></span></code>

I think there is something wrong in the regex.

[joshgoebel]

I don't think the regex changed... that code still runs, it was just moved to a plugin. Pretty sure we have tests for it, and they are still green.

[yakov116]

I know I will figure it out as a later time. Sorry for the bother

[joshgoebel]

No bother.

[joshgoebel]

Just an update (on my end). I took a quick review or the PR that touched this last and don't see anything obvious amiss here.

Here is the new "plugin" code:

https://github.com/highlightjs/highlight.js/pull/2873/files#diff-6214d870917fc8dbaf180113af76d2f1a674571612cd92e1f5500c04d159ac48R669-R681

And the whole PR: #2873

Pretty much it just moved code around. Always possible we broke something (or another PR did) but I'm not seeing it yet. If you end up with a reproducible test case, we'll be here. :)

[joshgoebel]

contenteditable is a small red-flag though. We're not designed to be editable. (if it works, great, but this isn't something we really support) Could be you're seeing some complex interaction related to other stuff you're doing on your end. Just a guess.

But useBR is on it's way out... so if already have your own solution, then that should keep working in the future. Optimally though you'd just use a HTML tag (or CSS) that preserves whitespace.

[joshgoebel]

Making this the deprecation only. Remove is tracked in v11 checklist. Marking complete as this PR was merged already.

[shaunix]

If this plugin is being moved out of tree, can we put it in a repo where multiple interested people can work on it, so I don't have to maintain a copy myself? Not clobbering markup in code was one of the hard requirements when we were evaluating syntax highlighters for the yelp documentation framework. So either I have to maintain a plugin, stay on an old version, or shop for a new highlighter (which I very much do not want to do). We build and bundle our own copy, so pulling in a maintained plugin isn't a problem per se, but I fear I might be constantly chasing API breakages.

For your consideration, putting markup in code blocks is pretty common in developer docs, and I don't think it's a fringe feature at all. Many documentation formats allow it, and even have features that specifically require it. Some common uses include:

• Highlighting parts of code that were added or changed since the last block in a tutorial.
• Adding callouts to label or explain parts of a code block.
• Linking functions to reference docs.

I can't break these features for a syntax highlighter. I understand why you might not want to allow markup if code blocks could be potentially garbage submitted by internet randos on a site like StackExchange (although surely those sites should have their own sanitizers). But for many documentation generators, there is no risk of HTML injections, and features require markup.

[joshgoebel]

can we put it in a repo where multiple interested people can work on it, so I don't have to maintain a copy myself?

Sure, though you don’t need our blessing to do that. That said, I don’t think we’d mind hosting a repo in the main organization, as we also offer for 3rd party grammar modules - as long as the README mentioned it was a 3rd party plugin and the Issues were open for those needing support. We could also create a PLUGINS.md to start to hold a list of 3rd party plugins (even those hosted elsewhere).

I fear I might be constantly chasing API breakages.

The whole point of our official documented plugin API to is to address this concern. We have a pretty strict policy about breaking changes with our APIs (and in general) and this typically only happens during a major release (semver). Only minor breaking API changes are currently planned for v11, and IIRC none related to the plugin architecture.

If you take the time to extract this to a separate repo and in the process had any specific questions or concerns (about our plugin APIs and future proofing) I’d be happy to field them, but without specifics I believe this is perhaps an unfounded fear.

putting markup in code blocks is pretty common in developer docs, and I don't think it's a fringe feature at all.

Perhaps I should not have so quickly presented it as a “corner case” - though I’m not certain I’m mistaken. We have millions of downloads/users. It’s possible that more use this feature than we imagine while it ALSO (statistically) remains a corner case.

But I will clarify:

Removal from core is not necessarily an expression or suggestion that a feature is uncommon or fringe, only that it should not exist in core. For example, see our long standing philosophy on line numbering - which has been a request since the beginning of time. Numbering is a common and useful thing in many use cases, yet purposely not a core feature. It’s easily enough implemented as a wrapper - any many have done so (even before plugins). Other things have slipped into core prior to our new plugin architecture.

The plugin architecture purposely exists to support use cases exactly like this one (and many, many others) outside of the core library. Now that we have it in place it’s time for certain features to continue their life going forward as plugins. Core desires to remain small and simple and hyper focused on it’s two core missions:

• Converting raw source into clean, highlighted HTML
• Auto-detection of language given a code snippet

Please also see: #2225

---

So under what circumstances might we remove features from core?

• to remove obvious (or non-obvious) corner cases that have slipped in over time (legacy stuff)
• a feature does not fit with our core mission
• to reduce code size and complexity (less code and less complex code is easier to maintain)
• to reduce the number of security incidents (smaller surface area for bugs/exploits)
• to reduce the number of support issues
• a feature is easily solved with a small plugin (leaving core clean and small)

...and often many of these will apply simultaneously since many are inter-related.

[joshgoebel]

For anyone following this thread: this is still not supported officially (in core) but I did just update the sample plugin source above such that it now works with the latest 11 releases (it now uses the correct callback hooks)...

[eric-wieser]

How much work would it be to publish the script at the top of this issue at https://github.com/highlightjs/highlight.js-html-merge or similar, without any guarantee of maintenance?

[joshgoebel]

We already link to all the legacy plugins (including this one) on the Wiki... I'd rather not go further at this time. Hosting a separate repository is too close to "official/supported" for my taste.

[alanhogan]

Removal from core is not necessarily an expression or suggestion that a feature is uncommon or fringe, only that it should not exist in core.

That’s tautologically true, but when your guides to getting started with highlight don’t include a way to use that feature, it absolutely is an expression that such usage/feature is fringe. Removing the feature altogether and suggesting that it could be a plugin instead is absolutely saying a whole lot more than 'we don’t want the code in core anymore'.

[joshgoebel]

guides to getting started with highlight don’t include a way to use that feature

Because it's not a feature. So far no one has expressed a desire to develop or maintain the plugin. If someone wanted to step up and maintain the plugin officially (perhaps you'd like to?) I think we'd be happy to link to it when someone asks "how do I do that?".