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

MSC2010: Add client-side spoilers #2010

Open
wants to merge 11 commits into
base: master
from

Conversation

@Sorunome
Copy link

commented May 22, 2019

Rendered

@Half-Shot
Copy link
Contributor

left a comment

Cool stuff, added a few things I'd like to see explained.

Show resolved Hide resolved proposals/2010-spoilers.md Outdated
Show resolved Hide resolved proposals/2010-spoilers.md Outdated
Show resolved Hide resolved proposals/2010-spoilers.md Outdated
Show resolved Hide resolved proposals/2010-spoilers.md Outdated

Sorunome added some commits May 22, 2019

@Sorunome Sorunome changed the title MSC 2010: Add client-side spoilers MSC2010: Add client-side spoilers May 22, 2019

@turt2live

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@Sorunome please wrap lines at around 100 characters to make line-by-line review a bit easier, and resolve the threads which no longer apply

@Sorunome Sorunome referenced this pull request Jun 17, 2019

Merged

handle spoilers #502

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Looks pretty straight-forward to me, and essential for bridging other networks which include spoiler functionality, such as Discord. Thanks!

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Team member @anoadragon453 has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@turt2live
Copy link
Member

left a comment

lgtm! It sucks that we can't use extensible events for this yet :(

@uhoreg

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

related to #604 and vector-im/riot-web#5550

@uhoreg
Copy link
Member

left a comment

Mostly looks good.

Show resolved Hide resolved proposals/2010-spoilers.md Outdated
Show resolved Hide resolved proposals/2010-spoilers.md Outdated

Sorunome added some commits Jun 25, 2019

@uhoreg

uhoreg approved these changes Jul 15, 2019

Show resolved Hide resolved proposals/2010-spoilers.md Outdated
Show resolved Hide resolved proposals/2010-spoilers.md Outdated
@dbkr
Copy link
Member

left a comment

I think this seems like the best way of doing this - looks like there's a whole collection of different ways that people have done this for various forums etc (eg. pretending the <spoiler> tag is a thing or a blockquote with a custom class) but afaics there's no obvious prevailing one, so I think this is as good as any.

A comment on why the 'details' HTML element was rejected would be nice though (presumably we wanted something inline?)

Show resolved Hide resolved proposals/2010-spoilers.md Outdated
@dbkr

dbkr approved these changes Jul 16, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

I had concerns about the media repo integration, but thought it truly is the only way to actually hide content in a very basic client, and it is backwards compatible. So all good from me.

@KitsuneRal
Copy link
Member

left a comment

So far it looks as a bit too heavy solution for the given problem. Also, since events are immutable, did you think of using another attribute or (custom) event instead of HTML to hold the spoiler(s)?


To preserve the semantics of a spoiler in the plaintext fallback it is recommended to upload the contents of the spoiler
as a text file and then link this: `[Spoiler](mxc://someserver/somefile)` and
`[Spoiler for reason](mxc://someserver/somefile)` respectively.

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Jul 17, 2019

Member
  1. As a client author, I'm not extremely happy to use two separate mechanisms for the purpose that can be covered by one uniform solution. Can we use only the text file and cover both the formatted and the unformatted case with that (maybe different files if a markup is involved inside the spoiler)?
  2. In the provided solution, you actually use markdown inside the plain body, and people has been complaining at least in #quotient pretty consistently that markdown becomes a sort of admitted markup inside the plain body. Besides, the word "Spoiler" will clash with localisation efforts: think of a Riot user who uses it in Russian but speaks English, then his readers will see "Спойлер" (or whichever translation chosen) in the same unfortunate way we now see translations of "In reply to", effectively leaking information about the author's locale. So in the absence of reason, can we just keep the link, and only use markdown to decorate the reason when it exists?

This comment has been minimized.

Copy link
@turt2live

turt2live Jul 18, 2019

Member

"In reply to" being translated is a bug on the client-side fwiw. I do agree that we shouldn't be using Markdown here though, and definitely should not continue the trend of English-only strings in the spec.

This comment has been minimized.

Copy link
@Sorunome

Sorunome Jul 18, 2019

Author
  1. the problem with using the text file in formatted body is that it doesn't render / look nearly as nicely. The problem with having the spoilered text in the plain body is that it destroys the semantics of the spoiler, which is why this solution was chosen

  2. AFAIK there is no markdown for spoiler, do you have any suggestion? Ideally the client will support the formatted body anyways, yielding the fallback unneeded

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Jul 18, 2019

Member

On (2), I meant the Markdown notation of using square brackets for the text, followed by URL in parentheses, to decorate the reason as a hyperlink.

I understand it's a lost battle to put something with semantics of hidden text into plain text. Still, we haven't specced Markdown as a standard lightweight markup for the plain body so I can't just let this in. Which is why I proposed the idea of an external reference to bytes in the plain body (which, admittedly, is heavier and less compatible than throwing in a bunch of brackets but on the backdrop of having to upload and refer to a file looks pretty minor).

Or what if we don't spec the fall-back for reason in the plain body, saying that full compat with minimal/old clients is not feasible? Basically, the spec would mandate using a URL to the file with the spoiler but explicitly refrain from defining the way clients are supposed to put a reason next to it? Then, if some client would prefer to use, say, the Slack style (<http://example.com|Text>) for the purpose it would still be just fine.

that doesn't take a spoiler reason into account.

## Security considerations
The spoiler reason needs to be properly escaped when rendered.

This comment has been minimized.

Copy link
@KitsuneRal

KitsuneRal Jul 17, 2019

Member

Separately for HTML and Markdown, moreover :-\

This comment has been minimized.

Copy link
@Sorunome

Sorunome Jul 18, 2019

Author

As the spoiler itself can contain formatting, not mroe escaping than you need to do already is required. In the formatted body cleartext segments are html escaped, and if someone uses markdown syntax in the body you can escape that one, if wanted. No new addition here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.