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

Implement spoiler tag support #9

Closed
aeharding opened this issue Jun 25, 2023 · 29 comments · Fixed by #1270
Closed

Implement spoiler tag support #9

aeharding opened this issue Jun 25, 2023 · 29 comments · Fixed by #1270
Assignees
Labels
enhancement New feature or request parity:lemmy

Comments

@aeharding
Copy link
Owner

:::spoiler

(stuff here)

:::

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

@aeharding aeharding added enhancement New feature or request parity:lemmy and removed parity:lemmy labels Jun 25, 2023
@phareous
Copy link

phareous commented Jul 9, 2023

Seconding this

IMG_3550

@ZarK
Copy link

ZarK commented Jul 12, 2023

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
Copy link

jachan commented Jul 15, 2023

Hey Alex,

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

@aeharding
Copy link
Owner Author

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

@aeharding aeharding added the help wanted Extra attention is needed label Jul 15, 2023
@80avin
Copy link
Contributor

80avin commented Aug 21, 2023

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: LemmyNet/lemmy-ui#687

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

@boozook
Copy link

boozook commented Oct 28, 2023

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


:::spoiler title

(stuff here)

:::

@DakshG07
Copy link
Contributor

DakshG07 commented Nov 7, 2023

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

@aeharding
Copy link
Owner Author

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
Copy link
Contributor

80avin commented Nov 7, 2023

@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
Copy link
Owner Author

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

@DakshG07
Copy link
Contributor

DakshG07 commented Nov 7, 2023

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
Copy link
Contributor

DakshG07 commented Nov 8, 2023

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
Copy link
Owner Author

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
Copy link
Contributor

DakshG07 commented Nov 9, 2023

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
Copy link
Contributor

80avin commented Nov 9, 2023

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

@aeharding
Copy link
Owner Author

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
Copy link
Contributor

DakshG07 commented Nov 9, 2023

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
Copy link
Contributor

80avin commented Nov 9, 2023

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
Copy link
Contributor

DakshG07 commented Nov 9, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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 😅

@DakshG07
Copy link
Contributor

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

@80avin
Copy link
Contributor

80avin commented Nov 11, 2023

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
Copy link
Contributor

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

@80avin this worked. Thanks!

@LazaroFilm
Copy link

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

@aeharding
Copy link
Owner Author

@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
Copy link
Contributor

DakshG07 commented Jan 21, 2024

@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
Copy link
Owner Author

aeharding commented Feb 17, 2024

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
Copy link
Owner Author

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.

aeharding/micromark-extension-lemmy-spoiler#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 aeharding assigned aeharding and unassigned 80avin and DakshG07 Feb 29, 2024
@aeharding aeharding removed the help wanted Extra attention is needed label Mar 1, 2024
aeharding added a commit that referenced this issue Mar 2, 2024
Resolves #9

Co-authored-by: Dukk <acedaksh07@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request parity:lemmy
Projects
None yet
8 participants