Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoiding double highlighting #2384

Closed
LeaVerou opened this issue May 15, 2020 · 9 comments · Fixed by #3166
Closed

Avoiding double highlighting #2384

LeaVerou opened this issue May 15, 2020 · 9 comments · Fixed by #3166
Labels

Comments

@LeaVerou
Copy link
Member

So, I run into a bit of an issue, and I was wondering what's the best way to handle this.

My (relevant) setup:

  • Both Prism Live and Prism included. Some <pre>s highlighted with Prism, some made editable with Prism Live (using <pre> and not <textarea> as source).
  • Keep Markup plugin

Both Prism and Prism Live run Prism on the <pre>s and since the Keep Markup plugin is included, the tokens from the first highlight stay for the second, so every token becomes nested (e.g. <span class="token keyword"><span class="token keyword">let</span></span>.

The thing is, none of these individually are doing anything wrong. All three scripts are doing their job, but interacting badly. I can't imagine a single case where the end result is desirable. This is not specific to Prism Live (which is an unreleased buggy script with little usage), but can occur with anything that includes their own Prism as a dependency and Prism is also included by something else or directly.

Should Prism throw away existing highlighting if present?
Should the Keep Markup plugin do it?
Should there be some sort of opt-in?

@RunDevelopment
Copy link
Member

Autoloader will have the same problem with Keep Markup because Autoloader re-highlights.

Maybe make so Keep Markup ignores all span.token? But people are actively using this to keep manually writing highlighting for unsupported languages/server-side rendering, so that's a no-go.

Make it so that Keep Markup only keeps tags with a certain class/attribute/tag? This is opt-in of course to keep backward compatibility.

@LeaVerou
Copy link
Member Author

Perhaps a drop-tokens class or something for Keep Markup? That seems to be the path with the least backwards compat issues.

@LeaVerou
Copy link
Member Author

Although, ideally we should address the problem itself: the nested tokens. That's not desirable for anyone, right?

@mAAdhaTTah
Copy link
Member

Can Keep Markup check if it's maintaining the same node as what was added and not readd it if it's the same? If that makes sense (I'm not familiar with Keep Markup's internals).

@RunDevelopment
Copy link
Member

Nested tokens aren't an issue in itself because users might want to keep deeply nested tokens (just not the span.token ones (usually)).

Checking whether the same node is added will be hard... If you can implement, ok but I'm with the drop-tokens one. Seems easier. But maybe name it so that it's associated with Keep Markup? keep-markup-drop-tokens?

@LeaVerou
Copy link
Member Author

In general nested tokens are desirable and necessary for some grammars to work, definitely! I was referring to this particular kind of nesting where everything is duplicated.

Before we go about adding a flag (which would require serious debugging by the end user to even understand the problem and why the flag is needed), are we sure there's no way to detect this and nip it in the bud without affecting legit use cases?

I see @Golmote originally wrote Keep Markup, @Golmote do you have any thoughts?

@Golmote
Copy link
Contributor

Golmote commented May 16, 2020

Hi!

Can Keep Markup check if it's maintaining the same node as what was added and not readd it if it's the same? If that makes sense (I'm not familiar with Keep Markup's internals).

The Keep Markup plugin currently uses the Range API. I remember I went for this cause it handles the weird cases automatically, like when the markup you wanna keep lies in the middle of future-highlighted tokens:
https://i.imgur.com/XiwEAA2.png
I can see in the source code that the nodes are cloned, not reused. I don't remember the reason, but I assume I wouldn't have cloned them if I did not need to. So it does not seem we currently have a way to keep a reference to the original node.

That being said, it's certainly possible to rewrite the plugin using a more precise but more tedious approach, where we grab the original nodes we wanna keep, detach them somehow, highlight the code and then reappend those same nodes. It might even help solving issues like #1618 and #1640. But that's a lot of work, cause we'd have to handle the weird cases ourselves.

With a reference to the original nodes, I guess we could hook into before-highlight only once per code block and then always reuse the stored data in after-highlight.
Now, that might raise new issues: what if the user wants to update the whole content of the code block with new markup and rehighlight it manually? How would the user tell the plugin to reanalyse the original nodes? Maybe the plugin would need an API to allow for this case.

Perhaps a drop-tokens class or something for Keep Markup?

This could be done with the current way the plugin is coded, I believe. The flag could make it possible to filter the nodes in the before-highlight hook and ignore them.

But as you mentionned, it might be hard to grasp for the end user.

are we sure there's no way to detect this and nip it in the bud without affecting legit use cases?

The only idea I can think of would be to give Prism a way to undo its highlighting and restore the original content. That way, it could always undo highlighting before rehighlighting a code block. But I can't wrap my head around how that would behave with the many different plugins and use cases. I'm actually not even sure it would work if Prism is included twice.

@galaxy4public
Copy link

It would be a bit of a crazy idea (since it will touch everything that PrismJS is), but have you considered leveraging Shadow DOM for presenting the highlighted result? I think something wonderful can be extracted from that, e.g. it kind of addresses @Golmote's question re: "undoing the highlighting" -- if we do the highlighting in the Shadow DOM we can always throw it away, create a copy, or get the original content as it was before we started our work.

This is once again, just a crazy idea if you did not consider this feature in the past.

@RunDevelopment
Copy link
Member

Shadow DOM has poor browser support (see first note). Also, I'm frankly quite glad that we didn't open that Pandora's box yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants