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.

Sign up for GitHub

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

Jump to bottom

[Web] Fix webview blocking js reload #1344

Merged
merged 4 commits into from
Sep 27, 2022
Merged

[Web] Fix webview blocking js reload #1344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dac87c3
Fix webview blocking js reload
bentrengrove Sep 19, 2022
c86668a
Try fix navigate forward test
bentrengrove Sep 25, 2022
32c02e6
Disable web tests on 31
bentrengrove Sep 26, 2022
53f3293
Mark test as flaky
bentrengrove Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions web/src/androidTest/kotlin/com/google/accompanist/web/WebTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.FlakyTest
import androidx.test.filters.SdkSuppress
import com.google.common.truth.Truth.assertThat
import junit.framework.TestCase.assertEquals
import kotlinx.coroutines.flow.toCollection
import okhttp3.mockwebserver.MockResponse
import okhttp3.mockwebserver.MockWebServer
Expand All @@ -59,8 +60,8 @@ import java.util.concurrent.TimeUnit
@RunWith(AndroidJUnit4::class)
// Emulator image doesn't have a WebView until API 26
// Google API emulator image seems to be really flaky before 28 so currently we will set these tests
// to mine 29
@SdkSuppress(minSdkVersion = 28)
// to min 29 and max 30. 31/32 image is also really flaky
@SdkSuppress(minSdkVersion = 28, maxSdkVersion = 30)
class WebTest {
@get:Rule
val rule = createComposeRule()
Expand Down Expand Up @@ -469,6 +470,7 @@ class WebTest {
.check(webMatches(getText(), equalTo(LINK_TEXT)))
}

@FlakyTest
@Test
fun testNavigatorForward() {
lateinit var state: WebViewState
Expand Down Expand Up @@ -501,6 +503,7 @@ class WebTest {

navigator.navigateForward()
rule.waitUntil { navigator.canGoBack }
rule.waitForIdle()

assertThat(state.content.getCurrentUrl()).isEqualTo(LINK_URL)
}
Expand Down Expand Up @@ -617,6 +620,48 @@ class WebTest {
assertThat(isOnDisposeCalled).isTrue()
}

@Test
fun testJSReloadTriggersRefresh() {
lateinit var state: WebViewState
var pageStartedCalled = 0
val client = object : AccompanistWebViewClient() {
override fun onPageStarted(view: WebView?, url: String?, favicon: Bitmap?) {
super.onPageStarted(view, url, favicon)
pageStartedCalled++
}
}

rule.setContent {
state = rememberWebViewStateWithHTMLData(
data =
"""
<html><body>
<input id="button" type="button" value="Reload"
onclick="window.location.reload()" />
</body></html>
""".trimIndent()
)

WebTestContent(
webViewState = state,
idlingResource = idleResource,
client = client
)
}

rule.waitForIdle()

onWebView()
.withElement(findElement(Locator.ID, "button"))
.perform(webClick())

// Check the url remained about:blank
onWebView()
.check(webMatches(getCurrentUrl(), equalTo("about:blank")))

assertEquals("Page should be loaded twice", 2, pageStartedCalled)
}

private val webNode: SemanticsNodeInteraction
get() = rule.onNodeWithTag(WebViewTag)
}
Expand Down
6 changes: 6 additions & 0 deletions web/src/main/java/com/google/accompanist/web/WebView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

return false
}

// Override all url loads to make the single source of truth
// of the URL the state holder Url
request?.let {
Expand Down