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

Spec spoilers and color attribute allowance #2549

Closed
wants to merge 3 commits into from

Conversation

turt2live
Copy link
Member

Spoilers MSC: #2010
color attribute MSC: #2422

Spoilers MSC: #2010
`color` attribute MSC: #2422
@turt2live turt2live requested a review from a team May 15, 2020 20:41
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should relax the terms a bit in favour of basic clients. Otherwise LGTM.

specification/modules/instant_messaging.rst Outdated Show resolved Hide resolved
specification/modules/instant_messaging.rst Outdated Show resolved Hide resolved
Comment on lines +541 to +543
The plain text (``body``) fallback for spoilers is a little different than the HTML-formatted
message as the ``body`` is often included as-is in notifications to the user. To prevent spoilers
in notifications and other places, clients are strongly encouraged to first upload the spoiler
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The plain text (``body``) fallback for spoilers is a little different than the HTML-formatted
message as the ``body`` is often included as-is in notifications to the user. To prevent spoilers
in notifications and other places, clients are strongly encouraged to first upload the spoiler
When sending a spoiler, clients SHOULD provide the plain text fallback in ``body``.
The fallback SHOULD omit the spoiler text verbatim (but include the reason), since ``body``
contents can be used as-is in basic clients, notifications to the user etc. To prevent spoilers
showing up in such situations, clients are strongly encouraged to first upload the spoiler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MSC does not make the fallback a SHOULD, anywhere (or I'm failing to see it). The MSC only ever references it as "the plaintext fallback" in a context of SHOULD upload the text, implying that it is required (in terms of language on the page).

To be clear, this concern is about the first SHOULD - the second SHOULD with respect to uploading the files is fine.

I'm happy to block this PR behind another MSC to clarify the intent, as this seems like it could be an opportunity for extended discussion which doesn't belong here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree the original MSC is not perfectly clear on the modality of the fallback (whether it's SHOULD or MUST).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soru intended to word it as a recommendation, however if it is ambiguous it seems like further discussion might be appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-authored-by: Kitsune Ral <Kitsune-Ral@users.sf.net>
@@ -499,6 +499,54 @@ the output looks similar to the following::
For ``m.image``, the text should be ``"sent an image."``. For ``m.video``, the text
should be ``"sent a video."``. For ``m.audio``, the text should be ``"sent an audio file"``.

Spoiler messages
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Mention that spoilers are only possible on m.room.message (specifically m.text) events.

If another MSC does show up, it'd be good to expand this to textual types in general (emotes, notices, etc).

@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label May 19, 2020
@turt2live
Copy link
Member Author

this is unblocked now, but needs to be reviewed for accuracy before going back to general spec review.

@uhoreg
Copy link
Member

uhoreg commented Mar 29, 2021

also needs to be ported to the markdown version

@turt2live turt2live marked this pull request as draft March 29, 2021 17:45
@turt2live turt2live self-assigned this Mar 29, 2021
turt2live added a commit that referenced this pull request Apr 6, 2021
@turt2live
Copy link
Member Author

Replaced by #3098

@turt2live turt2live closed this Apr 6, 2021
@turt2live turt2live deleted the travis/spec/2010-2422-spoilers-and-color branch April 6, 2021 02:09
richvdh pushed a commit that referenced this pull request Aug 23, 2021
richvdh pushed a commit that referenced this pull request Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something needs to be done before action can be taken on this PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants