Details

Reviewers

atul
ginsu
ashoat

Commits

rCOMM5a8aad609b04: [lib] Prevent spoiler text from appearing in MessagePreview

Summary

With the implementation of Spoilers on both web and native, we want to prevent the spoiler text from being revealed in MessagePreview on both platforms. This diff handles replacing the text enclosed with spoiler characters with three black squares, as discussed in the parent tasks for Spoilers, ENG-1472. We previously thought about replacing the spoilers with black square emojis.

The idea with this diff is to "intercept" the raw message text before it is parsed for markdown rules, so we can replace the text enclosed within || ... || before the spoiler characters are removed for the message preview.

Context Diff

Addresses ENG-2193
Addresses ENG-2055

Test Plan

[Web]:

Restart keyserver and web
Observe how spoilers are displayed in the message preview before the change
Observe after the change
Ensure other markdown functions as before

[Native]:

Restart keyserver and native
Observe how spoilers are displayed in the message preview before the change
Observe after the change
Ensure other markdown functions as before
Check how the change looks in both light and dark mode

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D5715

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Nov 23 2022, 5:19 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptNov 23 2022, 5:19 PM

rohan edited the summary of this revision. (Show Details)Nov 23 2022, 5:20 PM

rohan edited the test plan for this revision. (Show Details)

rohan attached a referenced file: F259298: Native Dark Before.png. (Show Details)

rohan attached a referenced file: F259299: Native Dark After.png. (Show Details)

rohan attached a referenced file: F259300: Native Light After.png. (Show Details)

rohan attached a referenced file: F259301: Native Light Before.png. (Show Details)

rohan attached a referenced file: F259296: Web After.png. (Show Details)

rohan attached a referenced file: F259297: Web Before.png. (Show Details)

Harbormaster completed remote builds in B13716: Diff 18796.Nov 23 2022, 5:33 PM

rohan requested review of this revision.Nov 23 2022, 5:33 PM

Extract logic to a function (both to make it easier to change and also in case this needs to be used in notifications)

Harbormaster completed remote builds in B13729: Diff 18812.Nov 24 2022, 4:05 AM

rohan edited the summary of this revision. (Show Details)Nov 24 2022, 4:12 AM

ashoat added inline comments.Nov 24 2022, 9:16 AM

lib/shared/messages/text-message-spec.js
87 ↗	(On Diff #18812)	We're already getting the `ast` here... it seems like it would be cleaner and more performant to recurse down the AST and replace the `content` of the `text` node inside the `spoiler` node with `⬛⬛⬛`

rohan added inline comments.Nov 26 2022, 7:54 AM

lib/shared/messages/text-message-spec.js
87 ↗	(On Diff #18812)	Yeah, that's also another option to traverse the AST - I was avoiding using recursion initially but it makes sense to use what we already have. I'll put up a revision for that

Recurse down the AST instead (tested web and native)

rohan edited the summary of this revision. (Show Details)Nov 26 2022, 8:03 AM

Harbormaster completed remote builds in B13762: Diff 18847.Nov 26 2022, 8:14 AM

ashoat requested changes to this revision.Nov 27 2022, 9:10 AM

ashoat added inline comments.

lib/shared/markdown.js
276 ↗	(On Diff #18847)	I think you need to consider a few other cases. Check out this code, and try to come up a test case of Markdown text that will produce something that needs that code to work

This revision now requires changes to proceed.Nov 27 2022, 9:10 AM

rohan added inline comments.Nov 29 2022, 9:43 AM

lib/shared/markdown.js
276 ↗	(On Diff #18847)	Ah, I see a case where things like a `List` markdown isn't going to be handled properly here - I can borrow some logic from `rawTextFromMarkdownAST` and modify it for this

Borrow logic from rawTextFromMarkdownAST to account for List Markdown types

Screenshot 2022-11-29 at 12.40.16 PM.png (1×2 px, 265 KB)

rohan attached a referenced file: F265099: Screenshot 2022-11-29 at 12.40.16 PM.png. (Show Details)Nov 29 2022, 9:53 AM

Harbormaster completed remote builds in B13849: Diff 18970.Nov 29 2022, 10:07 AM

Is it possible to get this looking more simple, eg. one simple function (instead of three) like rawTextFromMarkdownAST or markdownASTHasPressable in D5755? If not, can you explain what makes this case more complicated?

lib/shared/markdown.js
256 ↗	(On Diff #18970)	This is a bad name for the function that gets exported... you should clarify what it does, eg. `stripSoilersFromMarkdownAST`

This revision now requires changes to proceed.Nov 29 2022, 3:47 PM

Simplify the functions more

To address your question @ashoat with the new revision:

I think it makes most sense to have two functions:

stripSpoilersFromMarkdownAST: Called in text-message-spec, and handles returning the actual AST. Within this, we map over each of the nodes in the AST and recurse down the the content by calling replaceSpoilersFromMarkdownAST.

replaceSpoilersFromMarkdownAST: This returns a string, whether it be the original items/content, or the replaced spoiler text. I simplified this a bit and it should resemble rawTextFromMarkdownAST

I think the difference between this and rawTextFromMarkdownAST is that here, we need to keep recursively re-building the trees as we traverse, while in rawTextFromMarkdownAST, we just traverse and return a string.

Harbormaster completed remote builds in B13933: Diff 19091.Dec 1 2022, 8:42 PM

It seems this approach can leak details from inside the spoiler, eg. if there is an underline it will result in ⬛⬛⬛ being underlined. Let me know if I'm missing something, but it's also my suspicion that this is the source of much of the complexity.

Instead of trying to recurse down and replace all of the nested text with ⬛⬛⬛ once you realize we're in a spoiler, instead I think you should just replace the whole spoiler node wholesale (including all of its children) with a text node that includes just ⬛⬛⬛. This should obviate the need for your isTextASpoiler parameter, and should hopefully allow you to simplify the function further. In an ideal world, you can get everything down to just one function (I'm not yet sure if that's possible).

Let me know if that makes sense or if I'm off-base!

This revision now requires changes to proceed.Dec 2 2022, 7:27 AM

In D5715#173103, @ashoat wrote:

It seems this approach can leak details from inside the spoiler, eg. if there is an underline it will result in ⬛⬛⬛ being underlined. Let me know if I'm missing something, but it's also my suspicion that this is the source of much of the complexity.

Instead of trying to recurse down and replace all of the nested text with ⬛⬛⬛ once you realize we're in a spoiler, instead I think you should just replace the whole spoiler node wholesale (including all of its children) with a text node that includes just ⬛⬛⬛. This should obviate the need for your isTextASpoiler parameter, and should hopefully allow you to simplify the function further. In an ideal world, you can get everything down to just one function (I'm not yet sure if that's possible).

Let me know if that makes sense or if I'm off-base!

I'm happy to look into changing the implementation, though to first to address your point about the details being leaked, I believe that is already handled from your original comment, leading to this code. So already, message previews strip markdown from being visible - it's the same for spoilers, the || characters are removed. So that means when we have something like ||**Bold Text**|| or ||__Underlined Text__||, all that's visible is the Bold Text or Underlined Text. Now, with this diff we want to replace that content with the spoilerReplacement since it's a 'spoiler'. Let me know if that makes sense!

Screenshot 2022-12-02 at 12.24.49 PM.png (1×2 px, 191 KB)

rohan attached a referenced file: F269625: Screenshot 2022-12-02 at 12.24.49 PM.png. (Show Details)Dec 2 2022, 9:25 AM

It's not just about stripping those parts, it's also about simplifying. You're making the code more complicated than it needs to be by trying to preserve the structure beneath the spoiler. We don't need to preserve any of that – as mentioned above, we can replace wholesale. Let me know if that makes sense

In D5715#173130, @ashoat wrote:

It's not just about stripping those parts, it's also about simplifying. You're making the code more complicated than it needs to be by trying to preserve the structure beneath the spoiler. We don't need to preserve any of that – as mentioned above, we can replace wholesale. Let me know if that makes sense

Makes sense! If we're talking about simplifying the logic, would you prefer we just add a check for if (type === 'spoiler') --> return spoilerReplacement in rawTextFromMarkdownAST?

Hmmm... interesting idea, but I think it's probably best to keep it separate... I worry that that behavior might be non-obvious from the name of the function

Condense logic into one function, and just replace entire spoiler nodes
with text nodes. Return a ast

rohan added inline comments.Dec 2 2022, 12:43 PM

lib/shared/markdown.js
262 ↗	(On Diff #19113)	Returning an Array since we're returning an `ast`. This is the reason I used `return ast.map(node => ...)`

Much better!! One question inline

lib/shared/markdown.js
263 ↗	(On Diff #19113)	Can we avoid nesting everything in a massive `return ast.map` like this? Can you just recurse if it's an array, and then handle the single case separately from the array case?

This revision now requires changes to proceed.Dec 2 2022, 12:51 PM

Harbormaster completed remote builds in B13947: Diff 19113.Dec 2 2022, 12:53 PM

rohan added inline comments.Dec 2 2022, 1:04 PM

lib/shared/markdown.js
263 ↗	(On Diff #19113)	I'm nesting it within a `ast.map` since we want the array of nodes to be returned (since the ast is an array of nodes). If we remove that, wouldn't it make returning an array through recursion harder?

ashoat added inline comments.Dec 2 2022, 1:27 PM

lib/shared/markdown.js
263 ↗	(On Diff #19113)	Not sure I follow – you can check `if (Array.isArray` and then call the `map` to recurse into the same function, passing in a single node. And then you can handle the base case of the single node in the same function. Isn't this how `rawTextFromMarkdownAST` and `markdownASTHasPressable` work?

I could be off base here, but after looking into your feedback here's what I've come up with

lib/shared/markdown.js
263 ↗	(On Diff #19113)	From my understanding, the key differences between `rawTextFromMarkdownAST` and `stripSpoilersFromMarkdownAST` are: `rawTextFromMarkdownAST`: Is called on one singular node, so the accepted parameter is type `ASTNode` The return value is a `string`, so we don't need to preserve the `AST` structure, we just need to recurse as deep as needed for us to get a `text` node, where the check `if (content && typeof content === 'string')` will be looking if the `content` is a string (ie text) at the point. `stripSpoilersFromMarkdownAST`: Since we're just trying to traverse the AST and replace the `spoiler` content (even without worrying about going super deep and just replacing the entire `spoiler` node with a new `text` node), we're going to be accepting an array parameter, `ASTNode[]` or `SingleASTNode[]`. We're also going to be returning an array of `nodes`, so I felt it made sense to map through each `node` in the AST. With a check like `if (content && typeof content === 'string') {`, since the parameter accepts an array, unlike just a `node` in the other functions, it makes sense to check only a node (since `ast.content` isn't a field)

Why can't the function accept an (array or single) parameter, just like rawTextFromMarkdownAST and markdownASTHasPressable?

New approach that uses two functions like @rohan initially did, but a little simplified with more comments

ashoat accepted this revision.Dec 5 2022, 10:30 AM

rohan removed 1 blocking reviewer(s): atul.Dec 5 2022, 10:35 AM

This revision is now accepted and ready to land.Dec 5 2022, 10:35 AM

Harbormaster completed remote builds in B13967: Diff 19136.Dec 5 2022, 10:42 AM

Please make sure to test this one closely before landing, as I didn't do too much testing myself 😅

lib/shared/markdown.js
284	I typo'd "arrays" here

Spent time testing and fixed typo - will land if builds pass

Harbormaster completed remote builds in B13968: Diff 19140.Dec 5 2022, 12:40 PM

Closed by commit rCOMM5a8aad609b04: [lib] Prevent spoiler text from appearing in MessagePreview (authored by rohan). · Explain WhyDec 5 2022, 12:41 PM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM5a8aad609b04: [lib] Prevent spoiler text from appearing in MessagePreview.

[lib] Prevent spoiler text from appearing in MessagePreview
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 19136

lib/shared/markdown.js

lib/shared/messages/text-message-spec.js

[lib] Prevent spoiler text from appearing in MessagePreviewClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 19136

lib/shared/markdown.js

lib/shared/messages/text-message-spec.js

[lib] Prevent spoiler text from appearing in MessagePreview
ClosedPublic
Actions

Revision Contents
Changeset List