Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Markdown: Workaround for incorrect highlighting due to double
wrap
…
…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
2b355c9
There was a problem hiding this comment.
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.
2b355c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
2b355c9
There was a problem hiding this comment.
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.
2b355c9
There was a problem hiding this comment.
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.
2b355c9
There was a problem hiding this comment.
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.
2b355c9
There was a problem hiding this comment.
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
orWeakSet
so we can tag them as already-highlighted.2b355c9
There was a problem hiding this comment.
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
andWeakMap
. So whatever solution you have in mind, we could probably do it right now.2b355c9
There was a problem hiding this comment.
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.
2b355c9
There was a problem hiding this comment.
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.
2b355c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.