[url_launcher] launchUrl always returns true for valid schemes on the web.

[Frank3K]

Description

Since noopener is used to open URLs, there is no way to determine if the browser has succesfully opened a given URL (MDN). Therefore success is now assumed for supported schemes.

Fixes flutter/flutter#139783.

Implementation largely follows suggestions by @ditman written down in a comment.

The only exception is that launchUrl does not returns a hard-coded true, but a canLaunch(url). That way false is returned for invalid schemes (e.g. javascript:, as is checked in an integration test).

Testing

Sample application

The following application can be used to validate the fix. Note that the print is executed using the current version of url_launcher_web (2.3.1) and that it does not execute when the fix from this PR is used.

main.dart

import 'package:flutter/material.dart';
import 'package:url_launcher/url_launcher.dart';

final Uri _url = Uri.parse('https://flutter.dev');

void main() => runApp(
      const MaterialApp(
        home: Material(
          child: Center(
            child: ElevatedButton(
              onPressed: _launchUrl,
              child: Text('Show Flutter homepage'),
            ),
          ),
        ),
      ),
    );

Future<void> _launchUrl() async {
  if (!await launchUrl(_url)) {
    // ignore: avoid_print
    print('Failed opening link.');
  }
}

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene 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 linked to 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, or this PR is exempt from CHANGELOG changes.
• 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.

[ditman]

Any chance of adding a test? Most of the tests of this package are in the example/integration_test directory.

[ditman]

This should just return true;, no need to call canLaunch or anything else.

(Question: if canLaunch returns false, does it prevent launchUrl from actually attempting to open a browser tab with a bogus URL? I don't think it would, right?)

[Frank3K]

Returning true will break the url_launcher_web_test test.

I'll update the code and the test.

Question: if canLaunch returns false, does it prevent launchUrl from actually attempting to open a browser tab with a bogus URL? I don't think it would, right?

In the current setup it won't. If we would want that, we could do a canLaunch earlier and bail-out. But I agree returning true is better; if the user of this package wants such behaviour he can do so himself by calling canLaunch first in his own code.

[ditman]

canLaunch is not super valuable on the web, the browser doesn't know if it can handle a protocol and will just attempt to open anything, I think

[ditman]

@Frank3K how about:

• html.Window? openNewWindow(String url, {String? webOnlyWindowName}) becomes:
  • bool openNewWindow(String url, {String? webOnlyWindowName})

If it does NOT call window.open -> return false
If it calls window.open -> return true (regardless of what window.open returns)

Then the implementation of launchUrl can be just:

return openNewWindow...

That way we can still return false if the user really tries to mess up (like opening a disallowed protocol).

(I can incorporate those changes to this PR myself, if you think they make sense)

[Frank3K]

Good idea 👍 . I applied the proposed changes and added a small test.

This PR is ready for re-review.

[Frank3K]

Thanks for your review.

Any chance of adding a test? Most of the tests of this package are in the example/integration_test directory.

I have checked the tests and they are already quite extensive. With the proposed return true change, I could add a test with a bogus scheme to validate that it always returns true. Would that suffice or can you indicate what additional tests would be of value?

[ditman]

Woops, dart format is complaining, I'll get that fixed.

[ditman]

The flutter-dashboard flagged this as "not having tests" earlier, but it does now: opening a bogus scheme returns true, regardless of what the browser does.

[ditman]

LGTM!