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

Send correct window size on resize - this fixes popup crashes that happened #1166

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Send correct window size on resize - this fixes popup crashes that happened #1166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
572e171
[wasm] send correct window sized on resize
Schahen Mar 6, 2024
8af0963
Set only logical units to resize - it will be actually re-evaluated w…
Schahen Mar 6, 2024
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
3 changes: 2 additions & 1 deletion compose/mpp/demo/src/commonMain/kotlin/androidx/compose/mpp/demo/bug/BugReproducers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,6 @@ val BugReproducers = Screen.Selection(
IOSSelectionContainerCrash,
IOSDynamicKeyboardType,
NoPressInteractionInOutlinedTextField,
WebBaselineAlways0
WebBaselineAlways0,
ResizePopupCrashOnJS
)
67 changes: 67 additions & 0 deletions compose/mpp/demo/src/commonMain/kotlin/androidx/compose/mpp/demo/bug/ResizePopupCrashOnJS.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright 2024 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.compose.mpp.demo.bug

import androidx.compose.mpp.demo.Screen
import androidx.compose.material.ExperimentalMaterialApi
import androidx.compose.foundation.layout.*
import androidx.compose.material.*
import androidx.compose.runtime.*
import androidx.compose.ui.Modifier
import androidx.compose.ui.unit.dp

@OptIn(ExperimentalMaterialApi::class)
internal val ResizePopupCrashOnJS = Screen.Example("Resize Popup") {
MaterialTheme {
var expanded by remember { mutableStateOf(false) }
val items = listOf("Resize", "The", "Window")
var selectedOptionText by remember { mutableStateOf(items[0]) }
ExposedDropdownMenuBox(
expanded = expanded,
onExpandedChange = {
expanded = it
},
) {
OutlinedTextField(
modifier = Modifier
.fillMaxWidth()
.padding(top = 16.dp),
value = selectedOptionText,
onValueChange = {},
readOnly = true
)
ExposedDropdownMenu(
expanded = expanded,
onDismissRequest = {
expanded = false
},
) {
items.forEach { selectionOption ->
DropdownMenuItem(
onClick = {
selectedOptionText = selectionOption
expanded = false
},
) {
Text(selectionOption)

}
}
}
}
}
}
43 changes: 23 additions & 20 deletions compose/ui/ui/src/jsWasmMain/kotlin/androidx/compose/ui/window/ComposeWindow.js.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,14 +171,9 @@ private class ComposeWindow(
}

init {
layer.layer.attachTo(canvas)
initEvents(canvas)

canvas.setAttribute("tabindex", "0")
layer.layer.needRedraw()

_windowInfo.containerSize = IntSize(canvas.width, canvas.height)
layer.setSize(canvas.width, canvas.height)

layer.setDensity(density)
layer.setContent {
Expand All @@ -189,12 +184,22 @@ private class ComposeWindow(
}
}

fun resize(newSize: IntSize) {
canvas.width = newSize.width
canvas.height = newSize.height
_windowInfo.containerSize = IntSize(canvas.width, canvas.height)
fun resize(boxSize: IntSize) {
val density = density.density.toInt()

val width = boxSize.width * density
val height = boxSize.height * density

// TODO: What we actually should do is to pass this new dimensions to attachTo without setting them directly on canvas.
// We should be HTMLCanvas-agnostic in that sense
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should aim to be HTMLCanvas-agnostic in CavnasBasedWindow implementations.
It's easier to implement the details here rather than in skiko, IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

by agnostic I meant that ideally there should be only one place where we are setting canvas dimensions physically. Now we do it in three different places and this can get out of hand. If this place will be somewhere in compose this is indeed an issue of design. But I'd rather definitely width/height setting to one place.

canvas.width = boxSize.width
canvas.height = boxSize.height


_windowInfo.containerSize = IntSize(width, height)

layer.layer.attachTo(canvas)
layer.setSize(canvas.width, canvas.height)
layer.setSize(width, height)
layer.layer.needRedraw()
}

Expand Down Expand Up @@ -247,6 +252,11 @@ fun CanvasBasedWindow(
)
}

fun getParentContainerBox(): IntSize {
val documentElement = document.documentElement ?: return IntSize(0, 0)
return IntSize(documentElement.clientWidth, documentElement.clientHeight)
}

val actualRequestResize: suspend () -> IntSize = if (requestResize != null) {
requestResize
} else {
Expand All @@ -258,23 +268,16 @@ fun CanvasBasedWindow(
// because the default behaviour expects that the Canvas takes the entire window space,
// so the app has the same lifecycle as the browser tab.
window.addEventListener("resize", { _ ->
val w = document.documentElement?.clientWidth ?: 0
val h = document.documentElement?.clientHeight ?: 0
channel.trySend(IntSize(w, h))
channel.trySend(getParentContainerBox())
})

channel.trySend(getParentContainerBox())
Copy link
Member

Choose a reason for hiding this comment

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

We didn't have this before. Is it necessary to fix the issue? Or is it here for other reasons?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We trigger exactly the same event the very first time as we trigger on resize - thus the logic of resolving/assigning dimensions is uniform and less prone for bugs when this logic is slightly different the very first time and on resize (this is exactly what happened before actually)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need channel here? It is asynchronous, and we can have 1-frame lag/blink because of that.

Can we just write:

        var composeWindow: ComposeWindow? = null
        window.addEventListener("resize", { _ ->
            composeWindow?.resize(newSize)
        })
        composeWindow = ComposeWindow(...
        composeWindow?.resize(newSize)

(we need to rewrite the public API requestResize as well, so it is a suggestion for the future)

Copy link
Member

Choose a reason for hiding this comment

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

We intentionally do not react on every resize event anyway (there's a lot of them when we manually resize the browser window), because it's quite expensive. So we throttle the events anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we receive 1 resize event per frame, or it can be 10 events per frame? If it is the first, it shouldn't cause lags in theory.

We resize synchronously on desktop on every event. It is done intentionally, as the main relayout shouldn't be expensive - it just relayouts the top layout, and doesn't relayout the children. We strive to make resize as fast as the window resize, so users don't see black/white bars. We also have multiple reports that our resize still isn't ideal (because of AWT implementation we still have white bars).

As for resize on init - is there any reason to not make it synchronous? It isn't visible to user comparing to desktop (because the source code is loaded asynchronously), but I would still do that to avoid passing zero size on the first recomposition/layout - it can cause races with other init code.


Copy link
Collaborator

@igordmn igordmn Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A separate thing that is also important to mention (but not so important to fix) - if we still want async resize, better to subscribe on it inside a separate coroutine inside ComposeWindow instead of LaunchedEffect. When we subscribe inside LaunchedEffect:

  • a child (ComposeScene) manipulates its parent (Canvas), it can complicate the code in the future (it is simple right now).
  • the Coroutine dispatcher for effects is partially frame-bound. It is complicated to explain in 2 words, but the main rule - if a coroutine doesn't affect the Composable content and doesn't depend on it - don't place it inside LauchedEffect.

suspend {
channel.receive()
}
}

if (requestResize == null) {
(document.getElementById(canvasElementId) as? HTMLCanvasElement)?.let {
it.width = document.documentElement?.clientWidth ?: 0
it.height = document.documentElement?.clientHeight ?: 0
}
}

var composeWindow: ComposeWindow? = null
composeWindow = ComposeWindow(
canvasId = canvasElementId,
Expand Down
Loading