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

Fix broken external links #1395

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

Conversation

AllegraCodes
Copy link

Changes
Links in metadata would open in the app and lock it up. I override the function shouldOverrideUrlLoading to launch external links with the system. External links are defined as those that aren't intercepted by shouldInterceptRequest and don't contain the server hostname.

Issues
Fixes #984

@jellyfin-bot jellyfin-bot added this to the v2.7.0 milestone May 22, 2024
@AllegraCodes AllegraCodes marked this pull request as ready for review May 22, 2024 17:50
@Maxr1998
Copy link
Member

Hi, thanks for the PR. Do you have a specific example where this happens? For me, external links open in the browser just fine. So this seems to be an issue in the web client for some links instead.

@AllegraCodes
Copy link
Author

Sure, I replicated the issue by editing the metadata of one of my items to have <a href="https://developer.android.com/guide/components/intents-filters#extras">my test link</a> in the Overview. Then I navigated to that item's page in the app and tapped the link, and it opened in the app, unresponsive to back gestures.
Screenshot_20240522-162040

@Maxr1998
Copy link
Member

Fascinating. What type of library, item and which metadata field was it?

@AllegraCodes
Copy link
Author

I always used the Overview field and the behavior is the same for a movie, show, playlist, or collection

@Maxr1998
Copy link
Member

I see, I can reproduce it now, thanks for providing all these details.

I'm worried that the current matchers aren't enough and would cause pages to open externally which shouldn't. Authentication components/plugins, for example. I feel like web should preferably handle this, but I'm not sure how.

@AllegraCodes
Copy link
Author

that's a good point, i'll keep looking into this to see how android handles these calls and try to make it more robust.

@Maxr1998
Copy link
Member

Thanks! Appreciate your work, hopefully you can find a good solution to this issue.

@AllegraCodes
Copy link
Author

AllegraCodes commented May 24, 2024

ok, so the docs offer some notes on when this method actually gets called, and it's fairly limited. It's called for navigations initiated by the web page, the user, or an HTTP redirect.

Notably, it's not called for any navigations initiated by the app, but that doesn't mean it's impossible to trigger it from the app's code. The WebViewFragment triggers it when it loads <serverUrl>/web on initialization (but not when it loads a javascript url from the lifecycle scope). This is allowed to load internally because it's on the same host (including port) as the server, and it's the only instance I found of the app itself triggering this method in my limited testing so far.

One way I could lock it down a bit further is to require the request be associated with a gesture (such as a click, but we only get a boolean, not the type) in order to load it externally. I think I'll add that now so any reviewers can see it.

With all that said, I'm an android and web app novice, so I could easily be missing something with how android works here.

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.

Links in episode "Overview" (<plot>) don't work as expected
3 participants