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

[url_launcher] Add an inAppBrowserView mode #5155

Merged
merged 21 commits into from
Oct 24, 2023

Conversation

stuartmorgan
Copy link
Contributor

url_launcher_android recently switched from an in-app webview to an Android Custom Tab (when supported), which was intended to be an in-place upgrade. However, this broke closeInAppWebView, and that couldn't be fixed directly because Android Custom Tab has no mechanism for programatic close. To address the regression, this adds a new inAppBrowserView launch mode which is distinct from inAppWebView, so that use cases that require programatic close can specifically request inAppWebView instead. The default for web links is the new inAppBrowserView since that gives better results in most cases.

Since whether closeInAppWebView will work in any given case is now non-trivial (on iOS, both in-app modes are supported, but on Android it's only the web view mode), this adds a new support API to query it, in keeping with the relatively new guidance of https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#api-support-queries. It also adds API to query for support for being able to use specific launch modes, since there wasn't a good way to understand which modes worked in general on different platforms.

Since there are new APIs, this adds support for those APIs to all of our implementations to ensure that they give accurate responses.

Fixes flutter/flutter#134208

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@@ -126,7 +126,7 @@ class DefaultLinkDelegate extends StatelessWidget {
success = await launchUrl(
url,
mode: _useWebView
? LaunchMode.inAppWebView
? LaunchMode.inAppBrowserView
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this since on Android we generally want to use Custom Tabs when possible, and it'll automatically fall back to webview when support isn't available.

/// required. The default behavior of [LaunchMode.platformDefault] is up to each
/// platform, and its behavior for a given platform may change over time as new
/// modes are supported, so clients that want a specific mode should request it
/// rather than rely on any currently observed default behavior.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the documentation federation-friendly and evergreen. If we find that people want docs about specific platform behaviors, we can add it to the platform package READMEs later.

Copy link
Member

Choose a reason for hiding this comment

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

Per-platform docs are the way, agreed!

}

@override
Future<bool> launchUrl(String url, LaunchOptions options) async {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason there's so much change here is that we never went back and implemented launchUrl, so we only had the legacy launch platform interface method. This adds the entire method so that we can support inAppBrowserView.

(That could be done as a prequel without the new mode, but it didn't seem worth splitting out just that small piece; if anyone would prefer that let me know.)

url,
// Prefer custom tabs unless a webview was specifically requested.
options.mode != PreferredLaunchMode.inAppWebView,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't distinguish between inAppBrowserView and platformDefault here because I don't know of a use case for trying to force Custom Tabs (which would, e.g., prevent the current "fall back if custom headers are set" logic). If anyone ever actually requests that we can change this to an enum (only web view, only Custom Tab, or whichever works).

expect(launched, true);
expect(api.usedWebView, false);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything from here up is pretty much copies of the existing legacy (launch) tests, but using launchUrl.

}

@override
Future<bool> launchUrl(String url, LaunchOptions options) async {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this isn't required for this PR, but it seemed like a good opportunity to add launchUrl since it's very similar to the Android version. I can make this a separate PR later if preferred.

expect(api.usedSafariViewController, false);
expect(api.passedUniversalLinksOnly, false);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As with Android tests, the above is copies of the existing tests but with the non-deprecated API.

@@ -51,4 +51,16 @@ class UrlLauncherLinux extends UrlLauncherPlatform {
},
).then((bool? value) => value ?? false);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add launchUrl to the desktop platforms in this PR because they don't currently support any mode switches, so it didn't seem as useful. (Someday I'll update them just so we can deprecate launch.)

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Web bits LGTM!

Future<bool> supportsMode(PreferredLaunchMode mode) async {
// Web doesn't allow any control over the destination beyond
// webOnlyWindowName, so don't claim support for any mode beyond default.
return mode == PreferredLaunchMode.platformDefault;
Copy link
Member

Choose a reason for hiding this comment

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

These are good defaults for the web platform. This maybe could be expanded more (if you use window.open, and keep a reference to the window that you opened, it can be closed, for example), but so far I have never heard from anybody wanting the "close" feature on the web.

# FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE.
# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins
dependency_overrides:
{url_launcher_platform_interface: {path: ../../../url_launcher/url_launcher_platform_interface}}
Copy link
Member

Choose a reason for hiding this comment

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

Here and elsewhere, I assume this will be backed out?

Copy link
Contributor Author

@stuartmorgan stuartmorgan Oct 21, 2023

Choose a reason for hiding this comment

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

Yes, this is the combo PR. Once it's approved I'll break it into three and land them in order, and these become constraint updates on the new version.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm modulo removal of FOR TESTING AND INITIAL REVIEW bits.

@stuartmorgan stuartmorgan added the federated: all_changes PR that contains changes for all packages for a federated plugin change label Oct 23, 2023
@stuartmorgan
Copy link
Contributor Author

#5205 splits out the first part for landing.

auto-submit bot pushed a commit that referenced this pull request Oct 23, 2023
Platform interface portion of #5155. Adds the `inAppBrowserView` launch mode, as well as the `supportsMode` and `supportsCloseForMode` support query APIs.
auto-submit bot pushed a commit that referenced this pull request Oct 24, 2023
Implementation package portion of #5155

This adds:
- Android support for the new `inAppBrowserView` launch mode which is distinct from `inAppWebView`, so that use cases that require programatic close can specifically request `inAppWebView` instead.
  - The default for web links is the new `inAppBrowserView` since that gives better results in most cases.
  - `inAppBrowserView` will still automatically fall back to `inAppBrowserView` in cases where it's not supported. (In the future, we might want to tune that based on feedback. We could instead have three modes: the webview-only mode we now have, the dynamic mode we now have iff the user requested `platformDefault`, and a new Android Custom Tabs-only if it was explicitly requested which would fail if it doesn't work.)
- iOS support for treating `inAppBrowserView` as identical to `inAppWebView`, since in practice that's what its `inAppWebView` mode has always been.
- Support on all platforms for the new `supportsMode` and `supportsCloseForMode` support query methods.

Fixes flutter/flutter#134208
@stuartmorgan
Copy link
Contributor Author

The sub-PRs have landed and published, and this has been updated to set the implementation package constraints to require versions that have the implementations of the new methods.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 24, 2023
@auto-submit auto-submit bot merged commit c6821f9 into flutter:main Oct 24, 2023
74 checks passed
@stuartmorgan stuartmorgan deleted the url-launcher-in-app-browser branch October 24, 2023 19:25
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 25, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 25, 2023
flutter/packages@2faf992...f751ffb

2023-10-25 [email protected] [ci] Enable legacy Android emulator tests (flutter/packages#5190)
2023-10-25 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.0 to 2.3.1 (flutter/packages#5209)
2023-10-25 [email protected] Replace gems dependency with ruby. (flutter/packages#5219)
2023-10-25 [email protected] Update the ftl hardware version. (flutter/packages#5224)
2023-10-25 [email protected] [ci] Disable Windows Arm64 stable CI (flutter/packages#5217)
2023-10-24 [email protected] [webview_flutter_wkwebview] Only set `limitsNavigationsToAppBoundDomains` when it is set to true (flutter/packages#5137)
2023-10-24 [email protected] [url_launcher] Add an `inAppBrowserView` mode (flutter/packages#5155)
2023-10-24 [email protected] [Android] Introduces API 34 emulators for subset of tests (flutter/packages#5105)
2023-10-24 [email protected] [url_launcher] Add an `inAppBrowserView` mode in implementations (flutter/packages#5211)
2023-10-24 [email protected] [ci] Fix legacy Android task names (flutter/packages#5191)
2023-10-24 [email protected] Update annotations deps to 1.7 (flutter/packages#4935)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
Platform interface portion of flutter#5155. Adds the `inAppBrowserView` launch mode, as well as the `supportsMode` and `supportsCloseForMode` support query APIs.
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
…tter#5211)

Implementation package portion of flutter#5155

This adds:
- Android support for the new `inAppBrowserView` launch mode which is distinct from `inAppWebView`, so that use cases that require programatic close can specifically request `inAppWebView` instead.
  - The default for web links is the new `inAppBrowserView` since that gives better results in most cases.
  - `inAppBrowserView` will still automatically fall back to `inAppBrowserView` in cases where it's not supported. (In the future, we might want to tune that based on feedback. We could instead have three modes: the webview-only mode we now have, the dynamic mode we now have iff the user requested `platformDefault`, and a new Android Custom Tabs-only if it was explicitly requested which would fail if it doesn't work.)
- iOS support for treating `inAppBrowserView` as identical to `inAppWebView`, since in practice that's what its `inAppWebView` mode has always been.
- Support on all platforms for the new `supportsMode` and `supportsCloseForMode` support query methods.

Fixes flutter/flutter#134208
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
`url_launcher_android` recently switched from an in-app webview to an Android Custom Tab (when supported), which was intended to be an in-place upgrade. However, this broke `closeInAppWebView`, and that couldn't be fixed directly because Android Custom Tab has no mechanism for programatic close. To address the regression, this adds a new `inAppBrowserView` launch mode which is distinct from `inAppWebView`, so that use cases that require programatic close can specifically request `inAppWebView` instead. The default for web links is the new `inAppBrowserView` since that gives better results in most cases.

Since whether `closeInAppWebView` will work in any given case is now non-trivial (on iOS, both in-app modes are supported, but on Android it's only the web view mode), this adds a new support API to query it, in keeping with the relatively new guidance of https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#api-support-queries. It also adds API to query for support for being able to use specific launch modes, since there wasn't a good way to understand which modes worked in general on different platforms.

Since there are new APIs, this adds support for those APIs to all of our implementations to ensure that they give accurate responses.

Fixes flutter/flutter#134208
@reidbaker reidbaker mentioned this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App federated: all_changes PR that contains changes for all packages for a federated plugin change p: url_launcher platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

closeInAppWebView() not implemented for Android Custom Tabs (url_launcher version 6.1.14)
3 participants