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

Show available actions on embed error page #4760

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

johansenja
Copy link

Context

If you have privacyRedirect installed in your browser, any embedded youtube videos are automatically replaced by an invidious equivalent on pages you visit. The trouble is, often embeds don't always work - if 1/ the selected instance is down; 2/ the video is blocked or similar.

Current flow

If you particularly want to watch the video, you might right-click the iframe, click open in new tab, then replace the /embed/ part of the url with /watch?v= - or replace the domain with youtube.com to see the youtube version of it (but still as an embed)

Desired flow

It would be great if you had the same actions available on regular video error pages (switch instance / watch on youtube / refresh)

Caveats

It's worth noting that different browsers handle links inside iframes differently; eg. firefox often prompts you to open the link in a new tab - which IMO is ok, because if you are interested enough in the video to want to watch it, you would probably prefer to at least have this automatically, rather than doing it manually?

Chrome, on the other hand, doesn't even give you the option to "open frame in new tab" when you right click it, but it does seem to follow urls within the frame

Summary of changes in PR

  • Add request_path.starts_with?("/embed") check to error_redirect_helper

  • Move HTML string from error_redirect_helper into separate template
  • Add unit tests for error_redirect_helper
  • Add ContextWithPreferences and test_env helpers to help with test setup/compilation

The first point is obviously the main functional change, and the latter 3 are IMO code quality improvements (so they can be modified/removed if they are not helpful here)

Testing

I don't have this set up fully e2e locally so haven't checked this fully, but I found the unit tests went a decent way to verifying the behaviour I wanted

@johansenja johansenja requested a review from a team as a code owner June 21, 2024 17:46
@johansenja johansenja requested review from syeopite and removed request for a team June 21, 2024 17:46
Comment on lines +183 to +187
steps = {
refresh => env.request.resource,
switch_instance => "/redirect?referer=#{env.get("current_page")}",
go_to_youtube => "https://youtube.com#{env.request.resource}"
}
Copy link
Member

Choose a reason for hiding this comment

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

Note that by doing that, you're creating a Hash whose index is the translated string. Using an NamedTuple is more efficient as there is no need to hash a potentially long UTF-8 string:

Suggested change
steps = {
refresh => env.request.resource,
switch_instance => "/redirect?referer=#{env.get("current_page")}",
go_to_youtube => "https://youtube.com#{env.request.resource}"
}
steps = {
{label: refresh, url: env.request.resource},
{label: switch_instance, url: "/redirect?referer=#{env.get("current_page")}"},
{label: go_to_youtube, url: "https://youtube.com#{env.request.resource}"}
}

Comment on lines +5 to +11
it "shows next steps on embed page errors" do
current_url = "/embed/IeQT18gaB-c?si=YxBQzH-GBSTS4vBS"
test_env = test_env current_url
test_env.set "current_page", current_url

html = error_redirect_helper(test_env)
expect(html).to eq "<p style=\"margin-bottom: 4px;\">After which you should try to: </p>\n<ul>\n \n <li>\n <a href=\"/embed/IeQT18gaB-c?si=YxBQzH-GBSTS4vBS\">Refresh</a>\n </li>\n \n <li>\n <a href=\"/redirect?referer=/embed/IeQT18gaB-c?si=YxBQzH-GBSTS4vBS\">Switch Invidious Instance</a>\n </li>\n \n <li>\n <a href=\"https://youtube.com/embed/IeQT18gaB-c?si=YxBQzH-GBSTS4vBS\">Go to YouTube</a>\n </li>\n \n</ul>\n"
Copy link
Member

Choose a reason for hiding this comment

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

For a second I though that we had an URL handling issue, but it's just that you're not passing an encoded URL like the HTTP server handlers would do (see how in the "Switch Invidious Instance" URL nothing after referer= is URL encoded?).

It also made me realize that the si= parameter isn't removed from the "Watch on Youtube URL" (for the record, this is used to track who shared the video)

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

Successfully merging this pull request may close these issues.

2 participants