aeharding / voyager

Voyager — a mobile-first Lemmy client
https://vger.app
GNU Affero General Public License v3.0
1.49k stars 166 forks source link

Implement spoiler tag support #9

Closed aeharding closed 7 months ago

aeharding commented 1 year ago
:::spoiler

(stuff here)

:::

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

devzzm commented 1 year ago

Seconding this

IMG_3550

ZarK commented 1 year ago

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.

jachan commented 1 year ago

Hey Alex,

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

aeharding commented 1 year ago

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

80avin commented 1 year ago

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.

boozook commented 11 months ago

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


:::spoiler title

(stuff here)

:::
DakshG07 commented 11 months ago

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

aeharding commented 11 months ago

Okay, thanks @DakshG07!

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

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.
80avin commented 11 months ago

@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.

aeharding commented 11 months ago

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

DakshG07 commented 11 months ago

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 commented 11 months ago

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.

aeharding commented 11 months ago

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>

DakshG07 commented 11 months ago

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.

80avin commented 11 months ago

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

aeharding commented 11 months ago

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

DakshG07 commented 11 months ago

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.

80avin commented 11 months ago

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 ?
DakshG07 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

80avin commented 11 months ago

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 )
DakshG07 commented 11 months ago
  1. Add an onclick listener to details (or summary ?) which only does event.stopPropagation()

@80avin this worked. Thanks!

LazaroFilm commented 9 months ago

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

aeharding commented 9 months ago

@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.

DakshG07 commented 9 months ago

@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.

aeharding commented 8 months ago

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 commented 7 months ago

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!