flarum / issue-archive

0 stars 0 forks source link

Markdown code block highlighting doesn't work unless bbcode is enabled #86

Open clarkwinkelmann opened 3 years ago

clarkwinkelmann commented 3 years ago

Bug Report

Current Behavior If you enable the flarum/markdown extension but not flarum/bbcode extension, the markdown code block won't have its syntax highlighted, even if you specify a language.

Steps to Reproduce

  1. Disable all extensions except flarum/markdown
  2. Post something like:

    ```js let var = 'Hello World'; ```

Expected Behavior The block should be highlighted without having to enable bbcode.

Screenshots I'd take a screenshot but since highlighting is currently broken in TextFormatter it doesn't render for either situation. However we can see in the HTML that the script tag for highlightjs is missing when bbcode is disabled.

With bbcode enabled:

image

Without bbcode:

image

Environment

Possible Solution I don't know enough about TextFormatter to know what the best approach is. It seems like using the Litedown bundle does not enable the highlighting feature of bbcode, but I don't know if it should.

I don't know if we have a way to enable the highlighting from Markdown extension without also enabling the [code] bbcode.

Related code:

https://github.com/flarum/bbcode/blob/master/extend.php https://github.com/flarum/markdown/blob/master/extend.php

clarkwinkelmann commented 3 years ago

Summoning @JoshyPHP for feedback :innocent:

JoshyPHP commented 3 years ago

Yes, the Litedown plugin intentionally uses plain code blocks to better resemble the output of other Markdown-style libraries.

I believe you can retrieve and reuse the tag created for the default [CODE] BBCode with the following command, either before or after enabling the Litedown plugin:

$configurator->tags['CODE'] = $configurator->BBCodes->repositories['default']->get('CODE')['tag'];

You could just copy the tag's template but it's probably safer to copy the whole tag in case it requires some specific attribute filtering. Here's what I used to test it in isolation:

$configurator = new s9e\TextFormatter\Configurator;

$configurator->Litedown;
$configurator->tags['CODE'] = $configurator->BBCodes->repositories['default']->get('CODE')['tag'];

extract($configurator->finalize());

$text = "[code]...[/code]\n\n"
      . "```php\n"
      . "echo 'Hello world';\n"
      . '```';
$xml  = $parser->parse($text);
$html = $renderer->render($xml);

die("$html\n");
<p>[code]...[/code]</p>

<pre><code class="language-php">echo 'Hello world';</code><script async="" crossorigin="anonymous" data-hljs-style="github" integrity="sha384-t+82niWiIIbaln9oQeGls7B9M6d5/fZztzYzNtizyxznmrh/vGK2ZOkvvUKXGfA/" onload="hljsLoader.highlightBlocks(this.parentNode)" src="https://cdn.jsdelivr.net/gh/s9e/hljs-loader@1.0.23/loader.min.js"></script></pre>
JoshyPHP commented 3 years ago

Also, this may be something you'd want to implement as its own extension. Something that creates the CODE tag that will be used by other extensions, be it BBCodes, Litedown, or whatever else. You could have a user setting to customize the default style, e.g.:

$configurator->BBCodes->repositories['default']->get('CODE', ['style' => 'dark'])

Or you can replace it with a template parameter so you can choose the style dynamically for light/dark themes. The following snippet removes the default value then add an xsl:value-of element to the template:

$configurator = new s9e\TextFormatter\Configurator;

// During configuration
$configurator->Litedown;
$tag = $configurator->BBCodes->repositories['default']->get('CODE', ['style' => ''])['tag'];
$dom = $tag->template->asDOM();
foreach ($dom->query('//xsl:attribute[@name="data-hljs-style"]') as $xslAttribute)
{
    $xslAttribute->appendXslValueOf('$HLJS_STYLE');
}
$dom->saveChanges();
$configurator->tags['CODE'] = $tag;

// Might as well set a default value
$configurator->rendering->parameters['HLJS_STYLE'] = 'github';

extract($configurator->finalize());

$text = "```php\n"
      . "echo 'Hello world';\n"
      . '```';
$xml  = $parser->parse($text);
$html = $renderer->render($xml);

echo "$html\n\n";

// Change at rendering time
$renderer->setParameter('HLJS_STYLE', 'dark');
$html = $renderer->render($xml);

echo "$html\n";
<pre><code class="language-php">echo 'Hello world';</code><script async="" crossorigin="anonymous" data-hljs-style="github" integrity="sha384-t+82niWiIIbaln9oQeGls7B9M6d5/fZztzYzNtizyxznmrh/vGK2ZOkvvUKXGfA/" onload="hljsLoader.highlightBlocks(this.parentNode)" src="https://cdn.jsdelivr.net/gh/s9e/hljs-loader@1.0.23/loader.min.js"></script></pre>

<pre><code class="language-php">echo 'Hello world';</code><script async="" crossorigin="anonymous" data-hljs-style="dark" integrity="sha384-t+82niWiIIbaln9oQeGls7B9M6d5/fZztzYzNtizyxznmrh/vGK2ZOkvvUKXGfA/" onload="hljsLoader.highlightBlocks(this.parentNode)" src="https://cdn.jsdelivr.net/gh/s9e/hljs-loader@1.0.23/loader.min.js"></script></pre>

You may also want to consider removing the script element from the template altogether and move it to Flarum's template to avoid creating multiple script elements when multiple code blocks are displayed.

askvortsov1 commented 3 years ago

You may also want to consider removing the script element from the template altogether and move it to Flarum's template to avoid creating multiple script elements when multiple code blocks are displayed.

This also somewhat relates to our CSP issues, since that script has to be dynamically inserted, which isn't the safest approach in general.

We should probably disable that dynamic script insertion, and instead force extensions to run their own scripts.