voyager icon indicating copy to clipboard operation
voyager copied to clipboard

Implement spoiler tag support

Open aeharding opened this issue 2 years ago • 1 comments

:::spoiler

(stuff here)

:::

is supported by lemmy. It appears to be custom spoiler markup? So might need to implement our own markdown plugin. :/

aeharding avatar Jun 25 '23 05:06 aeharding

Seconding this

IMG_3550

devzzm avatar Jul 09 '23 19:07 devzzm

Second this, we’re now starting to get some active movies and TV show discussion communities, and it would be very nice to not see the content of the spoilers when using Voyager.

ZarK avatar Jul 12 '23 12:07 ZarK

Hey Alex,

Would love to take this and try and implement spoiler tag support, if no one else is working on it.

jachan avatar Jul 15 '23 00:07 jachan

I would appreciate that. mdast parsing is not my forte.

aeharding avatar Jul 15 '23 01:07 aeharding

I am looking to work on this, but looks like there's no defined format of how to define spoilers in markdown.

Similar discussion is going on here: https://github.com/LemmyNet/lemmy-ui/issues/687

I think we can support reddit's spoiler ( >!spoiler!< or >!spoiler line ) and lemmy's spoiler both.

80avin avatar Aug 21 '23 20:08 80avin

I have to clarify the format of the spoiler text given by the topic starter.


:::spoiler title

(stuff here)

:::

boozook avatar Oct 28 '23 23:10 boozook

Don't see anyone actively working on this, so I'll try to take a stab at it.

DakshG07 avatar Nov 07 '23 19:11 DakshG07

Okay, thanks @DakshG07!

If it helps, my investigations in the past led me to the following info:

  • Lemmy uses https://github.com/markdown-it/markdown-it-container to parse spoilers.
  • No perfect alternative for remark.js exists. There is this but its outdated. There's also remark-directive but it only accepts a custom format slightly different than lemmy's format and is inflexbile.

Which leads me to the thought, unless I'm missing anything, that either

  1. Lemmy should change their spoiler formatting to be something more compatible between various markdown parsers, or
  2. we need to build our own remark.js lemmy spoiler plugin.

aeharding avatar Nov 07 '23 20:11 aeharding

@DakshG07 You're welcome.

I have started on this few weeks ago, but learning about unifiedjs, remark, mdast, micromark, etc to make a spoiler plugin is taking more time than I can dedicate.

Then, since it will be a set of plugins for remark, mdast, micromark, I thought of creating a separate package, which again is another rabbit hole, so more time.

Anyways, it is being a good learning experience and I might require 2-3 weekends. But if you want to try your hands, I'm happy to concede.

80avin avatar Nov 07 '23 20:11 80avin

@80avin @DakshG07 I've assigned the two of you, feel free to collaborate and correspond in this issue!

aeharding avatar Nov 07 '23 20:11 aeharding

After some initial work, yeah, this is a lot more difficult than it really should be. In my mind there's a little "hack" we could use to avoid all the extra work of creating a minimark parser.

Firstly, we'd create a remark plugin to go down the AST and search for texts with spoiler tags. If the spoiler tags open and close in the same text node, then we're golden: just split the node into a text and a spoiler node with the appropriate child node. Otherwise, we'll have to find the text node containing the closing spoiler tag, and then set all the inbetween nodes as the children of the new spoiler node. This will allow us to properly handle markdown inside of a spoiler tag.

Secondly, we'll have to add a component to render in the spoilers themselves. The was the Lemmy client handles this (and the way I would go about it too) is through a <details><summary>{title}</summary>{content}</details> setup.

This requires lots of playing around with the AST, and I'll have to see if it's even possible. I also have no idea the performance implications of this: I'll have to get around to making a working prototype first.

DakshG07 avatar Nov 07 '23 23:11 DakshG07

Oh, and additionally: the spoiler component would also have to implement a click handler similar to LinkInterceptor, as currently clicking on it collapses the comment.

DakshG07 avatar Nov 08 '23 00:11 DakshG07

Oh, and additionally: the spoiler component would also have to implement a click handler similar to LinkInterceptor, as currently clicking on it collapses the comment.

Yeah. I'm hoping though that this won't be too difficult - hopefully just adding summary to components prop of <ReactMarkdown>

aeharding avatar Nov 08 '23 00:11 aeharding

Out of curiosity, where do you go to write your test posts? I want to do some testing with the spoilers, but I also don't want to create unnecessary spam.

DakshG07 avatar Nov 09 '23 16:11 DakshG07

@DakshG07 we can preview the comment before sending. That's what I use.

80avin avatar Nov 09 '23 17:11 80avin

Out of curiosity, where do you go to write your test posts? I want to do some testing with the spoilers, but I also don't want to create unnecessary spam.

https://voyager.lemmy.ml

No connection to the Voyager app, it's a staging environment for Lemmy named re: Star Trek

aeharding avatar Nov 09 '23 17:11 aeharding

I think I've discovered something which makes everything even harder.

::: spoiler Nested Spoiler
This is a nested spoiler
::: spoiler 1
How's
::: spoiler 2
The
::: spoiler 3
Weather
::: spoiler 4
Up
::: spoiler 5
There?
:::

The above is a nested spoiler. It works fine, but it has a really weird behaviour where all the spoiler tags must be closed with the same tag. Of course, nested spoilers aren't probably used very often and therefore won't be a main focus of mine, but this is some very weird behaviour nonetheless.

DakshG07 avatar Nov 09 '23 19:11 DakshG07

Looks great. Can you check these concerns as well ? (I was trying your simplistic approach initially, then these concerns forced me to do it the more formal way)

  1. Whether the spoiler markdown is replaced in code blocks or not ? It shouldn't be.
  2. Whether the markdown inside spoiler tags is rendered properly or not ?
  3. What if there's a code block containing a spoiler inside the spoiler ?

80avin avatar Nov 09 '23 20:11 80avin

1 & 3 can be addressed by checking the type of the node. I only look at text nodes when checking for spoilers: this comes with the side effect that any spoiler syntax that isn't plain text won't be rendered (this is, however, how Lemmy does it too).

As for 2, rendering markdown inside spoilers is something that will be a bit difficult. My workaround for this by splitting up the plugin into two parts: a remark plugin that splits the start and ends of spoilers into their own seperate nodes, and a rehype plugin that will take the content in between these nodes and set them as children. Then, the rehype plugin sets the parent nodes for the spoilers as a link, and sets the href to :::spoiler:::, which I'll then check for in LinkInterceptor to convert the link into a <details> tag. The extra LinkInterceptor dance is needed to override the onclick so that pressing on the details doesn't collapse the comment.

Currently, I've finished the remark plugin, and am now starting work on the rehype plugin.

DakshG07 avatar Nov 09 '23 20:11 DakshG07

image

Major success! The above screenshot shows spoilers being identified and displayed as links. However, these href property of these links is :::spoiler:::, and the title property is the title of the spoiler. This allows me to use the LinkInterceptor to render any links leading to :::spoiler::: correctly.

How it works

I've written a remark plugin to take any :::spoiler and ::: tags and seperate them from the AST so they can be processed by rehype. The topmost nodes of the AST are paragraphs, and since we want the spoilers to contain those nodes, we need to "split open" the paragraphs with these tags and seperate the spoiler tags. For example, a paragraph as such:

Text before spoiler.
::: spoiler Big secret
It's my birthday!
OK, you caught me, it isn't my birthday.
:::
Text after spoiler.

Would become:

Text before spoiler.
::: spoiler Big secret
It's my birthday!
OK, you caught me, it isn't my birthday.
:::
Text after spoiler.

Then, using the rehype plugin, we take note of the indexes of the paragraphs which contain the "tokens". We save these to a list and then go down each pair, using the start and end to splice the list and set the elements in between the start and end as the children of the new link element. The link element gets its title and href properties set, and we're good to go!

DakshG07 avatar Nov 11 '23 00:11 DakshG07

Working spoilers rendered with <details> tags.

Spoilers Opened Spoiler Opened

Spoilers Closed Spoiler Closed

Not very experienced with React so unsure how to prevent the comment from collapsing when I click on them :sweat_smile:

DakshG07 avatar Nov 11 '23 18:11 DakshG07

Still fairly buggy and unfinished, but will open a PR for it now.

DakshG07 avatar Nov 11 '23 18:11 DakshG07

Does this help ? https://developer.mozilla.org/en-US/docs/Web/API/Event/stopPropagation

I think you can try two ways

  1. Add an onclick listener to details (or summary ?) which only does event.stopPropagation()
  2. In comment's onclick listener, do nothing if click is coming from some child ( event.currentTarget != event.target )

80avin avatar Nov 11 '23 18:11 80avin

  1. Add an onclick listener to details (or summary ?) which only does event.stopPropagation()

@80avin this worked. Thanks!

DakshG07 avatar Nov 11 '23 18:11 DakshG07

Any updates on this? It's a pretty significant omission for certain subs that heavily use spoilers.

LazaroFilm avatar Jan 21 '24 17:01 LazaroFilm

@LazaroFilm, it boils down to building a plugin that adds support for spoilers via https://github.com/remarkjs/react-markdown.

Unfortunately Lemmy's spoiler format is nonstandard markdown. So, this is no easy task. @DakshG07 has a nice start, but it needs more testing and perf improvements before it could be integrated.

More info here: https://github.com/remarkjs/remark/blob/main/doc/plugins.md#create-plugins

The other alternative is that we try to advocate Lemmy to switch to a more standard spoiler format, like HTML's <summary> <detail>.


Lastly, to be completely honest, and as a rant, this isn't Lemmy's fault. This is the failure of markdown standards, including commonmark and GFM, adopting a spoiler format. https://talk.commonmark.org/t/what-could-a-spoiler-tag-extension-look-like/767?page=2

This Voyager issue is essentially a manifestation of a decade+ of neglect in any attempt at standardizing markdown spoilers.

aeharding avatar Jan 21 '24 17:01 aeharding

@LazaroFilm There’s this PR and this repository. Main issue is that this combs through the entire tree checking with regex, which at least on paper doesn’t sound too good for performance. Also needs more testing.

I’m working on some faster algorithms for this, but this will obviously take some time. The preferred approach would be to extend the mdast, but this is both incredibly complicated and poorly documented. Really, I’d have preferred if Lemmy used a more standard spoiler syntax: but there really isn’t a standard syntax. Hopefully I can get the plugin to a state soon where I feel it’s ready to be added to Voyager, as I’m not looking to rush and implement the plugin in a half-baked state.

Edit: Seems like @aeharding hits the same points 😅. Anyways, I wouldn’t hold my breath for spoilers—it seems it’ll be a while before they’re implemented in Voyager.

DakshG07 avatar Jan 21 '24 17:01 DakshG07

I looked into this a bit more this weekend.

I think we need to go lower level and make a micromark extension, instead of a remark plugin. Then we don't have to fight against the tree, which is really not what we should be doing. Micromark extensions step through each individual character and matches.

I made a micromark extension that parses :::spoiler but then I got stuck. The micromark architecture is pretty overwhelming.

Forking remark-extension-directive is an ok place to start, but it's pretty confusing because the extension is very abstract.

https://github.com/micromark/micromark-extension-directive

But testing is quite straightforward. We essentially need a micromark extension that has a test case for the following:

:::spoiler Test Header\nTest content!\n::: -> <details><summary>Test Header</summary>Test Content!</details>


Edit: you can see my very modest start of this here: https://github.com/aeharding/micromark-extension-lemmy-spoiler

Basically not doing much except forking micromark-directive, pulling out everything except container directives, hardcoding spoiler, and allowing a space between ::: and spoiler

aeharding avatar Feb 17 '24 21:02 aeharding

Another update, I am currently continuing work on the micromark extension. I made a few attempts at forking learning a bit each time, and the most recent fork is the most promising.

https://github.com/aeharding/micromark-extension-lemmy-spoiler/pull/1

The micromark plugin, unfortunately, isn't all that's needed. After this is done, I still need to implement a fork of mdast-util-directive.

Due to the progress I'm making on this, I am reassigning to myself. Thanks again for everyones help up to this point!

aeharding avatar Feb 29 '24 03:02 aeharding