Bugfix FXIOS-7871 [v125] Opening a link in external app leaves a "loading" about:blank page behind #19288
Conversation
…es a "loading" about:blank page behind
cyndichin
requested review from
mattreaganmozilla
and removed request for
cyndichin
| <string>paypal</string> | ||
| <string>instagram</string> | ||
| <string>youtube</string> |
There was a problem hiding this comment.
I'm a bit confused: this might work for these specific URL schemes but there is a wide variety of schemes for other apps, wouldn't they also suffer from a similar problem?
There was a problem hiding this comment.
Yes, you right but this is the way that we can find if an app is installed or not. This apps should be manually added in this list and yea... may be other apps too.
| if navigationAction.canOpenExternalApp, let url = navigationAction.request.url { | ||
| UIApplication.shared.open(url) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Isn't this basically a similar fix to what was implemented for whatsapp? e.g.:
if url.scheme == "whatsapp" && UIApplication.shared.canOpenURL(url) {
UIApplication.shared.open(url, options: [:])
}
If so I'm just curious why we're not handling this in the same way? (*To clarify, what I mean is that the code for whatsapp and the paypal/YT/insta links seems to be applying a similar fix at first glance, but the code is slightly different, but it seems like all of these could be consolidated. LMK if I'm overlooking something though)
There was a problem hiding this comment.
It may look the same but is a little bit different...
(few code for reference)
guard !navigationAction.isInternalUnprivileged,
shouldRequestBeOpenedAsPopup(navigationAction.request)
else {
guard let url = navigationAction.request.url else { return nil }
if url.scheme == "whatsapp" && UIApplication.shared.canOpenURL(url) {
UIApplication.shared.open(url, options: [:])
}
return nil
}
For other apps than "whatsapp" like "youtube" or "instagram", "shouldRequestBeOpenedAsPopup" method returns true because "url.scheme" is https not "youtube" or "instagram" as on "whatsapp" case. (based on the code, from above)
In this case, the logic, may be a little bit different.
|
Going to close and re-open to trigger Bitrise |
Client.app: Coverage: 32.46
Generated by 🚫 Danger Swift against fe0ee22 |
There was a problem hiding this comment.
I believe I understand the intent of the fix and it's nice that we can work around the about:blank page 👍 . I'm a bit confused though because it appears to require us to whitelist every scheme/app that we want to fix the bug for. So while this might fix PayPal/YouTube etc there are many other apps that will still suffer from the same problem (AFAIU). I see that the ticket mentions:
I do NOT see this in Safari.
I DO see a similar behavior in Chrome
So it sounds like this isn't unique to Firefox, but I would've expected Apple to provide some kind of (more elegant) solution to this. 🤔
Change looks OK to me, though it would be nice if we could get additional 👀 on this from folks familiar with our deeplinking. (Maybe @PARAIPAN9 or @nbhasin2 ?)
|
@mattreaganmozilla BR is green, can we merge this? Alex, doesn't have merge capabilities. |
@nbhasin2 Yes I was planning to merge I was just hoping to get some additional eyes from folks familiar with deeplinking (maybe Sorin). But fine with me to merge now if needed. |
mattreaganmozilla
merged commit 85cebaa
into
mozilla-mobile:main
|
@Mergifyio backport release/124 release/125 |
❌ No backport have been created
GitHub error:
GitHub error: |
|
@Mergifyio release/v124 release/v125 |
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
|
@Mergifyio backport release/v124 release/v125 |
✅ Backports have been created
|
📜 Tickets
Jira ticket
Github issue
💡 Description
Verify if an app like Youtube is installed and open it when a link (e.g. Youtube link) is accessed.
Based on the issue description and comments, I added in the apps list (LSApplicationQueriesSchemes), Youtube, Instagram and PayPal.
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)