-
Notifications
You must be signed in to change notification settings - Fork 614
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 not destroyed #1221
Conversation
Destroy webview by default on dispose
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.
Thank you for the bug report and the PR to fix it!
Could you please also add a basic test to ensure onDispose is called?
@@ -68,6 +70,7 @@ fun WebView( | |||
captureBackPresses: Boolean = true, | |||
navigator: WebViewNavigator = rememberWebViewNavigator(), | |||
onCreated: (WebView) -> Unit = {}, | |||
onDispose: (WebView) -> Unit = { it.destroy() }, |
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 don't mind exposing the callback but I think it.destroy()
should be moved to the DisposableEffect
onDispose
and always called. Is there a valid usecase for not calling it?
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 configuration changes. But it probably requires more work to handle it properly (cf #1178).
You'll also have to run
|
I added the minimal test, tell me if you prefer to not expose the onDispose callback |
That is a very good point about making that linked issue worse. This might actually be too big a breaking change to introduce in the rc stage. For now, let's remove the Once we are back to alphas, we can investigate where is the best place to call it. Anyone on 1.2 can use the new callback to call it themselves manually if required. |
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.
Thank you!
@bentrengrove Tests are not failing on my side 🤷♂️ |
Fix #1220
Give access to the webview during composable dispose run. By default, destroy the webview.
Use https://webbrowsertools.com/audio-test/ for testing (see #1220)