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

Preserve fragments in link previews #17889

Closed
jryans opened this issue Jul 6, 2021 · 7 comments · Fixed by matrix-org/matrix-react-sdk#6326
Closed

Preserve fragments in link previews #17889

jryans opened this issue Jul 6, 2021 · 7 comments · Fixed by matrix-org/matrix-react-sdk#6326

Comments

@jryans
Copy link
Collaborator

jryans commented Jul 6, 2021

When previewing links, we drop link fragments before sending to the homeserver for preview since they may contain private data and generally aren't meant to be sent to servers. However, the way we currently achieve this cause the fragment to be lost, so when we display the LinkPreviewWidget, the fragment is not used, resulting in a different link than expected.

We should keep the fragment around client side and carry it through to the LinkPreviewWidget's display of the link.

@t3chguy
Copy link
Member

t3chguy commented Jul 6, 2021

since they may contain private data and generally aren't meant to be sent to servers.

That isn't why we do it, we do it because if your message contains the same link with different fragments you end up with N identical looking previews

See #17494

@bmarty
Copy link
Member

bmarty commented Jul 6, 2021

I've created element-hq/element-android#3640 for Android, but maybe it is not a privacy problem?

Element Android displays only one 1 UrlPreview, for the first link in the message, so we do not have the same problem of multiple identical previews.

But the Fragment part is very important, so removing it when rendering the UrlPreview is a big loss of information.

@jryans
Copy link
Collaborator Author

jryans commented Jul 7, 2021

That isn't why we do it, we do it because if your message contains the same link with different fragments you end up with N identical looking previews

Ah didn't realise it was a recent change... still seems a bit odd privacy wise that we used to send the fragment when normally even the link destination doesn't receive that info...

Privacy aside, if we're continuing to remove the fragment for previewing, we should still keep it around for display, as otherwise we're making it harder to reach the link as sent. People are likely to click the link in the preview vs. the message, since the preview one is a bit larger, so it should preserve all fidelity, e.g. the fragment.

@t3chguy
Copy link
Member

t3chguy commented Jul 7, 2021

Right but how do we handle the same link with two different anchors without regressing the aforementioned issue?

@bmarty
Copy link
Member

bmarty commented Jul 7, 2021

Show a preview only for the first link :) as per Element Android. Multiple URL previews take too much space in the timeline IMHO. If the user wants to have a preview for each URL, they just have to send several messages.

@t3chguy
Copy link
Member

t3chguy commented Jul 7, 2021

There is another issue about that. But that mandates the ability to expand to all yet

@t3chguy
Copy link
Member

t3chguy commented Jul 7, 2021

Related #5433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants