EnlighterJS / Plugin.WordPress

:package: Official WordPress Plugin of EnlighterJS
http://wordpress.org/plugins/enlighter/
GNU General Public License v2.0
115 stars 17 forks source link

Crayon Syntax Highlighter parsing creates malformed whitespace #351

Closed dmadison closed 1 year ago

dmadison commented 2 years ago

Hi, I'm trying to switch a site from Crayon Syntax Highlighter to Enlighter and I'm running into an issue where text following a code block is not enclosed within paragraph tags and spurious breaks are introduced.

Steps to reproduce:

  1. Install WordPress (latest, 5.9.3)
  2. Install Classic Editor plugin (latest, 1.6.2) and activate
  3. Install Enlighter (latest, 4.5.0) and activate
  4. In Enlighter options, enable compatibility mode and Crayon compatibility
  5. Create a new post using the classic editor with the following content:

    <h2>Hello World</h2>
    Let's try an example:
    <pre class="lang:cpp decode:true">#include &lt;iostream&gt;
    
    int main() {
        std::cout &lt;&lt; "Hello World!";
        return 0;
    }
    </pre>
    The example above prints "Hello World".
  6. Publish the post and look at the result.

Expected Output:

<p>Let’s try an example:</p>
<div class="enlighter-default"></div> <!--abridged--> 
<p>The example above prints “Hello World”.</p>

Actual Output:

<p>Let’s try an example:
  <br>
</p>
<div class="enlighter-default"></div> <!--abridged--> 
<br>
The example above prints “Hello World”.
<p></p>
Full Output ```html

Let’s try an example:

#include <iostream>
int main() {
std::cout << "Hello World!";
return 0;
}
#include <iostream> int main() { std::cout << "Hello World!"; return 0; }
#include <iostream>

int main() {
  std::cout << "Hello World!";
  return 0;
}

The example above prints “Hello World”.

```

On my own website's theme this creates terrible whitespace problems. On the default Twenty-Twenty Two theme this pushes the next line outside the width of the post text:

enlighter-demo


This only appears to be a problem with legacy posts using the classic editor. The same content using the block editor is parsed fine and matches the expected output above.

Block editor post source ```html

Hello World

Let's try an example:

#include <iostream>

int main() {
    std::cout << "Hello World!";
    return 0;
}

The example above prints "Hello World".

```
AndiDittrich commented 2 years ago

Hi @dmadison

i'm unable to reproduce your issue:


Editor

image


Result

image


Page source

image

dmadison commented 2 years ago

How strange. I've tested with two installs (the existing site and the minimally reproducible example) and both have the same issue.

Your entry content looks slimmer than my full output. Could this be caused by a configuration difference?

Here is the MRE docker-compose file in case it's helpful:

docker-compose.yml ```yml services: wordpress: image: wordpress:5.9.3-apache ports: - 8080:80 environment: WORDPRESS_DB_HOST: db WORDPRESS_DB_USER: exampleuser WORDPRESS_DB_PASSWORD: examplepass WORDPRESS_DB_NAME: exampledb volumes: - type: bind source: ./site target: /var/www/html db: image: mysql:5.7 environment: MYSQL_DATABASE: exampledb MYSQL_USER: exampleuser MYSQL_PASSWORD: examplepass MYSQL_RANDOM_ROOT_PASSWORD: '1' volumes: - type: bind source: ./db target: /var/lib/mysql volumes: wordpress: db: ```
AndiDittrich commented 2 years ago

maybe it's the wpautop filter..

generally the compatibility mode doesn't change the markup structure. the code wrapper is just replaced with the EnlighterJS pre wrapper. EnlighterJS hides this container and injects a div container with the highlighted code above.

dmadison commented 2 years ago

I think you're right, and it's likely an interaction between the two. If I add the paragraph tags myself then everything appears fine, and it also appears fine if I disable Enlighter and use the un-highlighted pre tags.

AndiDittrich commented 2 years ago

normally i would disable wpautop - it's a very weak "hack" together with TinyMCE.

dmadison commented 2 years ago

I did try that, but unfortunately that just makes the whitespace issues worse. I also tried removing and re-adding wpautop at a lower filter priority and that did not seem to help either.

I know I can work around the problem by going through and re-editing all of the older posts, but unfortunately I don't have that sort of time at the moment.

AndiDittrich commented 2 years ago

try wpautop with a priority >=9999

see https://github.com/EnlighterJS/Plugin.WordPress/blob/master/modules/filter/FragmentBuffer.php for reference

dmadison commented 2 years ago

That did it! Looks like I wasn't aggressive enough with the priority level. At 9997 the issue still appears, at 9998+ it's fine:

add_filter( 'the_content', 'reprioritize_autop', 0 );

function reprioritize_autop($content) {
    remove_filter( 'the_content', 'wpautop' );
    add_filter( 'the_content', 'wpautop', 9999 );
    return $content;
}

I tested with both the full site and the test bed and that seems to have fixed it (in the post body, anyways). Is that an issue that can be fixed on the plugin side?

AndiDittrich commented 2 years ago

adding a pre wrapper around the filter placeholder may did the trick but i'm not sure if it might break other things.

dmadison commented 2 years ago

Following up on this. Is there any chance of a fix being merged into the plugin?

AndiDittrich commented 2 years ago

changing the enlighter regex filter may cause side effects for other users...i'm not sure howto deal with the issue - your workaround seems to work as expected or not ?

dmadison commented 2 years ago

It seems to work as expected for the test case, although I have yet to test it thoroughly with the rest of the site and the other plugins.

I'm not a web developer by trade, so I'm sure there's a more elegant way to accomplish this without changing the underlying regex filter. Perhaps the Crayon compatibility class could add an option to reprioritize wpautop?

AndiDittrich commented 2 years ago

basic rule for plugin development: never touch those filters by a plugin - it has quite a lot side effects and breaks a lot of other things. the only chance is the modification of the regex filter to add a pre wrapper tag around the placeholders - but as already said - this might also cause some side effects

dmadison commented 2 years ago

the only chance is the modification of the regex filter to add a pre wrapper tag around the placeholders - but as already said - this might also cause some side effects

Where would this go? I've tried adding it quick + dirty to CompatibilityModeFilter.php#L64:

                // run convert
                $code = CrayonCompat::convert($match);

                // generate code; retrieve placeholder
                return $this->_fragmentBuffer->storeFragment("<pre>" . $code . "</pre>");

Although the output still has the same whitespace issue. Am I adding this in the wrong place?

basic rule for plugin development: never touch those filters by a plugin - it has quite a lot side effects and breaks a lot of other things.

That makes sense. Although since wpautop can't be modified, it sounds to me like the Enlighter compatibility filter may be at the wrong relative priority?

As a test, modifying the filter priority in ContentProcessor#L165 from 0 to 10 fixes the issue (although it will presumably cause issues with shortcodes?):

    public function registerFilterTarget($filter, $name){
        // add content filter to first position - replaces all enlighter shortcodes with placeholders
        add_filter($name, array($filter, 'stripCodeFragments'), 10, 1);

Working off of that, I modified the registerFilterTarget() function to take a priority argument with a default of 0:

    public function registerFilterTarget($filter, $name, $priority=0){
        // add content filter to first position - replaces all enlighter shortcodes with placeholders
        add_filter($name, array($filter, 'stripCodeFragments'), $priority, 1);

And then I modified the compatibility mode call (L149) to use a priority of 10:

            // register filter targets
            foreach ($compatSections as $section){
                $this->registerFilterTarget($this->_compatFilter, $section, 10);
            }

This appears to solve the issue. Do you foresee any problems with this fix?

dmadison commented 2 years ago

Following up on this one last time. Do you see any issues with this fix? Should I create a pull request?

AndiDittrich commented 2 years ago

didn't had any time to check&validate the issue. just changing the priority doesn't solve the issue and causes other side effects (shortcode etc..) - the <pre> wrapper should to the trick

dmadison commented 2 years ago

It's entirely possible that I'm implementing it incorrectly, but the <pre> wrapper did not fix the issue for me.

Either way it's now been a month since I've opened this issue, and unfortunately I'm out of time. Here's the solution I'm going with:


I've made the following updates to ContentProcessor.php. Changing the registerFilterTarget function to accept an optional priority argument (same as above):

// add content filter (strip + restore) to the given content section
public function registerFilterTarget($filter, $name, $priority=0){
    // add content filter to first position - replaces all enlighter shortcodes with placeholders
    add_filter($name, array($filter, 'stripCodeFragments'), $priority, 1);

And modifying the filter registration to isolate the Crayon implementation and use a different filter priority for it:

    // register filter targets
    foreach ($compatSections as $section){
        // Use priority of 10 with Crayon to avoid wpautop issues
        if($this->_config['compat-crayon']) {
            $this->registerFilterTarget($this->_compatFilter, $section, 10);
        }
        else {
            $this->registerFilterTarget($this->_compatFilter, $section);
        }
    }

This applies the syntax highlighting after wpautop has already run, and matches the same filter priority that the Crayon plugin had originally. The Crayon plugin doesn't use shortcodes, so I don't anticipate any issues with that, at least.

I have noticed no ill effects so far, but I'll update this issue if I run into any problems. If anyone has a similar issue you can find my fix on my fork.

Feel free to close this issue if you cannot replicate the problem on your end.

AndiDittrich commented 1 year ago

the pre wrapper solution has been added to all content filters which solves your issue in all of my testcases - https://github.com/EnlighterJS/Plugin.WordPress/commit/6ed5d71032d477bf5f5f25733a9b6649e04a7f0b

dmadison commented 1 year ago

Thank you for following up on this. I must have been implementing the <pre> wrapper incorrectly. The latest version does fix this issue.