-
Notifications
You must be signed in to change notification settings - Fork 597
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
[Web] Fix webview blocking js reload #1344
Conversation
@@ -204,6 +204,12 @@ open class AccompanistWebViewClient : WebViewClient() { | |||
view: WebView?, | |||
request: WebResourceRequest? | |||
): Boolean { | |||
// If the url hasn't changed, this is probably an internal event like | |||
// a javascript reload. We should let it happen. | |||
if (view?.url == request?.url.toString()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding it correctly, this solution is to allow internal WebView
reload requests to pass through without overriding loading.
I'm wondering if there's an alternative solution using some sort of unique ID to compare against instead of url != view.url
to call view.loadUrl
.
Then, instead of returning false
here, we'd update the unique ID representing the particular request which would trigger a reload.
That might keep the number of codepaths down, and be closer to a more single-source-of-truth?
Not sure if that would change how we would want to do normal reloading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I could find, you get barely any information about the request. I couldn't even tell it was from a js event in this callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about something like
request?.let {
state.content = state.content.withUrl(it.url.toString())
state.requestId = UUID.randomUUID()
}
And then that requestId
could act like a key()
to trigger a reload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not quite that easy unfortunately because we also have to worry about if the page is displaying html data or a url. We don't get that information here to be able to update our state correctly. Worth investigating further in the future but will merge this fix for now.
3703047
to
32c02e6
Compare
Fixes #1335