Skip to content

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Sep 5, 2024

I set out to fix icon alignment on embedded posts after the icon update, but ended up doing this. This updates post embeds to use the same code and design as the web UI.

Instead of giving people the iframe HTML code directly, the embed code will now use a blockquote with inline styles that links to the post in question. This is a much better fallback when the origin server is unresponsive or disappears in the future. The included script will still load the iframe and replace the fallback once the height has been determined. I've added some code to determine the height given that React rendering happens asynchronously and involves fetching the post data from the API.

The embed is overlaid with an anchor link leading to the post in a new tab. This is done primarily because some of the dynamic elements in the UI would change the content height, and the iframe height cannot be practically updated every time this happens. So for example, a post with a content warning will render with the content warning collapsed, and clicking "Show anyway" will open the post in a new tab, instead of expanding inside the iframe.

One exception are video and audio players, which have a higher z-index and pierce through the overlay, allowing video and audio to be played and controlled from the embed.

Before JS loads After JS loads
grafik grafik

@Gargron Gargron added the ui Front-end, design label Sep 5, 2024
@Gargron Gargron force-pushed the feature-embed-status-react branch from fb3e93a to a28dfdf Compare September 5, 2024 20:00
@Gargron Gargron marked this pull request as ready for review September 5, 2024 20:05
@Gargron Gargron force-pushed the feature-embed-status-react branch from a28dfdf to 13aec27 Compare September 5, 2024 21:49
@Gargron Gargron force-pushed the feature-embed-status-react branch 3 times, most recently from 836a329 to 1663d07 Compare September 6, 2024 05:14
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

There is some minor breakage with modals not working but still being offered from media player / media gallery.

Otherwise, the look is more consistent, which is an improvement.

The failure case from loading the script switches from displaying a badly-sized frame to displaying a placeholder that does not contain the content from the post. I am not completely sure how useful that is, but at least it doesn't look too bad.

An issue I have (which is not new, and not really made worse, but made different) is that we are still relying on including an arbitrary script from the server, which is not especially good practice, especially when there is not one Mastodon server, but countless ones that are operated by different people whom news sites and so on would be wise not to blindly trust.

I wonder if we could find a better way. Maybe having the text of the post in the fallback? Although that would not play nice with retraction or edition of the post.

@trwnh
Copy link
Member

trwnh commented Sep 6, 2024

I wonder if we could find a better way. Maybe having the text of the post in the fallback? Although that would not play nice with retraction or edition of the post.

Generally the convention in citations is to provide the text at the time it was cited, as well as a "date of access" or "date retrieved". So I'm of the opinion that you could put an HTML representation of the post in the "placeholder", generated at the time that the user clicks "embed this post". The JS could then, at minimum, load an indication of whether the post was edited or deleted since then. This could also be accomplished by the JS loading in the whole post as currently, but with the caveat of not necessarily overriding the citation in the HTML.

@Gargron Gargron force-pushed the feature-embed-status-react branch from 1663d07 to 1190cbb Compare September 6, 2024 18:23
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Sorry I have not been able to focus enough to fully review this PR yet. The DetailedStatus rewrite makes the PR a bit noisy (but the resulting code is cleaner).

I have a security concern about the way the iframes are loaded.

Additionally, I think it might be worthwhile to include the text as fallback in the “placeholder”.

@Gargron Gargron force-pushed the feature-embed-status-react branch from ed96865 to 1282353 Compare September 9, 2024 22:04
@Gargron Gargron force-pushed the feature-embed-status-react branch from 1282353 to cb07e1b Compare September 10, 2024 08:14
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I have still not managed to fully review all the Typescript changes, but the overall approach seems good and a small but real improvement on the current embeds.

Testing the change, I could not see any regression.

@Gargron Gargron added this pull request to the merge queue Sep 12, 2024
Merged via the queue into main with commit 3d46f47 Sep 12, 2024
33 checks passed
@Gargron Gargron deleted the feature-embed-status-react branch September 12, 2024 09:51
@stefanbohacek
Copy link

stefanbohacek commented Sep 13, 2024

Is it necessary to include the Mastodon logo? The SVG needs a lot of code, and if/when the logo changes, all the old embeds will look outdated.

@stefanbohacek
Copy link

Also, someone reported an issue when viewing a post made by someone on mastodon.social using an account on a server running 4.2.10 (in my case botsin.space).

Screenshot of the 'Embed this post' modal, with the actual embed code missing.

justinwritescode pushed a commit to justinwritescode/mastodon that referenced this pull request Sep 15, 2024
Co-authored-by: Claire <claire.github-309c@sitedethib.com>
nileane pushed a commit to nileane/mastodon that referenced this pull request Sep 17, 2024
Co-authored-by: Claire <claire.github-309c@sitedethib.com>
@stefanbohacek
Copy link

@Gargron One more thing I noticed while looking at the documentation for the blockquote element, perhaps the cite attribute could be used instead of the custom data-embed-url one?

<blockquote cite="https://mastodon.social/@user/123456789" class="mastodon-embed">
  ....
</blockquote>

The embed URL can then be constructed by appending /embed to the value of cite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui Front-end, design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants