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

Do not fetch link cards for mentioned users #6934

Merged
merged 1 commit into from Oct 25, 2018

Conversation

@ThibG
Copy link
Collaborator

commented Mar 27, 2018

No description provided.

@ThibG ThibG force-pushed the ThibG:features/pleroma-compat branch 2 times, most recently from 8cc5022 to 35effb1 Mar 27, 2018

@ThibG ThibG requested a review from Gargron Mar 27, 2018

@Gargron

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

I don't like the idea of making a hacky solution even more hacky but also forcing all app developers to make this subtle adjustment. How about we process the HTML server-side and replace the URI with URL when applicable?

To be honest if we start doing server-side processing of that HTML, we're only a few steps away from parsing the HTML into plaintext with demarkated entities...

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2018

@Gargron I don't like the idea of rewriting HTML links because you're really rewriting links, as opposed to just adding handlers dynamically in the WebUI. If the URL and URI are different for a reason, this breaks the original status' meaning in some way.

@Gargron

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

Well... There is a reason the URL property exists in the first place, it's what should be linked to. Imagine an AP implementation that returns only JSON-LD from the id URI. What would be the point of linking to it in HTML that end-users are supposed to see? And if the HTML is not meant for end-users, then post-processing it server-side on our end is exactly in line with that, because we intend the HTML to be for end-users...

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2018

Yes I know, but hear me out: admittedly, that is an edge case, but what if I purposefully link to that JSON-LD-only URL in my toot, for instance to talk about some ActivityPub software? I wouldn't want that link to be rewritten in my back, that would be extremely confusing.

@MightyPork

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

is this the reason some foreign statuses show the user profile as an embed?

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2018

@MightyPork that's probably addressed by the last commit, yeah

@Gargron

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

@MightyPork Mastodon uses microformats rel="mention" to demarkate mention links and conversely skip them when fetching link previews. Other platforms don't do that so their mention links look just like any other links.

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2018

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2018

@Gargron actually, Mastodon uses CSS classes “u-url” and “mention”, and skips prefetching for links with a class of “u-url”.

@nightpool why not, but that's yet another thing to specify and push to other implementors of the fediverse…

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2018

Why would it have anything to do with other implementors in the fediverse? I'm just talking about for mastodon's API retrieval.

@nightpool

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2018

I'm not necessarily against this PR, it just seems like if we want a more stable solution, explicitly specifying the account id server-side before sending it to the client is the most robust and easy to implement one.

so something like <a href="whatever" data-mention-acct="899">@nightpool</a>

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 29, 2018

Ah ok, I thought you were talking about specifying the markup of the toot itself, and not something returned from the API.
I'm not sure how much I like it, but it makes sense. It's more work on the Mastodon backend side of things, and you still have to modify the clients to take advantage of it, but once it's done, if we ever do more elaborate guesswork, that could be kept to the backend.

@ThibG ThibG force-pushed the ThibG:features/pleroma-compat branch from 35effb1 to 5561b56 Apr 2, 2018

@lambadalambda

This comment has been minimized.

Copy link

commented Apr 3, 2018

btw, about that rel=mention, i didn't find it in the html the mastodon api returns, just class=mention. Where exactly are you using it?

@Gargron

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

btw, about that rel=mention

I got confused in my memory. It's rel=tag for hashtags, and class=u-url for mentions. rel=mention would have been consistent with rel=tag, but from a cursory google search, that does not seem to be a thing anywhere. rel=tag is indeed microformats.

@lambadalambda

This comment has been minimized.

Copy link

commented Apr 4, 2018

would it make sense for me to add that? where does the u-url come from?

@Gargron

This comment has been minimized.

Copy link
Member

commented Apr 8, 2018

@lambadalambda The u-url comes from hcard (microformats), see full markup: https://github.com/tootsuite/mastodon/blob/master/app/lib/formatter.rb#L226

I like @nightpool's suggestion with data-mention-account. I think the sanitizer strips out such attributes from remote HTML so it should be fine for clients to expect it to be valid & local.

@ThibG ThibG force-pushed the ThibG:features/pleroma-compat branch from 5561b56 to 698f993 Apr 19, 2018

@ThibG ThibG force-pushed the ThibG:features/pleroma-compat branch 3 times, most recently from 699f590 to fa59849 May 5, 2018

@Gargron

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Did this get adjusted on Pleroma's side? I would prefer to avoid these changes if possible.

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 28, 2018

Yeah, pleroma has been changed to play nice with Mastodon afaict. However, the preview card fetching code will still try to fetch mentioned users.

@ThibG ThibG force-pushed the ThibG:features/pleroma-compat branch from fa59849 to bb737c4 Oct 25, 2018

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 25, 2018

Updated to simply stop Mastodon from fetching cards for mentioned users

@Gargron Gargron changed the title Parse Pleroma-authored mentions properly Do not fetch link cards for mentioned users Oct 25, 2018

@Gargron Gargron merged commit 7fee968 into tootsuite:master Oct 25, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
cyber-gene added a commit to ikebuku-ro/mastodon that referenced this pull request Oct 26, 2018
Merge branch 'master' of github.com:tootsuite/mastodon
* 'master' of github.com:tootsuite/mastodon:
  Fix missing `mention` argument when processing incoming Create activities (tootsuite#9114)
  Skip link-back check if body is nil (tootsuite#9107)
  Remove character counter from edit profile (tootsuite#9100)
  Fix direct messages column not loading more items on scroll (tootsuite#9102)
  Fix conversations not being marked read on click (tootsuite#9103)
  Do not fetch preview card for mentioned users (tootsuite#6934)
  Allow inbox owner to view implicitly targeted ActivityPub payload (tootsuite#9093)
  cli: set exit_on_failure for all CLI classes (tootsuite#9094)

@ThibG ThibG deleted the ThibG:features/pleroma-compat branch Mar 14, 2019

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