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

Add FXIOS-9064 ⁃ Investigate logic of "Open link in external app"" #20280

Conversation

dicarobinho
Copy link
Collaborator

πŸ“œ Tickets

Jira ticket
Github issue

πŸ’‘ Description

Added a new option under General Settings -> Open Links In Apps
For URL schemes like "http", "https", "blob", "file", user will have the option to be automatically redirected to the external apps or not.
"By default", WKWebView redirect the user to the external apps, where is possible.

πŸ“ Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@dicarobinho dicarobinho marked this pull request as ready for review May 16, 2024 09:56
@dicarobinho dicarobinho requested a review from a team as a code owner May 16, 2024 09:56
@dicarobinho
Copy link
Collaborator Author

Here is a short demo with the changes.
⬇️
https://github.com/mozilla-mobile/firefox-ios/assets/61138287/65cf22d1-e17d-407b-87fa-4f5122d24875

@dicarobinho dicarobinho requested a review from nbhasin2 May 16, 2024 10:29
@nbhasin2 nbhasin2 added the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label May 16, 2024
Comment on lines 2992 to 3001
public static let SettingsOpenLinksInAppsTitle = MZLocalizedString(
key: "Settings.OpenLinksInApps.Title",
tableName: nil,
value: "Open Links In Apps",
comment: "Title of setting to enable open links in apps when pressing links.")
public static let SettingsOpenLinksInAppsStatus = MZLocalizedString(
key: "Settings.OpenLinksInApps.Status",
tableName: nil,
value: "When pressing Links",
comment: "Description displayed under the ”Open Links In Apps” option")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get a copy review on these along with screenshot @dicarobinho

Copy link
Contributor

Choose a reason for hiding this comment

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

or if @afurlan-firefox gives the go ahead for strings then I am okay as well otherwise its best to get these double checked

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The setting title is from the Android app. Here is how them look.
Screenshot 2024-05-16 at 17 32 37
iphone

@ewachowiak
Copy link

  • Yes, keep the same string as Android but it should be "Open Links in Apps" (lowercase i) to be proper title case
  • This new option does not require any subcopy so please remove "When pressing links"
  • For the two options above this, the subcopy should not be in title case. Can we change to sentence case? So the new strings will be
  • "When opening Firefox"
  • "When long-pressing links"
    This will help us resolve some outstanding UX debt. Thank you.

@afurlan-firefox
Copy link
Collaborator

Hi! I wanted to add some context on the option. There is a small difference in between the Android and iOS behavior.
In iOS, the option means:

  • If On -> Apple will decide what to do. Meaning, it is not always opening the link in external app. It will be around 95% of the cases but not "always", like it is in Android.
  • If Off -> Firefox will override the Apple behavior and always open the links inside inside the navigator.

@nbhasin2 can add more context, but this is how Universal Links are managed at Apple and there is not much we can do here. So, that is why I want to make sure we have the right copy on the option.

@ewachowiak
Copy link

Based on competitive research and discussion with the team, we are recommending we both change the copy and reverse the meaning of the toggle.

New copy: "Block opening external apps"
New toggle meaning: Off by default (means links will open in external apps as determined by Apple), On means we will block external apps from opening and open the links in the browser instead.

…external app""

Changed logic from Open Links in Apps to Block opening external apps
@dicarobinho
Copy link
Collaborator Author

@ewachowiak Here is the change. For the other texts, my suggestion is to have another task.

Simulator Screenshot - iPhone 15 Pro Max - 2024-05-20 at 11 59 38

@ewachowiak
Copy link

ewachowiak commented May 20, 2024

Please excuse me for having Friday brain last week. I forgot that the setting itself needs to be in title case. Can we please change the copy to "Block Opening External Apps"? Sorry for this mistake.

Additionally, I am hoping we can still address the 2 subcopy strings that should be in sentence case:

  • "When Opening Firefox" --> "When opening Firefox"
  • When Long-pressing Links --> "When long-pressing links"

This will help us better standardize our settings and reduce UX debt.

@dicarobinho
Copy link
Collaborator Author

@ewachowiak This is how it looks.

Note: I also changed this:

"When Opening Firefox" --> "When opening Firefox"
When Long-pressing Links --> "When long-pressing links"

but the new texts will be available only when the localization team will do also the changes.

Simulator Screenshot - iPhone 15 Pro Max - 2024-05-20 at 17 03 21

Copy link
Contributor

@nbhasin2 nbhasin2 left a comment

Choose a reason for hiding this comment

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

@dicarobinho make sure to update the keys before merging

…external app""

Updated the keys for strings
@mobiletest-ci-bot
Copy link

Warnings
⚠️ Variable 'crossCircleFill' in Medium is out of alphabetical order.
⚠️ Variable 'cross' in Medium is out of alphabetical order.
Messages
πŸ“– Project coverage: 32.46%
πŸ“– Edited 4 files
πŸ“– Created 0 files

Client.app: Coverage: 31.06

File Coverage
AppSettingsTableViewController.swift 23.82% ⚠️
BrowserViewController+WebViewDelegates.swift 3.27% ⚠️

Generated by 🚫 Danger Swift against 22a9f66

@dicarobinho dicarobinho merged commit 81ea917 into mozilla-mobile:main May 22, 2024
9 of 10 checks passed
@willfs84
Copy link

Thank you for doing this. And when Firefox is in private mode, you set it to block whether that toggle is on or off? That would fix #20060 #6717 #8417 (it's been filed 3 times)

@nbhasin2 nbhasin2 removed the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label May 29, 2024
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.

None yet

6 participants