Skip to content

Conversation

@sztobar
Copy link
Collaborator

@sztobar sztobar commented Apr 29, 2025

Summary

Test plan

  • checked current demo web page and found no regressions. It doesn't provider bug with url change inside webview that this PR fixes, so it has to be tested on a different app

* update API changes that includes updating visitableURL to initialVisitableURL and currentVisitableURL
Comment on lines +120 to +127
func session(_ session: Session, decidePolicyFor navigationAction: WKNavigationAction) -> WebViewPolicyManager.Decision {
guard let url = navigationAction.request.url else {
return .allow
}
// regardless of the return value here nothing happens, so we have to manually open external URL
visitableView?.didOpenExternalUrl(url: url)
return .allow
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This has introduced a new behavior in our app. This seems to be called for on-page <iframe src=".."> elements.

As an example, in our app we use Google Tag Manager. They have an initialization script that includes an <iframe src="https://www.googletagmanager.com/ns.html?...">. On every page view in the app, we see that didOpenExternalUrl is called with the Google Tag Manager URL with no way to differentiate if it was a user-decided navigation action or an on-page iframe.

Copy link
Contributor

@pfeiffer pfeiffer May 1, 2025

Choose a reason for hiding this comment

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

Hotwire Native has an extension that provides some additional properties to indicate if the navigationAction happened in the main frame etc: https://github.com/hotwired/hotwire-native-ios/blob/f18b87938f55cbe8976e96702b5512fa9afcd674/Source/Turbo/Navigator/Extensions/WKNavigationAction%2BUtils.swift#L1C1-L12C1

I think there's a few things going on here:

  1. The previous Session#openExternalURL that we relied on to trigger the didOpenExternalUrl on the RN-side was changed in Hotwire Native iOS to support the cross-origin redirects.
  2. We have not yet fully supported the change
  3. The current implementation in this PR is to naive and triggers even for iframes etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened a PR #225 that addresses this.

@pfeiffer
Copy link
Contributor

After extensive testing, I think this isn't ready to merge. I'll see if I can wrap up a PR that addresses the issues separately - there are a few regressions here besides the one in #225.

@piaccho
Copy link
Collaborator

piaccho commented Jun 12, 2025

Hello @pfeiffer 👋 I was planning to merge #225 and wanted to check if this PR fully addresses the three issues you mentioned there. Regarding the unresolved regressions – could you clarify what they are exactly? If possible, a minimal, reproducible example would be super helpful. I've just started working with this repository and am trying to understand the context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants