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] Adds documentation that a launch needs to be triggered by a user action #5143

Merged

Conversation

nilsreichardt
Copy link
Contributor

@nilsreichardt nilsreichardt commented Oct 13, 2023

Some web browsers, such as Safari, may prevent URL launching if it is not triggered by a user action (e.g. a button click). Even if a user triggers a launch through a button click, if there is a delay due to awaiting a Future before the launch, the browser may still block it. This can be confusing if you don't know it. As flutter/flutter#78524 suggested, should this be documented. Therefore, I added a few sentences to the launchUrl.

Please let me know if this documentation is too long or not the right place.

Fixes flutter/flutter#78524

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.

@@ -37,6 +37,16 @@ import 'type_conversion.dart';
/// - "_self" opens the new URL in the current tab.
/// Default behaviour when unset is to open the url in a new tab.
///
/// Some web browsers, such as Safari, may prevent URL launching if it is not
Copy link
Member

Choose a reason for hiding this comment

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

I normally add these notes to the README of the web package under a "Limitations of the web platform" section (example, very similar to the Autoplay problem!), but I'm not sure if those are discoverable enough, since they're by default "hidden" in pub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree that it's a bit hidden and the README.md would also a good place 👍 If you or @stuartmorgan like the approach with README.md more, I could copy the structure of the video_player package (having a short note that there are limitations on web in the main README and a more detailed description in url_launcher_web package).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think putting the long explanation in the README and then referencing that here is probably the best option.

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 made the following changes:

  • Adding a link about the web limitations in the "Configuration" section of the README.md of url_launcher (like video_player has), please let me know if this is the wrong location
  • Adding a new section "Limitations on the Web platform" with the detailed documentation in the README.md of the url_launcher_web package
  • Just referencing to this limitation in the launchUrl method

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Looks good to me with nits, but @ditman should do the main review.


### A launch needs to be triggered by a user action

Some web browsers, such as Safari, may prevent URL launching if it is not triggered by a user action (e.g. a button click).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We generally try to wrap our markdown files at about 80 characters; could you wrap all of these long lines?


Even if a user triggers a launch through a button click, if there is a delay due to awaiting a Future before the launch, the browser may still block it. This is because the browser might perceive the launch as not being a direct result of user interaction, particularly if the Future takes too long to complete.

In such cases, you can use the `webOnlyWindowName` parameter, setting it to "_self", to open the URL within the current tab. Another approach is to ensure that the `uri` is synchronously ready.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is directly about the same thing as the previous text, so should not be a new paragraph.

@nilsreichardt nilsreichardt requested a review from ditman December 3, 2023 11:15
@nilsreichardt
Copy link
Contributor Author

@ditman I applied the suggestions by @stuartmorgan and resolved the merge conflicts 👍 It's ready for review :)

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.

Hey @nilsreichardt, I left a few tweaks to the docs and a link to a relevant page of the MDN, lmk if my changes make sense to you. We can land this once you adopt those changes (or something similar). Important to say that this is definitely not a safari-only thing, all browsers are doing this.

packages/url_launcher/url_launcher_web/README.md Outdated Show resolved Hide resolved
packages/url_launcher/url_launcher_web/README.md Outdated Show resolved Hide resolved
packages/url_launcher/url_launcher_web/README.md Outdated Show resolved Hide resolved
packages/url_launcher/url_launcher_web/CHANGELOG.md Outdated Show resolved Hide resolved
@nilsreichardt
Copy link
Contributor Author

@ditman Thanks for the suggestions ❤️ LGTM

@ditman
Copy link
Member

ditman commented Dec 8, 2023

Thanks! I've restarted a couple of flaky checks, and will apply autosubmit. This should land today.

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 8, 2023
@auto-submit auto-submit bot merged commit e848d62 into flutter:main Dec 8, 2023
80 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 11, 2023
flutter/packages@6cd0657...cb6dbcd

2023-12-09 stuartmorgan@google.com [multicast_dns] Adjust README (flutter/packages#5529)
2023-12-09 stuartmorgan@google.com [tool] Use ^ for Dart SDK (flutter/packages#5623)
2023-12-09 ditman@gmail.com [google_sign_in_web] Migrate to pkg:web. (flutter/packages#5612)
2023-12-08 stuartmorgan@google.com [google_maps_flutter] Disable failing iOS tests (flutter/packages#5629)
2023-12-08 43759233+kenzieschmoll@users.noreply.github.com Add `parse` constructors for the `BenchmarkResults` and `BenchmarkScore` classes (flutter/packages#5614)
2023-12-08 43054281+camsim99@users.noreply.github.com [path_provider_android] Disable `getExternalStorageDirectories (type: ...)` test (flutter/packages#5619)
2023-12-08 stuartmorgan@google.com [ci] Fix indentation in labeler.yml (flutter/packages#5625)
2023-12-08 102626803+drewroengoogle@users.noreply.github.com Update labeler to 5.0.0, fix yml format (flutter/packages#5580)
2023-12-08 me@nils.re [url_launcher] Adds documentation that a launch needs to be triggered  by a user action (flutter/packages#5143)

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://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
… by a user action (flutter#5143)

Some web browsers, such as Safari, may prevent URL launching if it is not triggered by a user action (e.g. a button click). Even if a user triggers a launch through a button click, if there is a delay due to awaiting a Future before the launch, the browser may still block it. This can be confusing if you don't know it. As flutter/flutter#78524 suggested, should this be documented. Therefore, I added a few sentences to the `launchUrl`.

Please let me know if this documentation is too long or not the right place.

Fixes flutter/flutter#78524
@reidbaker reidbaker mentioned this pull request Jan 12, 2024
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
… by a user action (flutter#5143)

Some web browsers, such as Safari, may prevent URL launching if it is not triggered by a user action (e.g. a button click). Even if a user triggers a launch through a button click, if there is a delay due to awaiting a Future before the launch, the browser may still block it. This can be confusing if you don't know it. As flutter/flutter#78524 suggested, should this be documented. Therefore, I added a few sentences to the `launchUrl`.

Please let me know if this documentation is too long or not the right place.

Fixes flutter/flutter#78524
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-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[url_launcher] Document the need to launch from a user action handler on web
3 participants