Skip to content

Commit

Permalink
Markdown: Workaround for incorrect highlighting due to double wrap …
Browse files Browse the repository at this point in the history 
…hook (#2719)

The hook that highlights code blocks in markdown code was unable to handle code blocks that were highlighted already. The hook can now handle any existing markup in markdown code blocks.
  • Loading branch information
@RunDevelopment
RunDevelopment committed Jan 29, 2021
1 parent 30b0444 commit 2b355c9
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 3 deletions.
6 changes: 4 additions & 2 deletions components/prism-markdown.js
View file
Expand Up @@ -347,8 +347,10 @@
});
}
} else {
// reverse Prism.util.encode
var code = env.content.replace(/&lt;/g, '<').replace(/&amp;/g, '&');
// get the textContent of the given env HTML
var tempContainer = document.createElement('div');
tempContainer.innerHTML = env.content;
var code = tempContainer.textContent;

env.content = Prism.highlight(code, grammar, codeLang);
}
Expand Down
2 changes: 1 addition & 1 deletion components/prism-markdown.min.js
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 comments on commit 2b355c9

@LeaVerou
Copy link
Member

@LeaVerou LeaVerou commented on 2b355c9 Jul 2, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually ran into issues of double highlighting with regular Prism too and had to employ hacks to heuristically detect if highlighting had already ran. I wonder if this is something we need to fix more centrally.

@RunDevelopment
Copy link
Member Author

@RunDevelopment RunDevelopment commented on 2b355c9 Jul 2, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is something we need to fix more centrally.

We certainly do.

The underlying problem here is that hooks aren't removed when the language gets reloaded. This leads to the same hook being executed multiple times.

To fix this, we have to figure out how to properly unload plugins, a long-standing issue (#459).

@RunDevelopment
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think our best shot for solving #459 will be the new dependency system (#2880) planned for v2.0. With some relatively minor additions, we can include hooks into that system. This gives us enough information to let Prism load and unload hooks automatically.

@LeaVerou
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem is present even when no language gets reloaded, just with simple stock Prism included via a script tag, if you just pass it the same element to highlight twice.

@RunDevelopment
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's what you meant. You had an issue with Keep Markup, right? Yeah, that definitely a problem too.

@mAAdhaTTah
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we start on v2 & drop IE11 support, we could hold onto already-highlighted nodes in a Set or WeakSet so we can tag them as already-highlighted.

@RunDevelopment
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE11 does support most of Set and WeakMap. So whatever solution you have in mind, we could probably do it right now.

@LeaVerou
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't work, because we still want to be able to overwrite a node's contents and highlight it again.

@mAAdhaTTah
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% clear on the larger problem, but I was just thinking Set/WeakSet to solve the more direct problem, tracking whether highlighting already ran. The plugins that have problems when reloading can track to avoid re-highlighting, if that's something we want to do.

@RunDevelopment
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugins that have problems when reloading can track to avoid re-highlighting, if that's something we want to do.

That's all plugins. This is a fundamental problem with the way we reload components. I don't think tracking already highlighted elements will help.

Please sign in to comment.