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_android] Add support for Custom Tabs #4739

Merged

Conversation

rajveermalviya
Copy link
Contributor

@rajveermalviya rajveermalviya commented Aug 18, 2023

Implement support for Android Custom Tabs.

Custom Tabs will only be used if all of the following conditions are true:

  • launchMode == LaunchMode.inAppWebView (or LaunchMode.platformDefault; only if url is web url)
  • WebViewConfiguration.headers == {} (or if it only contains CORS-safelisted headers)

Fixes flutter/flutter#18589

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@rajveermalviya

This comment was marked as outdated.

@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch from e557810 to a40c741 Compare August 18, 2023 14:04
@rajveermalviya

This comment was marked as outdated.

@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch from a40c741 to 3568c2f Compare August 18, 2023 14:28
@rajveermalviya

This comment was marked as outdated.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Welcome @rajveermalviya, and thanks for the contribution!

This version introduces Chrome custom tabs to replace launching the URL in an external app, for LaunchMode.externalApplication. The thread flutter/flutter#18589 is a bit confusing, because that is the approach requested in the original issue description. But I think what we actually would like to do is to instead have Chrome custom tabs replace the custom WebView-based screen, for LaunchMode.inAppWebView.

See @stuartmorgan's comment at flutter/flutter#18589 (comment) suggesting that, and my two comments starting at flutter/flutter#18589 (comment) about why the existing custom WebView screen isn't a good UX.

Conversely, I think it wouldn't be appropriate for LaunchMode.externalApplication to open the URL in a browser that stays within the app, which is what a Chrome custom tab means from a user perspective.

The FLAG_ACTIVITY_REQUIRE_NON_BROWSER part is also useful but should probably be left for a separate PR; see below.

Intent nativeAppIntent =
new Intent(Intent.ACTION_VIEW, uri)
.addCategory(Intent.CATEGORY_BROWSABLE)
.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_REQUIRE_NON_BROWSER);
Copy link
Member

Choose a reason for hiding this comment

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

This part is useful functionality, but should be controlled by the launch mode — this is implementing LaunchMode.externalNonBrowserApplication.

An implementation of that would resolve flutter/flutter#66721 and would also be quite welcome. It's probably simplest to handle that as a separate PR, though. In order to make it properly conditional on the launch mode, I think it may also require changes in url_launcher_platform_interface.

@rajveermalviya
Copy link
Contributor Author

rajveermalviya commented Aug 22, 2023

Hello @gnprice 👋

I think what we actually would like to do is to instead have Chrome custom tabs replace the custom WebView-based screen, for LaunchMode.inAppWebView.

Yeah, I went with the updating LaunchMode.externalApplication mode because that's what Custom Tabs actually feels like - links are still opened by an external application (a browser in this case) but with a compact look, a custom tab.
If the WebView mode is replaced then that will indeed be a breaking change. Also Custom Tabs won't be able to completely replace them anyway, because WebView mode allows passing headers not present in CORS-safelisted headers list; Custom Tabs can't support that.
One way would be to break the users and tell them to migrate to using webview package for their exotic needs.

Also I wouldn't call them Chrome Custom Tabs, since it is supported by many browsers on Android, and the doc website also calls them Android Custom Tabs now. It's API is also part of androidx:browser jetpack library - nothing chrome specific anymore ^^.

Firefox Chrome
firefox chrome

The FLAG_ACTIVITY_REQUIRE_NON_BROWSER part is also useful but should probably be left for a separate PR; see below.

That was added to match the behavior of legacy launch (Intent(Intent.ACTION_VIEW).setData(uri)). So that there is no breaking change, only changed behavior is that if the link is open-able in a browser it will now open in a Custom Tab and if not it will be handed over to the corresponding native app, if any, the latter part remains the same.

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Aug 22, 2023

Thanks for the contribution, but along the lines of @gnprice's comments above this is not a change we would accept as written.

Yeah, I went with the updating LaunchMode.externalApplication mode because that's what Custom Tabs actually feels like - links are still opened by an external application (a browser in this case) but with a compact look, a custom tab.

That is not the way the custom tabs feature is described:

Custom Tabs are a feature in Android browsers that gives app developers a way to add a customized browser experience directly within their app.

Custom Tabs offer a better user experience than simply opening an external browser. They allow users to remain within the app while browsing

And importantly, it's also not the way they actually behave from a system perspective. If you open a custom tab and then pull up the app switching UI you will be shown as in the same app, not in your browser. It is closely analogous to the Safari View Controller used for launch-in-app mode on iOS.

If the WebView mode is replaced then that will indeed be a breaking change.

As currently written, this PR removes the ability to launch in a the user's browser, which is absolutely a breaking change. And one that violates the documented and intended behavior of that mode.

WebView mode allows passing headers not present in CORS-safelisted headers list; Custom Tabs can't support that.

We need to keep the webview for the foreseeable future anyway, to handle devices that don't have Custom Tab support; we can fall back to that codepath if any unsupported headers are passed (and add a comment to that effect on the header property) so that nothing regresses.

I'm going to mark this as a draft for now; if you are interested in reworking this to match the behavior we want for the plugin please mark it as ready for review once those changes are made.

@stuartmorgan stuartmorgan marked this pull request as draft August 22, 2023 11:21
@rajveermalviya
Copy link
Contributor Author

Thanks for clearing the doubts @stuartmorgan - I'll update the PR to use Custom Tabs when LaunchMode.inAppWebView
is used and fallback to actual WebView implementation if Custom Tabs are unavailable or if any headers passed are not present in CORS-safelisted headers list.

@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch 3 times, most recently from b753406 to 12bf608 Compare August 22, 2023 16:42
@rajveermalviya rajveermalviya marked this pull request as ready for review August 22, 2023 16:43
@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch from 12bf608 to 9783b14 Compare August 22, 2023 17:45
@rajveermalviya
Copy link
Contributor Author

(also updated the PR description to reflect current implementation)

@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch 2 times, most recently from 90960ac to c2fa46a Compare August 23, 2023 08:37
@stuartmorgan
Copy link
Contributor

Custom Tabs will only be used if all of the following conditions are true:
[...]

  • WebViewConfiguration.enableJavaScript == true
  • WebViewConfiguration.enableDomStorage == true

We don't need these checks; the docs already say they may not be supported on all platforms. It's fine for us to silently ignore the false setting on the Custom Tab codepath the same way they are already ignored on iOS.

@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch 2 times, most recently from 8e38d0b to b2c8f29 Compare August 23, 2023 14:16
@rajveermalviya
Copy link
Contributor Author

Thanks @stuartmorgan, updated the description & implementation to ignore them.

@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch from b2c8f29 to 1c0ba3a Compare August 23, 2023 15:30
@rajveermalviya rajveermalviya force-pushed the url_launcher_android-custom-tabs branch from c022497 to 84a09e7 Compare August 30, 2023 16:47
@gmackall
Copy link
Member

Thanks for the contribution, and for the fast iteration on reviews! I'll take a look this afternoon 🙂

@gmackall
Copy link
Member

Re running the checks, as it looks like the failures are due to infra

Copy link
Member

@gmackall gmackall left a comment

Choose a reason for hiding this comment

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

This mostly LGTM, I just have one question I want to clarify before approving!


// Checks if headers contains a CORS restricted header.
// https://developer.mozilla.org/en-US/docs/Glossary/CORS-safelisted_request_header
private static boolean containsRestrictedHeader(Map<String, String> headersMap) {
Copy link
Member

Choose a reason for hiding this comment

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

There are additional restrictions listed in the link - my understanding is that if we try to launch with the safelisted headers, but in a case that fails the additional restrictions, we will simply fail and fall back to the webview mode.

Do you have a sense of how frequently we would end up in this case of mismatch between containsRestrictedHeader, and the source of truth link? Is it worth complicating this check to make it line up fully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Digging into this a bit more, Chromium has a lot more exhaustive list of headers and checks for the values. Should the same be done here?
(Disclaimer; BSD licensed) - https://source.chromium.org/chromium/chromium/src/+/main:services/network/public/cpp/cors/cors.cc;l=278-425;drc=86339296c23927c8e8abb6cd81c8cb8d4bb3700e

Problem with not doing this correctly, that is check everything header+values, chrome & firefox will just ignore the restricted headers and the page will be opened in the custom tabs without those headers, it doesn't fail to open custom tab. So we can't fallback to webview if that happens, unless we do the check ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to not add a bunch of complexity without knowing if anyone actually needs it. I would vote for keeping the current basic check, and see if we get feedback about it. Anyone affected can trivially work around it in the meantime by just adding an arbitrary other header to the request (e.g., add 'do-not-use-custom-tabs': true to the header map).

Copy link
Member

Choose a reason for hiding this comment

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

Anyone affected can trivially work around it in the meantime by just adding an arbitrary other header to the request (e.g., add 'do-not-use-custom-tabs': true to the header map).

Good point, waiting and seeing if we get feedback SGTM with that in mind

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2023
@auto-submit auto-submit bot merged commit e668c43 into flutter:main Aug 31, 2023
72 checks passed
@stuartmorgan
Copy link
Contributor

This is now published. Thanks for the great contribution!

@rajveermalviya rajveermalviya deleted the url_launcher_android-custom-tabs branch September 1, 2023 08:21
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Sep 1, 2023
url_launcher plugin now supports the desired behavior, which is using
Android Custom Tabs, so we don't need the workaround of opening the
links in external browser.

Upstream PR:
 flutter/packages#4739

Fixes zulip#279
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Sep 1, 2023
`url_launcher` plugin now supports the desired behavior, which is using
Android Custom Tabs, so we don't need the workaround of opening the
links in external browser anymore, thus removed them.

Upstream PR:
 flutter/packages#4739

Fixes zulip#279
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 1, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 7, 2023
flutter/packages@e7d812c...22d4754

2023-09-07 engine-flutter-autoroll@skia.org Roll Flutter (stable) from ff5b5b5 to 2524052 (6 revisions) (flutter/packages#4866)
2023-09-07 maurits@vnbskm.nl [webview_flutter_platform_interface] Adds option to override console log (flutter/packages#4701)
2023-09-01 stuartmorgan@google.com [tools,pigeon] Update tooling to handle Windows build output changes (flutter/packages#4826)
2023-08-31 amuramoto@users.noreply.github.com [google_maps_flutter] Cloud-based map styling support (flutter/packages#3682)
2023-08-31 stuartmorgan@google.com [ci] Convert version presubmit check to LUCI (flutter/packages#4822)
2023-08-31 rajveer0malviya@gmail.com [url_launcher_android] Add support for Custom Tabs (flutter/packages#4739)
2023-08-31 tarrinneal@gmail.com [webview_flutter] update pigeon to 11 (flutter/packages#4821)
2023-08-31 engine-flutter-autoroll@skia.org Roll Flutter (stable) from e1e4722 to ff5b5b5 (1 revision) (flutter/packages#4823)
2023-08-31 engine-flutter-autoroll@skia.org Roll Flutter from 1fe2495 to c175cf8 (30 revisions) (flutter/packages#4825)

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 flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@reidbaker reidbaker mentioned this pull request Sep 15, 2023
11 tasks
gnprice added a commit to rajveermalviya/zulip-flutter that referenced this pull request Nov 7, 2023
This gets us Android Custom Tabs support, via Rajesh's upstream PR:
  flutter/packages#4739
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 p: url_launcher platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Chrome Custom Tab in url_launcher, instead of custom WebView activity
4 participants