Page MenuHomePhabricator

[native] Block link press if spoiler is not yet revealed
ClosedPublic

Authored by rohan on Dec 6 2022, 1:22 PM.
Tags
None
Referenced Files
F3381030: D5835.diff
Thu, Nov 28, 3:44 AM
Unknown Object (File)
Sun, Nov 10, 8:46 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Subscribers

Details

Summary

If we have a link contained within a spoiler, we want to prevent the link from being visible/clickable until the spoiler is revealed. Here, we introduce MarkdownSpoilerContext since we need to keep track, on a per-spoiler basis, if there is a link within a spoiler. If there is a spoiler link, and it is not yet revealed, we do not want the onPress for MarkdownLink to fire.

The hierarchy in react-devtools for a MarkdownLink contained in a MarkdownSpoiler is:

MarkdownParagraph
  MarkdownSpoiler
    MarkdownLink

This is why I used Context, and text[0].props.target is only valid if a MarkdownLink is rendered below a MarkdownSpoiler, since MarkdownLink has a target prop.

Linear Task: https://linear.app/comm/issue/ENG-2233/make-spoiler-components-block-link-reveal-click-until-the-spoiler-is

Test Plan

Android/iOS video below

Before:

After:

Diff Detail

Repository
rCOMM Comm
Branch
spoiler_link
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan edited the test plan for this revision. (Show Details)
native/markdown/markdown-link.react.js
61 ↗(On Diff #19196)

I use optional chaining since if a link is not rendered in a spoiler, it will not have access to MarkdownSpoilerContext

native/markdown/styles.js
10 ↗(On Diff #19196)

The link colors look normal without this, but the problem I saw keeping this is that the color for MarkdownLink overrides the color for MarkdownSpoiler, so the link spoiler appears white and is visible through the spoiler

rohan requested review of this revision.Dec 6 2022, 1:35 PM

Remove markdownLink from colors.js

native/markdown/markdown-spoiler-context.js
6 ↗(On Diff #19222)

Using a Context here is helpful to limit further refactoring for any kind of pressables within a spoiler, such as when a user is mentioned. It's also useful since within the hierarchy

MarkdownParagraph
    MarkdownSpoiler
         MarkdownLink

there are Text components rendered in-between, making it difficult to just add another prop to pass down.

Adding @ashoat as blocking since he has much more context on this work

atul requested changes to this revision.Dec 7 2022, 12:48 PM

I think the current approach is kind of brittle. I think we can simplify it a good amount, left some comments inline.

Definitely feel free to re-request review if I'm misunderstanding or missing something, I don't have a ton of context on the recent work here.

native/markdown/markdown-link.react.js
59 ↗(On Diff #19222)

Are we unable to use the existing MarkdownContext to determine whether MarkdownLink is "within" a revealed spoiler?

If not, could we use MarkdownSpoilerContext to pass down the messageID and spoiler key so it can be determined using MarkdownContext within MarkdownLink?

I think that'd be a cleaner design rather than holding onto this spoilerContainsUnrevealedLink value

60 ↗(On Diff #19222)

I think from within the MarkdownLink component, linkWithinUnrevealedSpoiler would be a clearer name

native/markdown/markdown-spoiler.react.js
49 ↗(On Diff #19222)

Isn't it the spoiler that is "unrevealed" rather than the link?

50–53 ↗(On Diff #19222)

It seems kind of brittle to try to introspect whether a Spoiler contains a MarkdownLink be using the text[0].props?.target check.

Could we instead pass down the messageID and spoiler key to MarkdownLink? From MarkdownLink we can use the existing MarkdownContext to determine whether the link is within a revealed spoiler?

This revision now requires changes to proceed.Dec 7 2022, 12:48 PM

I think @atul covered all of the things I would cover – gonna resign for now (I'm out through Monday) and let @rohan iterate on that feedback, and then feel free to re-add me for a final check if you need

native/markdown/markdown-link.react.js
59 ↗(On Diff #19222)

Yeah basically @rohan would love more justification as to why we need yet another new context. Open to it but would love to know the alternatives you considered and why they aren't good

native/markdown/markdown-spoiler.react.js
50–53 ↗(On Diff #19222)

Agreed

Thanks for the feedback - I'll address the name changes in the next revision. I left a comment explaining why I felt the need for a new Context, but feel free to let me know if you guys still disagree. The short answer is the state.key for a link contained in a spoiler may not be the same state.key for that spoiler, so it wouldn't find the right spoiler in spoilerRevealed.

To get the messageKey & spoiler key (or even just the isRevealed state) down to MarkdownLink since it's not a "direct" ancestor (there are Text components rendered in between), I felt like Context would be the solution.

Screenshot 2022-12-09 at 4.43.01 PM.png (292×464 px, 38 KB)

native/markdown/markdown-link.react.js
59 ↗(On Diff #19222)

@atul @ashoat With the MarkdownContext, it's not "message-unique", so the same context is used for each of the messages (this is since we removed it from encompassing the text-messages, and moved it up to root.react.js. That's why we no longer keep track of spoilers/links with just a boolean, but instead a messageKey.

However, spoilerRevealed has the following structure (where parsedSpoilerIndex is the state.key passed in from rules.react.js:

[messageKey]: { 
    [parsedSpoilerIndex]: boolean
}

Because of this, the key for a spoiler may not be the same for the link contained within it. For example, in a message like:

Have you visited https://comm.app? You can go there from ||https://google.com||

The keys could be something like this:

https://comm.app --> 0 (since it's the first link)
||https://google.com|| --> 0 (since it's the first spoiler)
https://google.com --> 1 (since it's the second link)

Because of this, trying to access directly from spoilerRevealed within MarkdownLink doesn't seem too consistent, since in MarkdownSpoiler the spoiler would be at a key of 0 within the messageKey, but in MarkdownLink, we'd be accessing the link that is contained in the spoiler, but it has a key of 1, so it wouldn't find a match in the object.

As a result, I was looking for a solution that's more "message-unique", since we could then, for each spoiler, pass it's state down to it's children components, hence the new context. This can be extended to the @ feature that's currently being worked on since that could also be a pressable within a spoiler.

In retrospect, I agree with the approach being a bit fragile. Let me take a look at passing the messageKey and parsedSpoilerIndex in the context, or even better if I can just pass down the isRevealed state.

Respond to feedback & ensure isRevealed cannot be typed to boolean | string

native/markdown/markdown-link.react.js
64 ↗(On Diff #19284)

Defaulting to false here makes sense, since some MarkdownLink may not be a descendent of a MarkdownSpoiler. So in that case, linkWithinUnrevealedSpoiler = false, so we will have default link behavior onPress

native/markdown/markdown-spoiler.react.js
40 ↗(On Diff #19284)

Previously, flow typed isRevealed as boolean | string, but if we check the typeof messageKey === 'string', it evaluates to a boolean so it makes isRevealed consistently a boolean. I made this change part of this diff since it impacts how we use isRevealed here anyways

tomek removed a reviewer: atul. tomek added 1 blocking reviewer(s): ashoat.

I don't have too much context but it seems ok to me.

native/markdown/markdown-link.react.js
59 ↗(On Diff #19222)

For me it makes sense to have a context per spoiler: it is a lot simpler solution.

native/markdown/markdown-spoiler-context.js
9 ↗(On Diff #19284)

Can we make this line shorter?

native/markdown/markdown-spoiler.react.js
56 ↗(On Diff #19284)

Why do we include link in the name? It seems like this variable tells only if a spoiler was revealed and different components can react to it accordingly.

Rebase on D5873 since I need to disable onLongPress if the spoiler is unrevealed (otherwise it seems like a link spoiler cannot be revealed since it's waiting for an onLongPress - that's my theory)

native/markdown/markdown-spoiler-context.js
9 ↗(On Diff #19284)

I'm not too sure - seems like the eslint/prettier forces this line, unless we probably reduce the length of the name. Explicitly typing the Context seems to be the convention in the codebase. Do you have any suggestion?

native/markdown/markdown-spoiler.react.js
56 ↗(On Diff #19284)

Good point, let me update the name. It'll make extending it to other pressables easier as well.

ashoat requested changes to this revision.Dec 15 2022, 11:00 AM
ashoat added inline comments.
native/markdown/markdown-spoiler.react.js
38 ↗(On Diff #19396)

Easier way to do that is just to convert messageKey to a boolean, eg. !!messageKey

54 ↗(On Diff #19396)

Shorthand

native/markdown/styles.js
10 ↗(On Diff #19196)

Need a more thorough analysis here. Did you test both light and dark mode? Did you test linkification of Entrys on the Calendar (on both light/dark mode)? If the color was set here I assume there was some good reason, but maybe not

This revision now requires changes to proceed.Dec 15 2022, 11:00 AM
native/markdown/styles.js
10 ↗(On Diff #19196)

I didn't see any issues on either chats or calendars.

Simplify context and use !!messageKey

Ah, so in all those cases the link color is white... I mistakenly thought light/dark mode controls that, but forgot it's actually about the thread color. Can you make sure to test all of those surfaces (Calendar + Chat, iOS + Android) with a dark-color text (by changing the thread's color) before landing? No need to share a video this time :)

This revision is now accepted and ready to land.Dec 15 2022, 11:48 AM

Just tested, everything looks normal!

native/markdown/markdown-spoiler-context.js
9 ↗(On Diff #19284)

It is more than a convention: we have to assign a type to everything we export from a file, otherwise Flow will complain (this gives some context: https://medium.com/flow-type/types-first-the-only-supported-mode-in-flow-jan-2021-3c4cb14d7b6c).

Regarding the length, I can see that in other places we're also having the same issue, so it's fine to keep it as it is.