[url_launcher] Fixed missing # in links href when opening in new tab on the web #4221

TheOneWithTheBraid · 2021-08-09T09:16:10Z

When using the Link widget, it's theoretically possible to open the link in a new tab, e.g. by dragging the widget in the tab bar of modern browsers. In This case, up to now, an invalid route was formed, as the leading # from Flutters named routes is not included in the Anchor node's href.

List which issues are fixed by this PR. You must list at least one issue.

Fixes [url_launcher] Missing # in links href when opening in new tab on the web flutter#87941

I'm not exactly sure whether it's possible to cover a behavior like drag outside of Flutter to a new tab in tests. As I don't think so, I left out such a test.

Pre-launch Checklist

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

google-cla · 2021-08-09T09:16:16Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

TheOneWithTheBraid · 2021-08-09T09:28:14Z

@googlebot I signed it!

stuartmorgan-g · 2021-08-09T13:22:30Z

Thanks for the submission.

I'm not exactly sure whether it's possible to cover a behavior like drag outside of Flutter to a new tab in tests. As I don't think so, I left out such a test.

Just because an end-to-end test may not be feasible does not mean a change can't have tests. You are transforming a property in a setter, which should be easily unit testable.

I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.

In the future, please don't check things in the checklist that you have not actually done. Asserting yourself that you don't think it needs a test isn't one of the options described here.

TheOneWithTheBraid · 2021-08-09T13:39:37Z

You are fully right, @stuartmorgan. I definitely should not tick a checkbox then there is an unanswered question on it, and you are right with the getter too. Thank you for the reminder. I just added a corresponding test, and it's passing (at least on my local machine).

ditman · 2021-08-14T00:23:31Z

packages/url_launcher/url_launcher_web/lib/src/link.dart

+      // in case an internal uri is given, a leading `#` must be added to avoid
+      // invalid links when opening in a new tab
+      if (!uri.hasScheme) {
+        href = '#' + href;
+      }


I am not entirely sure this is going to be true forever, for example, there's a new Uri Strategy that doesn't use the # and instead uses the browser history API.

/cc @mdebbar can you take a look at this fix and see if it can be applied as is?

You are right @ditman. This works only with the HashUrlStrategy. If someone uses the PathUrlStrategy or any other custom url strategy, it won't work.

We need to figure out a way to grab the current url strategy used by the app, and call prepareExternalUrl to obtain the correct path that should go in the link href.

May you tell me whether my idea is right:

Would it be possible to somehow publicly make available _customUrlStrategy in engine:lib/web_ui/lib/src/engine/window.dart:38?

That way, the platform interface could check the present url strategy, right?

Would you mind if I create a corresponding getter in the file?

Except of that it's nether public nor exported in the library, the UrlStrategy could be fetched from the web's platform dispatcher as in the following dummy code: EnginePlatformDispatcher.instance.(_windows[0]! as EngineFlutterWindow).browserHistory.urlStrategy; as depicted in engine:lib/web_ui/lib/src/engine/platform_dispatcher.dart:972

@TheOneWithTheBraid: at the plugins level we can't know (for now) what's the URL strategy picked by the current app (even if we exposed that information from the engine, it'd take a few months for it to trickle down to the stable channel), to add a # or not.

However, programmers of web apps do know what type of links their web app needs.

As an alternative solution, have you tried setting your link widget with a URL similar to:

#/another/internal/route instead of /another/internal/route (I think this was the example in the originally reported issue)?

Is the problem then that when clicking on the URL it doesn't work?

I think it might be easier to teach the Link widget/Flutter framework to recognize those URLs when dealing with the onClick of the Link element, rather than attempting to "fix" the URL that the programmer is setting in links.

(Note that now we have 2 URL strategies, but if more end up being added, we might end up needing some extra behavior for each of them!)

Apologies if I'm misunderstanding the problem, but as far as I'm currently understanding this, I think we're attempting a fix in the wrong component!

it might be useful to access to current URL strategy anyway, right?

Yes, I don't have any problem with some of that information being exposed (or maybe a Link generator component that is compatible with the current URL strategy). The biggest problem of exposing these web-only APIs (for now) is that there's not a good place to expose them in a consistent way. (this is not a problem exclusive of this particular API, it's a flutter web issue in general!)

The purpose of the link is to work as a "normal" a href, with regards to clicking, copy-pasting and dragging to the address bar; we need to review that flutter is not doing "too much" with links for them to work (because the browser will not do that much)

May you tell me whether my idea is right:

Would it be possible to somehow publicly make available _customUrlStrategy in engine:lib/web_ui/lib/src/engine/window.dart:38?

That way, the platform interface could check the present url strategy, right?

Would you mind if I create a corresponding getter in the file?

Yes, this should work.

However, programmers of web apps do know what type of links their web app needs.

The idea is that inside your app, you don't need to know how the URL is represented in the browser. It could be a simple path /route or a hash path #/route or something completely different, depending on the url strategy. But your app shouldn't care about this difference. Only the url strategy needs to know about this (and you could implement your own url strategy if you want to customize this).

The url strategy has two methods to handle this:

prepareExternalUrl: converts an app route name to the path that goes in the browser url bar and <a> elements, etc. For example, HashUrlStrategy.prepareExternalUrl adds a # to the beginning of the route name to make it a hash path.

getPath: converts a path from the browser url bar to a route name for the app to consume. For example, HashUrlStrategy.getPath gets the hash part of the browser location, and strips the # from it.

(Note that now we have 2 URL strategies, but if more end up being added, we might end up needing some extra behavior for each of them!)

It doesn't matter how many url strategies there are (actually, users can create their own custom url strategies today, so we already have more than two). The Link widget shouldn't do any #-specific behavior/logic. It should just call UrlStrategy.prepareExternalUrl and let it handle the hash if necessary.

maybe a Link generator component that is compatible with the current URL strategy

Ahh, there you go @mdebbar, the prepareExternalUrl from each strategy!

I now opened flutter/flutter#90708 exposing a getter required to call prepareExternalUrl. Let's wait for it to trickle down, I'd propose. Right?

@mdebbar I now use the newly implemented getter here. May you check whether my implementation is proper?

stuartmorgan-g · 2021-11-04T20:32:06Z

Marking as a draft to remove it from our review queue; please remove the draft designation once this can proceed on stable.

stuartmorgan-g · 2021-12-19T20:57:32Z

Was the necessary change in 2.8? I see that the commit has some 2.8.0-*.pre tags, which suggest yes, but I know there were some version oddities on master.

TheOneWithTheBraid · 2022-01-30T08:18:10Z

It seems like the urlStrategy getter trickled down to stable. Could you check whether this PR is now eligible for merging?

TheOneWithTheBraid · 2022-02-09T09:17:41Z

The PR is now rebased on the current HEAD. It would be wonderful to have it merged before new conflicts occur.

stuartmorgan-g · 2022-02-09T17:09:21Z

Could you check whether this PR is now eligible for merging?

It would be wonderful to have it merged before new conflicts occur.

We don't merge unreviewed PRs; this PR has not been approved by any reviewer.

packages/url_launcher/url_launcher_web/CHANGELOG.md

- fix the the CHANGELOG in order to use the requestd past-tense statements Signed-off-by: TheOneWithTheBraid <[email protected]>

mdebbar

Looks good to me! I would love for @ditman to take a look too in case there's some plugins-specific stuff that I missed.

packages/url_launcher/url_launcher_web/lib/src/link.dart

Co-authored-by: Mouad Debbar <[email protected]>

ditman

If @mdebbar is happy, I'm happy. I checked that the minimum version of flutter is >=2.10.0 so this only resolves for users in a modern enough version of the stable channel.

Ship it!

…n new tab on the web (flutter/plugins#4221)

google-cla bot added the cla: no label Aug 9, 2021

github-actions bot added p: url_launcher platform-web labels Aug 9, 2021

google-cla bot added cla: yes and removed cla: no labels Aug 9, 2021

stuartmorgan-g added the needs tests label Aug 9, 2021

stuartmorgan-g removed the needs tests label Aug 9, 2021

stuartmorgan-g requested a review from ditman August 14, 2021 00:08

ditman reviewed Aug 14, 2021

View reviewed changes

ditman requested a review from mdebbar August 14, 2021 00:24

TheOneWithTheBraid requested review from LHLL, bparrishMines, cyanglaz and gaaclarke as code owners October 18, 2021 08:33

github-actions bot added p: android_alarm_manager p: android_intent p: battery Edits files for a battery plugin p: camera p: connectivity p: cross_file p: device_info p: espresso p: file_selector p: flutter_plugin_android_lifecycle p: google_maps_flutter p: google_sign_in labels Oct 18, 2021

github-actions bot removed the p: android_intent label Oct 18, 2021

TheOneWithTheBraid changed the title ~~[url_launcher] Fixed missing # in links href when opening in new tab on the web~~ [url_launcher] WIP: Waiting for master to trickle down - Fixed missing # in links href when opening in new tab on the web Oct 18, 2021

stuartmorgan-g marked this pull request as draft November 4, 2021 20:31

godofredoc changed the base branch from master to main January 6, 2022 23:36

Merge branch 'main' into new-tab-fix

c81a83e

TheOneWithTheBraid marked this pull request as ready for review January 28, 2022 08:31

TheOneWithTheBraid changed the title ~~[url_launcher] WIP: Waiting for master to trickle down - Fixed missing # in links href when opening in new tab on the web~~ [url_launcher] Fixed missing # in links href when opening in new tab on the web Jan 28, 2022

Fixed integration test mocks

4bf3520

Merge branch 'main' into new-tab-fix

0debd15

stuartmorgan-g requested review from ditman and removed request for LHLL, bparrishMines, cyanglaz and gaaclarke February 9, 2022 17:10

stuartmorgan-g reviewed Feb 9, 2022

View reviewed changes

packages/url_launcher/url_launcher_web/CHANGELOG.md Outdated Show resolved Hide resolved

fix: requested tense in CHANGELOG

95701e5

- fix the the CHANGELOG in order to use the requestd past-tense statements Signed-off-by: TheOneWithTheBraid <[email protected]>

mdebbar approved these changes Feb 23, 2022

View reviewed changes

packages/url_launcher/url_launcher_web/lib/src/link.dart Outdated Show resolved Hide resolved

Update packages/url_launcher/url_launcher_web/lib/src/link.dart

434e3ae

Co-authored-by: Mouad Debbar <[email protected]>

ditman approved these changes Feb 25, 2022

View reviewed changes

ditman added last mile waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. labels Feb 25, 2022

fluttergithubbot merged commit 06922c0 into flutter:main Feb 25, 2022

TheOneWithTheBraid deleted the new-tab-fix branch February 28, 2022 06:40

engine-flutter-autoroll mentioned this pull request Feb 28, 2022

Roll Plugins from 351415caa735 to f96d633e7a54 (3 revisions) flutter/flutter#99275

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 28, 2022

06922c04f [url_launcher] Fixed missing # in links href when opening i…

91a6659

…n new tab on the web (flutter/plugins#4221)

[url_launcher] Fixed missing # in links href when opening in new tab on the web #4221

[url_launcher] Fixed missing # in links href when opening in new tab on the web #4221

Uh oh!

Conversation

TheOneWithTheBraid commented Aug 9, 2021 • edited by ditman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

google-cla bot commented Aug 9, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

TheOneWithTheBraid commented Aug 9, 2021

Uh oh!

stuartmorgan-g commented Aug 9, 2021

Uh oh!

TheOneWithTheBraid commented Aug 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheOneWithTheBraid Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Nov 4, 2021

Uh oh!

stuartmorgan-g commented Dec 19, 2021

Uh oh!

TheOneWithTheBraid commented Jan 30, 2022

Uh oh!

TheOneWithTheBraid commented Feb 9, 2022

Uh oh!

stuartmorgan-g commented Feb 9, 2022

Uh oh!

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TheOneWithTheBraid commented Aug 9, 2021 •

edited by ditman

Loading

TheOneWithTheBraid Aug 27, 2021 •

edited

Loading